[libcamera-devel] [PATCH v3 7/9] ipa: ipu3: convert AGC to the new algorithm interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 19 00:00:58 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Wed, Aug 18, 2021 at 05:54:01PM +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>
> ---
> meson.build | 4 ----
> src/ipa/ipu3/ipu3.cpp | 8 +++-----
> src/ipa/ipu3/ipu3_agc.cpp | 19 +++++++------------
> src/ipa/ipu3/ipu3_agc.h | 9 ++-------
> 4 files changed, 12 insertions(+), 28 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/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2b4a4c8c..423cc957 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -341,7 +341,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;
> }
> @@ -418,8 +418,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_;
>
> @@ -447,8 +446,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>
> 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 c6782ff4..1c1f5fb5 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -51,20 +51,20 @@ 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;
Line wrap again :-)
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> maxExposureTime_ = kMaxExposure * lineDuration_;
> +
> + return 0;
> }
>
> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> @@ -144,8 +144,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 +153,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,12 +175,10 @@ 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;
> }
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index f8290abd..0e922664 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(IPAContext &context, const ipu3_uapi_stats_3a *stats);
> - 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