[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 13:14:02 CEST 2021
Hi Jean-Michel,
On Fri, Aug 20, 2021 at 07:33:02AM +0200, Jean-Michel Hautbois wrote:
> On 20/08/2021 03:12, Laurent Pinchart wrote:
> > 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.
>
> Yes, I want it to be done on top because this is more than what this
> patch does (it is converting AGC to the interface, and as the grid is
> used in two places it would be a bit too much, according to me :-)).
Sure, no problem. Please record it so it's not forgotten.
> >>
> >> - 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