[libcamera-devel] [PATCH v3 3/3] ipa: ipu3: agc: Introduce lineDuration in IPASessionConfiguration
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Thu Feb 24 15:56:12 CET 2022
Hi Kieran,
On 10/02/2022 14:12, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2022-02-10 12:35:34)
>> 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 ?
>
> Yes, that's fine, but was there any reason it was called in configure so
> that something gets reset in between configures rather than init() when
> it was added?
>
> I.e. is anything after configure relying on the fact that it is reset on
> every configure() call.
>
> (I.e. is there anything relying on if (value == 0) value = x; during the
> algorithm configure() calls or otherwise?)
>
> For example, why is this set in init() now, rather than the
> contstructor?
>
> Is it that we need to make sure the FrameContext is cleared down on any
> reconfiguration? (Which perhaps will move out when it's moved to a
> per-frame-contexts anyway).
It should probably be reset in both init() and configure, to be sure we
reset frameContext at the very least on each configure() call. I will
send v4 with this.
>>
>>>
>>> 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