[libcamera-devel] [PATCH v4 7/9] ipa: ipu3: convert AGC to the new algorithm interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 20 03:12:16 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Thu, Aug 19, 2021 at 04:57:57PM +0200, Jean-Michel Hautbois wrote:
> In preparation for using the AGC through the new algorithm interfaces,
> convert the existing code to use the new function types.
>
> Now that the process call is rewritten, re-enable the compiler flag to
> warn when a function declaration hides virtual functions from a base class
> (-Woverloaded-virtual).
>
> We never use converged_ so remove its declaration.
> The controls may not need to be updated at each call, but it should be
> decided on the context side and not by a specific call by using a lock
> status in the Agc structure for instance.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> meson.build | 4 ----
> src/ipa/ipu3/ipa_context.h | 5 +++++
> src/ipa/ipu3/ipu3.cpp | 33 +++++++++++++++++++++++++--------
> src/ipa/ipu3/ipu3_agc.cpp | 24 +++++++++++-------------
> src/ipa/ipu3/ipu3_agc.h | 9 ++-------
> 5 files changed, 43 insertions(+), 32 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 4a10e2b6..a49c484f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -108,10 +108,6 @@ if cc.has_argument('-Wno-c99-designator')
> ]
> endif
>
> -# Do not warn when a function declaration hides virtual functions from
> -# a base class
> -cpp_arguments += '-Wno-overloaded-virtual'
> -
> c_arguments += common_arguments
> cpp_arguments += common_arguments
>
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 24dd8bf7..9d9444dc 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -24,6 +24,11 @@ struct IPASessionConfiguration {
> };
>
> struct IPAFrameContext {
> + struct {
> + uint32_t exposure;
> + double gain;
> + } agc;
> +
> struct {
> struct {
> double red;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index cede897e..33e21993 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -95,6 +95,22 @@ static constexpr uint32_t kMaxCellHeightPerSet = 56;
> * \brief BDS output size configured by the pipeline handler
> */
>
> +/**
> + * \struct IPAFrameContext::agc
> + * \brief Context for the Automatic Gain Control algorithm
> + *
> + * The exposure and gain determined are expected to be applied to the sensor
> + * at the earliest opportunity.
> + *
> + * \var IPAFrameContext::agc::exposure
> + * \brief Exposure time expressed as a number of lines
> + *
> + * \var IPAFrameContext::agc::gain
> + * \brief Analogue gain multiplier
> + *
> + * The gain should be adapted to the sensor specific gain code before applying.
> + */
> +
> /**
> * \struct IPAFrameContext::awb
> * \brief Context for the Automatic White Balance algorithm
> @@ -375,7 +391,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>
> awbAlgo_ = std::make_unique<IPU3Awb>();
> agcAlgo_ = std::make_unique<IPU3Agc>();
> - agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
> + agcAlgo_->configure(context_, configInfo);
>
> return 0;
> }
> @@ -452,8 +468,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> for (auto const &algo : algorithms_)
> algo->prepare(context_, params_);
>
> - if (agcAlgo_->updateControls())
> - awbAlgo_->prepare(context_, params_);
> + awbAlgo_->prepare(context_, params_);
>
> *params = params_;
>
> @@ -472,14 +487,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> for (auto const &algo : algorithms_)
> algo->process(context_, stats);
>
> - double gain = camHelper_->gain(gain_);
> - agcAlgo_->process(stats, exposure_, gain);
> - gain_ = camHelper_->gainCode(gain);
> + /* \todo These fields should not be written by the IPAIPU3 layer */
> + context_.frameContext.agc.gain = camHelper_->gain(gain_);
> + context_.frameContext.agc.exposure = exposure_;
> + agcAlgo_->process(context_, stats);
> + exposure_ = context_.frameContext.agc.exposure;
> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
>
> awbAlgo_->process(context_, stats);
>
> - if (agcAlgo_->updateControls())
> - setControls(frame);
> + setControls(frame);
>
> /* \todo Use VBlank value calculated from each frame exposure. */
> int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> index 20c2d3b4..30888e10 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -51,20 +51,21 @@ static constexpr double kEvGainTarget = 0.5;
> static constexpr uint8_t kCellSize = 8;
>
> IPU3Agc::IPU3Agc()
> - : frameCount_(0), lastFrame_(0), converged_(false),
> - updateControls_(false), iqMean_(0.0),
> - lineDuration_(0s), maxExposureTime_(0s),
> - prevExposure_(0s), prevExposureNoDg_(0s),
> + : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> + maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
> currentExposure_(0s), currentExposureNoDg_(0s)
> {
> }
>
> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)
> +int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> {
> - aeGrid_ = bdsGrid;
> + aeGrid_ = context.configuration.grid.bdsGrid;
Do we need to keep a copy of the grid, can't we git it from the context
when needed ? It doesn't have to be addressed in this patch, could be
done on top if easier.
>
> - lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
> + lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
> + / configInfo.sensorInfo.pixelRate;
> maxExposureTime_ = kMaxExposure * lineDuration_;
> +
> + return 0;
> }
>
> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> @@ -144,8 +145,6 @@ void IPU3Agc::filterExposure()
>
> void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> {
> - updateControls_ = false;
> -
> /* Algorithm initialization should wait for first valid frames */
> /* \todo - have a number of frames given by DelayedControls ?
> * - implement a function for IIR */
> @@ -155,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> /* Are we correctly exposed ? */
> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> - converged_ = true;
> } else {
> double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>
> @@ -178,20 +176,20 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
> newExposure = currentExposure_ / exposure;
> gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> - updateControls_ = true;
> } else if (currentShutter >= maxExposureTime_) {
> gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
> newExposure = currentExposure_ / gain;
> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
> - updateControls_ = true;
> }
> LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
> }
> lastFrame_ = frameCount_;
> }
>
> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)
> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *&stats)
> {
> + uint32_t &exposure = context.frameContext.agc.exposure;
> + double &gain = context.frameContext.agc.gain;
> processBrightness(stats);
> lockExposureGain(exposure, gain);
> frameCount_++;
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index 2d86127d..0bd70021 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -29,10 +29,8 @@ public:
> IPU3Agc();
> ~IPU3Agc() = default;
>
> - void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
> - void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
> - bool converged() { return converged_; }
> - bool updateControls() { return updateControls_; }
> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> + void process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) override;
>
> private:
> void processBrightness(const ipu3_uapi_stats_3a *stats);
> @@ -44,9 +42,6 @@ private:
> uint64_t frameCount_;
> uint64_t lastFrame_;
>
> - bool converged_;
> - bool updateControls_;
> -
> double iqMean_;
>
> Duration lineDuration_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list