[libcamera-devel] [PATCH v4 3/4] ipa: ipu3: agc: Introduce lineDuration in IPASessionConfiguration
Umang Jain
umang.jain at ideasonboard.com
Fri Mar 11 14:49:57 CET 2022
Hi Kieran,
On 2/28/22 20:00, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2022-02-24 15:11:12)
>> Instead of having a local cached value for line duration, store it in
>> the IPASessionConfiguration::sensor structure.
>> While at it, configure the default analogue gain and shutter speed to
>> controlled fixed values.
>>
>> The latter is set to be 10ms as it will in most cases be close to the
>> one needed, making the AGC faster to converge.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> ---
>> v4: clean only frameContext at configure call.
>> [Kieran, I took your R-b if you are ok ?]
> Fine with me if I already gave it ;-)
>
> I'll skim down through this to be sure anyway.
>
> I have a worry about the context_ = {}; to clean / initialise it now
> that there is a utils::Duration in it.
>
> I think it can possibly be fine, but we probably need to validate it.
Good spotting!
I validated with,
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -293,7 +293,9 @@ int IPAIPU3::init(const IPASettings &settings,
}
/* Clean context */
+ context_.configuration.sensor.lineDuration = 345ms;
context_ = {};
+ LOG(IPAIPU3, Info) << "lineduration after reset : " <<
context_.configuration.sensor.lineDuration.get<std::milli>();
context_.configuration.sensor.lineDuration =
sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
gives an o/p:
[8:20:50.953925353] [29439] INFO IPAIPU3 ipu3.cpp:298 lineduration
after reset : 0
so it seems it should be fine.
>
>
>> ---
>> src/ipa/ipu3/algorithms/agc.cpp | 26 ++++++++++++++------------
>> src/ipa/ipu3/algorithms/agc.h | 3 +--
>> src/ipa/ipu3/ipa_context.cpp | 8 ++++++++
>> src/ipa/ipu3/ipa_context.h | 4 ++++
>> src/ipa/ipu3/ipu3.cpp | 28 +++++++++++++++-------------
>> 5 files changed, 42 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index a929085c..1eb1bcef 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10;
>> static constexpr double kRelativeLuminanceTarget = 0.16;
>>
>> Agc::Agc()
>> - : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s),
>> + : frameCount_(0), minShutterSpeed_(0s),
>> maxShutterSpeed_(0s), filteredExposure_(0s)
>> {
>> }
>> @@ -83,13 +83,13 @@ Agc::Agc()
>> *
>> * \return 0
>> */
>> -int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>> +int Agc::configure(IPAContext &context,
>> + [[maybe_unused]] const IPAConfigInfo &configInfo)
>> {
>> - stride_ = context.configuration.grid.stride;
>> + IPASessionConfiguration &configuration = context.configuration;
>> + IPAFrameContext &frameContext = context.frameContext;
>>
>> - /* \todo use the IPAContext to provide the limits */
>> - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>> - / configInfo.sensorInfo.pixelRate;
>> + stride_ = configuration.grid.stride;
>>
>> minShutterSpeed_ = context.configuration.agc.minShutterSpeed;
>> maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed,
>> @@ -99,8 +99,8 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>> maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
>>
>> /* Configure the default exposure and gain. */
>> - context.frameContext.agc.gain = minAnalogueGain_;
>> - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;
>> + frameContext.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
>> + frameContext.agc.exposure = 10ms / configuration.sensor.lineDuration;
>>
>> return 0;
>> }
>> @@ -182,9 +182,11 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
>> * \param[in] yGain The gain calculated based on the relative luminance target
>> * \param[in] iqMeanGain The gain calculated based on the relative luminance target
>> */
>> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
>> +void Agc::computeExposure(IPAContext &context, double yGain,
>> double iqMeanGain)
>> {
>> + const IPASessionConfiguration &configuration = context.configuration;
>> + IPAFrameContext &frameContext = context.frameContext;
>> /* Get the effective exposure and gain applied on the sensor. */
>> uint32_t exposure = frameContext.sensor.exposure;
>> double analogueGain = frameContext.sensor.gain;
>> @@ -200,7 +202,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
>> /* extracted from Rpi::Agc::computeTargetExposure */
>>
>> /* Calculate the shutter time in seconds */
>> - utils::Duration currentShutter = exposure * lineDuration_;
>> + utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
>>
>> /*
>> * Update the exposure value for the next computation using the values
>> @@ -247,7 +249,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
>> << stepGain;
>>
>> /* Update the estimated exposure and gain. */
>> - frameContext.agc.exposure = shutterTime / lineDuration_;
>> + frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration;
>> frameContext.agc.gain = stepGain;
>> }
>>
>> @@ -354,7 +356,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>> break;
>> }
>>
>> - computeExposure(context.frameContext, yGain, iqMeanGain);
>> + computeExposure(context, yGain, iqMeanGain);
>> frameCount_++;
>> }
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
>> index 84bfe045..ad705605 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -34,7 +34,7 @@ private:
>> double measureBrightness(const ipu3_uapi_stats_3a *stats,
>> const ipu3_uapi_grid_config &grid) const;
>> utils::Duration filterExposure(utils::Duration currentExposure);
>> - void computeExposure(IPAFrameContext &frameContext, double yGain,
>> + void computeExposure(IPAContext &context, double yGain,
>> double iqMeanGain);
>> double estimateLuminance(IPAFrameContext &frameContext,
>> const ipu3_uapi_grid_config &grid,
>> @@ -43,7 +43,6 @@ private:
>>
>> uint64_t frameCount_;
>>
>> - utils::Duration lineDuration_;
>> utils::Duration minShutterSpeed_;
>> utils::Duration maxShutterSpeed_;
>>
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 86794ac1..9c4ec936 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -86,6 +86,14 @@ namespace libcamera::ipa::ipu3 {
>> * \brief Maximum analogue gain supported with the configured sensor
>> */
>>
>> +/**
>> + * \var IPASessionConfiguration::sensor
>> + * \brief Sensor-specific configuration of the IPA
>> + *
>> + * \var IPASessionConfiguration::sensor.lineDuration
>> + * \brief Line duration in microseconds
>> + */
>> +
>> /**
>> * \var IPAFrameContext::agc
>> * \brief Context for the Automatic Gain Control algorithm
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index c6dc0814..e7c49828 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -31,6 +31,10 @@ struct IPASessionConfiguration {
>> double minAnalogueGain;
>> double maxAnalogueGain;
>> } agc;
>> +
>> + struct {
>> + utils::Duration lineDuration;
>> + } sensor;
>> };
>>
>> struct IPAFrameContext {
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index c0c29416..17324616 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -173,8 +173,6 @@ private:
>> uint32_t minGain_;
>> uint32_t maxGain_;
>>
>> - utils::Duration lineDuration_;
>> -
>> /* Interface to the Camera Helper */
>> std::unique_ptr<CameraSensorHelper> camHelper_;
>>
>> @@ -206,8 +204,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>> *
>> * \todo take VBLANK into account for maximum shutter speed
>> */
>> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_;
>> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_;
>> + context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
>> + context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> Some long lines, but those might only be easier to shorten if we had a
> local reference to the configuration structure or sensor configuration?
yes, there are few as well in algorithms too. Those need to trimmed as
well I think.
Local reference seems a solution, but then would be not be neat when the
context itself becomes large and we are using local references all the IPA.
I see some upside while code reading - that the 'x value' comes the
context - which will be a hidden when using local references. Just my
preference.
>
>> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
>> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
>> }
>> @@ -229,6 +227,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>> ControlInfoMap *ipaControls)
>> {
>> ControlInfoMap::Map controls{};
>> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>>
>> /*
>> * Compute exposure time limits by using line length and pixel rate
>> @@ -237,9 +236,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>> * microseconds.
>> */
>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
>> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.get<std::micro>();
>> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.get<std::micro>();
>> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.get<std::micro>();
>> + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
>> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
>> + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
>> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
>> defExposure);
>>
>> @@ -293,6 +292,10 @@ int IPAIPU3::init(const IPASettings &settings,
>> return -ENODEV;
>> }
>>
>> + /* Clean context */
>> + context_ = {};
> Context_ now contains a utils::Duration which is a std::chrono::duration
> type.
>
> What happens to that /object/ when this context is cleaned? Does it lose
> any state? Or does it correctly get initialised successfully?
>
> We might want to validate that this is safe C++ ... I'm quite fearful
> that it might not be.
Answered and validated above. Should be okay!
>
>
>
>> + context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>> +
>> /* Construct our Algorithms */
>> algorithms_.push_back(std::make_unique<algorithms::Agc>());
>> algorithms_.push_back(std::make_unique<algorithms::Awb>());
>> @@ -454,12 +457,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>
>> defVBlank_ = itVBlank->second.def().get<int32_t>();
>>
>> - /* Clean context at configuration */
>> - context_ = {};
>> -
>> calculateBdsGrid(configInfo.bdsOutputSize);
>>
>> - lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
>> + /* Clean frameContext at each reconfiguration. */
>> + context_.frameContext = {};
> Yes, I think this is likely required for now, but should go when we get
> a per-frame frameContext.
Ah something for me to take note of for my (already submitted)
IPAFrameContext queue series o_O
>
>> /* Update the camera controls using the new sensor settings. */
>> updateControls(sensorInfo_, ctrls_, ipaControls);
>> @@ -620,6 +621,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>> [[maybe_unused]] int64_t frameTimestamp,
>> const ipu3_uapi_stats_3a *stats)
>> {
>> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>> ControlList ctrls(controls::controls);
>>
>> for (auto const &algo : algorithms_)
>> @@ -628,14 +630,14 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>> setControls(frame);
>>
>> /* \todo Use VBlank value calculated from each frame exposure. */
>> - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>();
>> + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration;
>> ctrls.set(controls::FrameDuration, frameDuration);
>>
>> ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
>>
>> ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);
>>
>> - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>());
>> + ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
>>
>> /*
>> * \todo The Metadata provides a path to getting extended data
>> --
>> 2.32.0
>>
More information about the libcamera-devel
mailing list