[libcamera-devel] [PATCH v4 3/4] ipa: ipu3: agc: Introduce lineDuration in IPASessionConfiguration
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Feb 28 15:30:33 CET 2022
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.
> ---
> 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?
> 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.
> + 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.
> /* 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