[libcamera-devel] [PATCH v3 9/9] ipa: ipu3: Move IPU3 agc into algorithms
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 19 00:18:23 CEST 2021
Hi Kieran,
On Wed, Aug 18, 2021 at 11:09:03PM +0100, Kieran Bingham wrote:
> On 18/08/2021 23:05, Laurent Pinchart wrote:
> > On Wed, Aug 18, 2021 at 05:54:03PM +0200, 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>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 21 +++++++++----------
> >> src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 +++++++++---------
> >> src/ipa/ipu3/algorithms/meson.build | 1 +
> >> src/ipa/ipu3/ipu3.cpp | 15 +++++--------
> >> src/ipa/ipu3/meson.build | 1 -
> >> 5 files changed, 26 insertions(+), 32 deletions(-)
> >> rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (93%)
> >> 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 93%
> >> rename from src/ipa/ipu3/ipu3_agc.cpp
> >> rename to src/ipa/ipu3/algorithms/agc.cpp
> >> index 1c1f5fb5..bb119cb4 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,14 +50,13 @@ 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),
> >> - currentExposure_(0s), currentExposureNoDg_(0s)
> >
> > Why can those two fields not be initialized anymore ?
> >
> >> + maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s)
> >> {
> >> }
> >>
> >> -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >> +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >> {
> >> aeGrid_ = context.configuration.grid.bdsGrid;
> >>
> >> @@ -67,7 +66,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,
> >> @@ -108,7 +107,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) {
> >> @@ -142,7 +141,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 ?
> >> @@ -185,7 +184,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;
> >> @@ -194,6 +193,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 87377f4e..acaadd90 100644
> >> --- a/src/ipa/ipu3/algorithms/meson.build
> >> +++ b/src/ipa/ipu3/algorithms/meson.build
> >> @@ -2,6 +2,7 @@
> >>
> >> ipu3_ipa_algorithms = files([
> >> 'algorithm.cpp',
> >> + 'agc.cpp',
> >
> > Wrong alphabetical order.
>
> This one doesn't worry me too much if it's sorted alphabetically...
>
> >> 'awb.cpp',
> >> 'contrast.cpp',
> >> ])
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index f4f49025..d73ec79d 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -30,9 +30,9 @@
> >> #include "libcamera/internal/mapped_framebuffer.h"
> >>
> >> #include "algorithms/algorithm.h"
> >> +#include "algorithms/agc.h"
> >
> > Here too.
>
> But this one does.
>
> If agc.h comes before algorithm.h, then by definition we're relying upon
> the inclusion of algorithm.h from agc.h, so there's no point it being
> here at all.
I've thought the same, but every exception will be flagged repeatedly by
checkstyle.py. If it's not clear why the exception is there, someone
will probably submit a patch. That's why I'd rather sort headers
alphabetically here too, and then forget about it :-) I wouldn't be
against dropping algorithm.h if it bothers you. Or algorithm.h could
move to the parent directory, with only the algorithm implementation
stored in algorithms/. It's really nitpicking :-)
> But I'd personally keep it, and keep it at the beginning of the
> inclusions, as an exception to the alphabetical ordering.
>
> The other algorithms can be sorted alphabetically though
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> >> #include "algorithms/awb.h"
> >> #include "algorithms/contrast.h"
> >> -#include "ipu3_agc.h"
> >> #include "libipa/camera_sensor_helper.h"
> >>
> >> static constexpr uint32_t kMaxCellWidthPerSet = 160;
> >> @@ -136,8 +136,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_;
> >>
> >> @@ -218,6 +216,7 @@ int IPAIPU3::init(const IPASettings &settings,
> >> *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> >>
> >> /* Construct our Algorithms */
> >> + algorithms_.emplace_back(new algorithms::Agc());
> >> algorithms_.emplace_back(new algorithms::Awb());
> >> algorithms_.emplace_back(new algorithms::Contrast());
> >>
> >> @@ -337,10 +336,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> >> return ret;
> >> }
> >>
> >> -
> >> - agcAlgo_ = std::make_unique<IPU3Agc>();
> >> - agcAlgo_->configure(context_, configInfo);
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -436,9 +431,6 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >> for (auto const &algo : algorithms_)
> >> algo->process(context_, stats);
> >>
> >> - agcAlgo_->process(context_, stats);
> >> - exposure_ = context_.frameContext.agc.exposure;
> >> - gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
> >> setControls(frame);
> >>
> >> /* \todo Use VBlank value calculated from each frame exposure. */
> >> @@ -458,6 +450,9 @@ void IPAIPU3::setControls(unsigned int frame)
> >> IPU3Action op;
> >> op.op = ActionSetSensorControls;
> >>
> >> + exposure_ = context_.frameContext.agc.exposure;
> >> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
> >> +
> >> 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