[PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 6 03:56:30 CEST 2024
On Wed, Mar 27, 2024 at 05:11:04PM +0100, Stefan Klug wrote:
> Hi Daniel,
>
> thanks for the patch.
>
> On Fri, Mar 22, 2024 at 01:14:50PM +0000, 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>
> > ---
> > src/ipa/rkisp1/algorithms/agc.cpp | 91 +++++++++++++++++++++++++++++--
> > src/ipa/rkisp1/algorithms/agc.h | 5 +-
> > src/ipa/rkisp1/ipa_context.h | 5 ++
> > src/ipa/rkisp1/rkisp1.cpp | 2 +-
> > 4 files changed, 96 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index d380194d..3389c471 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -64,6 +64,36 @@ Agc::Agc()
> > supportsRaw_ = true;
> > }
> >
> > +/**
> > + * \brief Initialise the AGC algorithm from tuning files
> > + *
> > + * \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;
> > +
> > + parseRelativeLuminanceTarget(tuningData);
> > +
> > + ret = parseConstraintModes(tuningData);
> > + if (ret)
> > + return ret;
> > +
> > + ret = parseExposureModes(tuningData);
> > + if (ret)
> > + return ret;
> > +
>
> Same comment as for ipu3
>
> > + context.ctrlMap.merge(controls());
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * \brief Configure the AGC given a configInfo
> > * \param[in] context The shared IPA context
> > @@ -81,6 +111,13 @@ 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;
> >
> > + /*
> > + * \todo We should use the first available mode rather than assume that
> > + * the "Normal" modes are present in tuning data.
> > + */
> > + context.activeState.agc.constraintMode = controls::ConstraintNormal;
> > + context.activeState.agc.exposureMode = controls::ExposureNormal;
> > +
> > /*
> > * Define the measurement window for AGC as a centered rectangle
> > * covering 3/4 of the image width and height.
> > @@ -95,6 +132,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > * frame index
> > */
> > frameCount_ = 0;
> > +
> > + for (auto &[id, helper] : exposureModeHelpers()) {
> > + /* \todo Run this again when FrameDurationLimits is passed in */
> > + helper->configure(context.configuration.sensor.minShutterSpeed,
> > + context.configuration.sensor.maxShutterSpeed,
>
> Not part of this patch, but why are these called minShutterSpeed and not
> minExposureTime?
Let's bite the bullet and define a standard vocabulary.
> > + context.configuration.sensor.minAnalogueGain,
> > + context.configuration.sensor.maxAnalogueGain);
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -234,7 +280,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 +345,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;
> > }
> >
> > /**
> > @@ -383,6 +424,19 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,
> > context.hw->numHistogramBins));
> > }
> >
> > +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 */
How so ?
> > +
> > + return ySum / expMeans_.size() / 255;
> > +}
> > +
> > /**
> > * \brief Process RkISP1 statistics, and run AGC operations
> > * \param[in] context The shared IPA context
> > @@ -448,6 +502,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > computeExposure(context, frameContext, yGain, iqMeanGain);
> > frameCount_++;
> >
> > + parseStatistics(stats, context);
>
> Regarding my previous mail. Here expMean_ would be assigned...
>
> > +
> > + /*
> > + * 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;
> > +
> > + utils::Duration shutterTime;
> > + double aGain, dGain;
> > + std::tie(shutterTime, aGain, dGain) =
> > + calculateNewEv(context.activeState.agc.constraintMode,
> > + context.activeState.agc.exposureMode, 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);
>
> ...and here it would be reset.
>
> > }
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > index b0de4898..1271741e 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -14,6 +14,7 @@
> >
> > #include <libcamera/geometry.h>
> >
> > +#include "libipa/agc.h"
> > #include "libipa/histogram.h"
> >
> > #include "algorithm.h"
> > @@ -22,12 +23,13 @@ namespace libcamera {
> >
> > namespace ipa::rkisp1::algorithms {
> >
> > -class Agc : public Algorithm
> > +class Agc : public Algorithm, public MeanLuminanceAgc
> > {
> > 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,
> > @@ -51,6 +53,7 @@ private:
> > ControlList &metadata);
> > void parseStatistics(const rkisp1_stat_buffer *stats,
> > IPAContext &context);
> > + double estimateLuminance(double gain) override;
> >
> > uint64_t frameCount_;
> >
> > 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..5f74762f 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 }, {} })
> > {
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list