[libcamera-devel] [PATCH v3 3/3] ipa: ipu3: agc: Introduce lineDuration in IPASessionConfiguration
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Thu Feb 10 13:35:34 CET 2022
Hi Kieran,
On 10/02/2022 12:58, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2022-02-10 11:43:16)
>> 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>
>> ---
>> src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++++----------
>> 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 | 27 ++++++++++++++-------------
>> 5 files changed, 42 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index a929085c..95720ca6 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)
>> {
>> }
>> @@ -85,11 +85,14 @@ Agc::Agc()
>> */
>> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>> {
>> - stride_ = context.configuration.grid.stride;
>> + IPASessionConfiguration &configuration = context.configuration;
>> + IPAFrameContext &frameContext = context.frameContext;
>> +
>> + stride_ = configuration.grid.stride;
>>
>> /* \todo use the IPAContext to provide the limits */
>> - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>> - / configInfo.sensorInfo.pixelRate;
>> + configuration.sensor.lineDuration = configInfo.sensorInfo.lineLength * 1.0s
>> + / configInfo.sensorInfo.pixelRate;
>
> Can this be removed now that it's set in IPAIPU3::init()? What limits
> does the \todo talk about? (Or do they apply only to the calculations
> below this?)
>
> In fact I see it gets updated in IPAIPU3::configure() too, so I think
> this is just a leftover.
>
>>
>> minShutterSpeed_ = context.configuration.agc.minShutterSpeed;
>> maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed,
>> @@ -99,8 +102,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 +185,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 +205,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 +252,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 +359,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..b137c66c 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;
>> 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;
>
> Seems a shame that we can't work with Durations more natively with
> Controls to make the control know the correct units, but that's not
> specific to this patch.
>
>> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
>> defExposure);
>>
>> @@ -293,6 +292,10 @@ int IPAIPU3::init(const IPASettings &settings,
>> return -ENODEV;
>> }
>>
>> + /* Clean context */
>> + context_ = {};
>> + 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,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>
>> defVBlank_ = itVBlank->second.def().get<int32_t>();
>>
>> - /* Clean context at configuration */
>> - context_ = {};
>> -
>
> We're we relying on this to reset anything specifically when calling
> configure()? Or is this ok that it doesn't get reset now. (I think if we
> need to reset something at configure() it's probably better to be
> explicit about it anyway).
>
Is is moved in init() now, which, AFAIK, is called before configure ?
>
> If there's no expected regression with moving this clearing of the
> context during configure, then with the extra setting of
> sensor.lineDuration removed in the AGC:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
>> calculateBdsGrid(configInfo.bdsOutputSize);
>>
>> - lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
>> + context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
>>
>> /* Update the camera controls using the new sensor settings. */
>> updateControls(sensorInfo_, ctrls_, ipaControls);
>> @@ -620,6 +620,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 +629,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