[PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc
Dan Scally
dan.scally at ideasonboard.com
Tue Apr 23 11:48:07 CEST 2024
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)
>
>> + 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
>
>> 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() ?
It's assigned after the call to the bespoke estimateLuminance() - the overriding function for the
base class is called later from calculateNewEv(). Resetting expMeans_ at the end of ::process() was
a suggestion from Stefan, since it's holding references to stats->params->ae that might not be valid
after process() is over.
>
>> }
>>
>> 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