[libcamera-devel] [PATCH v2 10/10] ipa: ipu3: Move IPU3 agc into algorithms
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 16 02:21:44 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Fri, Aug 13, 2021 at 11:27:01AM +0100, Kieran Bingham wrote:
> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> > Now that the interface is properly used by the AGC class, move it into
> > ipa::ipu3::algorithms and let the loops do the calls.
> > As we need to exchange the exposure_ and gain_ by passing them through the
> > FrameContext, use the calculated values in setControls() function to
> > ease the reading.
> >
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > ---
> > .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 18 +++++++--------
> > src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 ++++++++---------
> > src/ipa/ipu3/algorithms/meson.build | 1 +
> > src/ipa/ipu3/ipu3.cpp | 22 +++++++------------
> > src/ipa/ipu3/meson.build | 1 -
> > 5 files changed, 28 insertions(+), 34 deletions(-)
> > rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (94%)
> > rename src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} (73%)
> >
> > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > similarity index 94%
> > rename from src/ipa/ipu3/ipu3_agc.cpp
> > rename to src/ipa/ipu3/algorithms/agc.cpp
> > index 0d0584a8..ee3d3bc2 100644
> > --- a/src/ipa/ipu3/ipu3_agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -5,7 +5,7 @@
> > * ipu3_agc.cpp - AGC/AEC control algorithm
> > */
> >
> > -#include "ipu3_agc.h"
> > +#include "agc.h"
> >
> > #include <algorithm>
> > #include <chrono>
> > @@ -21,7 +21,7 @@ namespace libcamera {
> >
> > using namespace std::literals::chrono_literals;
> >
> > -namespace ipa::ipu3 {
> > +namespace ipa::ipu3::algorithms {
> >
> > LOG_DEFINE_CATEGORY(IPU3Agc)
> >
> > @@ -50,7 +50,7 @@ static constexpr double kEvGainTarget = 0.5;
> > /* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
> > static constexpr uint8_t kCellSize = 8;
> >
> > -IPU3Agc::IPU3Agc()
> > +Agc::Agc()
> > : frameCount_(0), lastFrame_(0),
> > iqMean_(0.0), lineDuration_(0s), maxExposureTime_(0s),
> > prevExposure_(0s), prevExposureNoDg_(0s),
> > @@ -58,7 +58,7 @@ IPU3Agc::IPU3Agc()
> > {
> > }
> >
> > -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> > +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> > {
> > aeGrid_ = context.configuration.grid.bdsGrid;
> >
> > @@ -68,7 +68,7 @@ int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> > return 0;
> > }
> >
> > -void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> > +void Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> > {
> > const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
> > Rectangle aeRegion = { statsAeGrid.x_start,
> > @@ -109,7 +109,7 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> > iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
> > }
> >
> > -void IPU3Agc::filterExposure()
> > +void Agc::filterExposure()
> > {
> > double speed = 0.2;
> > if (prevExposure_ == 0s) {
> > @@ -143,7 +143,7 @@ void IPU3Agc::filterExposure()
> > LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevExposure_;
> > }
> >
> > -void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> > +void Agc::lockExposureGain(uint32_t &exposure, double &gain)
> > {
> > /* Algorithm initialization should wait for first valid frames */
> > /* \todo - have a number of frames given by DelayedControls ?
> > @@ -186,7 +186,7 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> > lastFrame_ = frameCount_;
> > }
> >
> > -void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> > +void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> > {
> > uint32_t &exposure = context.frameContext.agc.exposure;
> > double &gain = context.frameContext.agc.gain;
> > @@ -195,6 +195,6 @@ void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> > frameCount_++;
> > }
> >
> > -} /* namespace ipa::ipu3 */
> > +} /* namespace ipa::ipu3::algorithms */
> >
> > } /* namespace libcamera */
> > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/algorithms/agc.h
> > similarity index 73%
> > rename from src/ipa/ipu3/ipu3_agc.h
> > rename to src/ipa/ipu3/algorithms/agc.h
> > index 0e922664..1739d2fd 100644
> > --- a/src/ipa/ipu3/ipu3_agc.h
> > +++ b/src/ipa/ipu3/algorithms/agc.h
> > @@ -2,10 +2,10 @@
> > /*
> > * Copyright (C) 2021, Ideas On Board
> > *
> > - * ipu3_agc.h - IPU3 AGC/AEC control algorithm
> > + * agc.h - IPU3 AGC/AEC control algorithm
> > */
> > -#ifndef __LIBCAMERA_IPU3_AGC_H__
> > -#define __LIBCAMERA_IPU3_AGC_H__
> > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> > +#define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> >
> > #include <linux/intel-ipu3.h>
> >
> > @@ -13,21 +13,21 @@
> >
> > #include <libcamera/geometry.h>
> >
> > -#include "algorithms/algorithm.h"
> > +#include "algorithm.h"
> >
> > namespace libcamera {
> >
> > struct IPACameraSensorInfo;
> >
> > -namespace ipa::ipu3 {
> > +namespace ipa::ipu3::algorithms {
> >
> > using utils::Duration;
> >
> > -class IPU3Agc : public Algorithm
> > +class Agc : public Algorithm
> > {
> > public:
> > - IPU3Agc();
> > - ~IPU3Agc() = default;
> > + Agc();
> > + ~Agc() = default;
> >
> > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> > @@ -53,8 +53,8 @@ private:
> > Duration currentExposureNoDg_;
> > };
> >
> > -} /* namespace ipa::ipu3 */
> > +} /* namespace ipa::ipu3::algorithms */
> >
> > } /* namespace libcamera */
> >
> > -#endif /* __LIBCAMERA_IPU3_AGC_H__ */
> > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__ */
> > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> > index 9ef2dd41..e75d92e0 100644
> > --- a/src/ipa/ipu3/algorithms/meson.build
> > +++ b/src/ipa/ipu3/algorithms/meson.build
> > @@ -1,6 +1,7 @@
> > # SPDX-License-Identifier: CC0-1.0
> >
> > ipu3_ipa_algorithms = files([
> > + 'agc.cpp',
> > 'awb.cpp',
> > 'contrast.cpp',
> > 'grid.cpp',
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index bea2a372..a99fbcac 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -30,10 +30,10 @@
> > #include "libcamera/internal/mapped_framebuffer.h"
> >
> > #include "algorithms/algorithm.h"
> > +#include "algorithms/agc.h"
> > #include "algorithms/awb.h"
> > #include "algorithms/contrast.h"
> > #include "algorithms/grid.h"
> > -#include "ipu3_agc.h"
> > #include "libipa/camera_sensor_helper.h"
> >
> > namespace libcamera {
> > @@ -84,8 +84,6 @@ private:
> > uint32_t minGain_;
> > uint32_t maxGain_;
> >
> > - /* Interface to the AEC/AGC algorithm */
> > - std::unique_ptr<IPU3Agc> agcAlgo_;
> > /* Interface to the Camera Helper */
> > std::unique_ptr<CameraSensorHelper> camHelper_;
> >
> > @@ -167,6 +165,7 @@ int IPAIPU3::init(const IPASettings &settings,
> > /* Construct our Algorithms */
> > algorithms_.emplace_back(new algorithms::Grid());
> > /* Grid needs to be prepared before AWB */
> > + algorithms_.emplace_back(new algorithms::Agc());
> > algorithms_.emplace_back(new algorithms::Awb());
> > algorithms_.emplace_back(new algorithms::Contrast());
>
> Ultimately, I would expect this 'table' of algorithms to look like the
> diagram in (or at least match the dependencies in):
>
> https://www.kernel.org/doc/html/latest/admin-guide/media/ipu3.html?highlight=ipu3#overview-of-ipu3-pipeline
>
> I wonder if it's worth referencing that link here.
>
> Probably when we add the first algorithms_.emplace_back().
>
> >
> > @@ -229,9 +228,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> > return ret;
> > }
> >
> > - agcAlgo_ = std::make_unique<IPU3Agc>();
> > - agcAlgo_->configure(context_, configInfo);
> > -
> > return 0;
> > }
> >
> > @@ -320,17 +316,12 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> > [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> > {
> > ControlList ctrls(controls::controls);
>
> At least a new line required here.
>
> And as commented before, this needs some explicit
> documentation/reasoning/comments as to why the IPU3 layer is setting agc
> specific parameters outside of the AGC module.
I'm also interested in understanding the rationale for this, and the
next steps.
> Of course, if those are added in the earlier patch, they'll then move
> here with the code move.
>
> Are these lines even actually required?
> Are you expecting those values to have changed such that you need to
> reset them?
>
>
> > + context_.frameContext.agc.gain = camHelper_->gain(gain_);
> > + context_.frameContext.agc.exposure = exposure_;
> > +
If I understand correctly, this is related to providing historical data
(as in the gain and exposure for the previous frame) to the AGC
implementation. We'll need a more generic mechanism to access the
history, and this can come in subsequent patches. Care should be taken,
when designing that, to make it easy to use for algorithms. If an
algorithm needs to access values it has computed for the last frame,
that's fairly simple, it can always look at the last frame. Things get
more complicated when an algorithm needs to access the latest values
computed by another algorithm, as in that case where the latest value is
stored will depend on the order in which algorithms are run. The
implementation of the algorithms should as much as possible not depend
on any particular order (any departure from this rule needs to be
discussed), and they should also not need to manually look at values for
the current frame and for the last frame to find the most recent one. We
need either helper functions, or a data storage design that ensures that
the latest value is always available from the same place.
> > for (auto const &algo : algorithms_)
> > algo->process(context_, stats);
> >
> > - double gain = camHelper_->gain(gain_);
> > - context_.frameContext.agc.exposure = exposure_;
> > - context_.frameContext.agc.gain = gain;
> > - agcAlgo_->process(context_, stats);
> > - exposure_ = context_.frameContext.agc.exposure;
> > - gain = context_.frameContext.agc.gain;
> > - gain_ = camHelper_->gainCode(gain);
> > -
> > setControls(frame);
> >
> > /* \todo Use VBlank value calculated from each frame exposure. */
> > @@ -350,6 +341,9 @@ void IPAIPU3::setControls(unsigned int frame)
> > IPU3Action op;
> > op.op = ActionSetSensorControls;
> >
> > + exposure_ = context_.frameContext.agc.exposure;
> > + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
>
> Perhaps these should be local variables, and the gain_ and exposure_
> variables in the IPU3 class can be removed?
>
>
> There's plenty more we can clean up on the IPU3 now that the interfaces
> are more modular, but I think this is all going in the right direction.
>
> With all relevant comments addressed,
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > +
> > ControlList ctrls(ctrls_);
> > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> > diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> > index d1126947..39320980 100644
> > --- a/src/ipa/ipu3/meson.build
> > +++ b/src/ipa/ipu3/meson.build
> > @@ -6,7 +6,6 @@ ipa_name = 'ipa_ipu3'
> >
> > ipu3_ipa_sources = files([
> > 'ipu3.cpp',
> > - 'ipu3_agc.cpp',
> > ])
> >
> > ipu3_ipa_sources += ipu3_ipa_algorithms
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list