[PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Apr 24 12:07:23 CEST 2024
Hi Dan
On Wed, Apr 24, 2024 at 11:01:18AM +0100, Dan Scally wrote:
> Hi Jacopo
>
> On 22/04/2024 11:49, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
> > > Now that we have a MeanLuminanceAgc class that centralises our AEGC
> > > algorithm, derive the RkISP1's Agc class from it and plumb in the
> > > necessary framework to enable it to be used. For simplicities sake
> > > this commit switches the algorithm to use the derived class, but
> > > does not remove the bespoke functions at this time.
> > >
> > > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > > ---
> > > Changes in v2:
> > >
> > > - Squashed the patch adding parseStatistics()
> > > - Used the first available constraint and exposure modes rather than
> > > assuming the "Normal" mode was available
> > > - Stopped storing a Histogram in the class members when it's only needed
> > > during ::process()
> > > - Remembered to report the controls out to IPARkISP1::updateControls()
> > >
> > > src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
> > > src/ipa/rkisp1/algorithms/agc.h | 9 +++-
> > > src/ipa/rkisp1/ipa_context.h | 5 ++
> > > src/ipa/rkisp1/rkisp1.cpp | 3 +-
> > > 4 files changed, 92 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 47a6f7b2..0d66fcca 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -64,6 +64,30 @@ Agc::Agc()
> > > supportsRaw_ = true;
> > > }
> > >
> > > +/**
> > > + * \brief Initialise the AGC algorithm from tuning files
> > > + *
> > no empty line
> >
> > > + * \param[in] context The shared IPA context
> > > + * \param[in] tuningData The YamlObject containing Agc tuning data
> > > + *
> > > + * This function calls the base class' tuningData parsers to discover which
> > > + * control values are supported.
> > > + *
> > > + * \return 0 on success or errors from the base class
> > > + */
> > > +int Agc::init(IPAContext &context, const YamlObject &tuningData)
> > > +{
> > > + int ret;
> > > +
> > > + ret = parseTuningData(tuningData);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + context.ctrlMap.merge(controls());
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /**
> > > * \brief Configure the AGC given a configInfo
> > > * \param[in] context The shared IPA context
> > > @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > > context.activeState.agc.autoEnabled = !context.configuration.raw;
> > >
> > > + context.activeState.agc.constraintMode = constraintModes().begin()->first;
> > > + context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > > +
> > > /*
> > > * Define the measurement window for AGC as a centered rectangle
> > > * covering 3/4 of the image width and height.
> > > @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > * frame index
> > > */
> > > frameCount_ = 0;
> > > +
> > > + /* \todo Run this again when FrameDurationLimits is passed in */
> > > + configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> > > + context.configuration.sensor.maxShutterSpeed,
> > > + context.configuration.sensor.minAnalogueGain,
> > > + context.configuration.sensor.maxAnalogueGain);
> > > +
> > I think I also commented this on the base class, but this should just
> > be "setLimits" or something similar (iow do not expose the
> > "ExposureModeHelpers" name to the derived classes)
> Why not, particularly? Deriving from AgcMeanLuminance inherently means
> relying on ExposureModeHelpers, why would we have to hide them?
The base class uses it, the derived classes simply rely on the base
class calculateNewEv() function, right ?
How the base class does that it's opaque to the derived classes,
henche I don't see any benefits in mentioning the helper is a function
name. Isn't 'setLimits()' enough ? What is the benfint of mentioning
the ExposureModeHelper here ?
> > > + resetFrameCount();
> > > +
> > As this HAS to be run at configure time, I would also consider a base
> > class 'configure()' which sets limits and resets the frame counter and
> > have derived classes call it
>
> Sure, we can do that
And that would also hide the helper
> >
> > > return 0;
> > > }
> > >
> > > @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > > double yGain, double iqMeanGain)
> > > {
> > > IPASessionConfiguration &configuration = context.configuration;
> > > - IPAActiveState &activeState = context.activeState;
> > >
> > > /* Get the effective exposure and gain applied on the sensor. */
> > > uint32_t exposure = frameContext.sensor.exposure;
> > > @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > > LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
> > > << shutterTime << " and "
> > > << stepGain;
> > > -
> > > - /* Update the estimated exposure and gain. */
> > > - activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> > > - activeState.agc.automatic.gain = stepGain;
> > > }
> > >
> > > /**
> > > @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > > metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > > }
> > >
> > > +double Agc::estimateLuminance(double gain)
> > > +{
> > > + double ySum = 0.0;
> > > +
> > > + /* Sum the averages, saturated to 255. */
> > > + for (uint8_t expMean : expMeans_)
> > > + ySum += std::min(expMean * gain, 255.0);
> > > +
> > > + /* \todo Weight with the AWB gains */
> > > +
> > > + return ySum / expMeans_.size() / 255;
> > > +}
> > > +
> > > /**
> > > * \brief Process RkISP1 statistics, and run AGC operations
> > > * \param[in] context The shared IPA context
> > > @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > computeExposure(context, frameContext, yGain, iqMeanGain);
> > > frameCount_++;
> > >
> > > + expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > > +
> > > + /*
> > > + * The Agc algorithm needs to know the effective exposure value that was
> > > + * applied to the sensor when the statistics were collected.
> > > + */
> > > + utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > > + * frameContext.sensor.exposure;
> > > + double analogueGain = frameContext.sensor.gain;
> > > + utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > same question, is this a duration ?
> >
> > > +
> > > + utils::Duration shutterTime;
> > > + double aGain, dGain;
> > > + std::tie(shutterTime, aGain, dGain) =
> > > + calculateNewEv(context.activeState.agc.constraintMode,
> > > + context.activeState.agc.exposureMode,
> > > + Histogram(hist), effectiveExposureValue);
> > > +
> > > + LOG(RkISP1Agc, Debug)
> > > + << "Divided up shutter, analogue gain and digital gain are "
> > > + << shutterTime << ", " << aGain << " and " << dGain;
> > > +
> > > + IPAActiveState &activeState = context.activeState;
> > > + /* Update the estimated exposure and gain. */
> > > + activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> > > + activeState.agc.automatic.gain = aGain;
> > > +
> > > fillMetadata(context, frameContext, metadata);
> > > + expMeans_ = {};
> > isn't this reset at the beginning of this function ?
> >
> > Or am I confused ? I see two estimateLuminance() (this is expected)
> > one uses expMeans_ the other doesn't, but expMeans_ is assigned after
> > the call to estimateLuminance() ?
> >
> > > }
> > >
> > > REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > > index fb82a33f..a8080228 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.h
> > > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > > @@ -14,18 +14,22 @@
> > >
> > > #include <libcamera/geometry.h>
> > >
> > > +#include "libipa/agc_mean_luminance.h"
> > > +#include "libipa/histogram.h"
> > > +
> > > #include "algorithm.h"
> > >
> > > namespace libcamera {
> > >
> > > namespace ipa::rkisp1::algorithms {
> > >
> > > -class Agc : public Algorithm
> > > +class Agc : public Algorithm, public AgcMeanLuminance
> > > {
> > > public:
> > > Agc();
> > > ~Agc() = default;
> > >
> > > + int init(IPAContext &context, const YamlObject &tuningData) override;
> > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > > void queueRequest(IPAContext &context,
> > > const uint32_t frame,
> > > @@ -47,10 +51,13 @@ private:
> > > double measureBrightness(Span<const uint32_t> hist) const;
> > > void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > > ControlList &metadata);
> > > + double estimateLuminance(double gain) override;
> > >
> > > uint64_t frameCount_;
> > >
> > > utils::Duration filteredExposure_;
> > > +
> > > + Span<const uint8_t> expMeans_;
> > > };
> > >
> > > } /* namespace ipa::rkisp1::algorithms */
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 10d8f38c..256b75eb 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -12,6 +12,7 @@
> > >
> > > #include <libcamera/base/utils.h>
> > >
> > > +#include <libcamera/controls.h>
> > > #include <libcamera/geometry.h>
> > >
> > > #include <libipa/fc_queue.h>
> > > @@ -67,6 +68,8 @@ struct IPAActiveState {
> > > } automatic;
> > >
> > > bool autoEnabled;
> > > + uint32_t constraintMode;
> > > + uint32_t exposureMode;
> > > } agc;
> > >
> > > struct {
> > > @@ -151,6 +154,8 @@ struct IPAContext {
> > > IPAActiveState activeState;
> > >
> > > FCQueue<IPAFrameContext> frameContexts;
> > > +
> > > + ControlInfoMap::Map ctrlMap;
> > > };
> > >
> > > } /* namespace ipa::rkisp1 */
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 9dc5f53c..d8610095 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
> > > } /* namespace */
> > >
> > > IPARkISP1::IPARkISP1()
> > > - : context_({ {}, {}, {}, { kMaxFrameContexts } })
> > > + : context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
> > > {
> > > }
> > >
> > > @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > > frameDurations[1],
> > > frameDurations[2]);
> > >
> > > + ctrlMap.merge(context_.ctrlMap);
> > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > > }
> > >
> > > --
> > > 2.34.1
> > >
More information about the libcamera-devel
mailing list