[{"id":29096,"web_url":"https://patchwork.libcamera.org/comment/29096/","msgid":"<20240327161104.n2ryp73fqdx2f2vk@jasper>","date":"2024-03-27T16:11:04","subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Daniel,\n\nthanks for the patch.\n\nOn Fri, Mar 22, 2024 at 01:14:50PM +0000, Daniel Scally wrote:\n> Now that we have a MeanLuminanceAgc class that centralises our AEGC\n> algorithm, derive the RkISP1's Agc class from it and plumb in the\n> necessary framework to enable it to be used. For simplicities sake\n> this commit switches the algorithm to use the derived class, but\n> does not remove the bespoke functions at this time.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 91 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/agc.h | 5 +-\n> src/ipa/rkisp1/ipa_context.h | 5 ++\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> 4 files changed, 96 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index d380194d..3389c471 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -64,6 +64,36 @@ Agc::Agc()\n> \tsupportsRaw_ = true;\n> }\n> \n> +/**\n> + * \\brief Initialise the AGC algorithm from tuning files\n> + *\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] tuningData The YamlObject containing Agc tuning data\n> + *\n> + * This function calls the base class' tuningData parsers to discover which\n> + * control values are supported.\n> + *\n> + * \\return 0 on success or errors from the base class\n> + */\n> +int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> +{\n> +\tint ret;\n> +\n> +\tparseRelativeLuminanceTarget(tuningData);\n> +\n> +\tret = parseConstraintModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = parseExposureModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n\nSame comment as for ipu3\n\n> +\tcontext.ctrlMap.merge(controls());\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Configure the AGC given a configInfo\n> * \\param[in] context The shared IPA context\n> @@ -81,6 +111,13 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> \tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n> \n> +\t/*\n> +\t* \\todo We should use the first available mode rather than assume that\n> +\t* the \"Normal\" modes are present in tuning data.\n> +\t*/\n> +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n> +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n> +\n> \t/*\n> \t * Define the measurement window for AGC as a centered rectangle\n> \t * covering 3/4 of the image width and height.\n> @@ -95,6 +132,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t * frame index\n> \t */\n> \tframeCount_ = 0;\n> +\n> +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n> +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> +\t\thelper->configure(context.configuration.sensor.minShutterSpeed,\n> +\t\t\t\t context.configuration.sensor.maxShutterSpeed,\n\nNot part of this patch, but why are these called minShutterSpeed and not\nminExposureTime?\n\n> +\t\t\t\t context.configuration.sensor.minAnalogueGain,\n> +\t\t\t\t context.configuration.sensor.maxAnalogueGain);\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -234,7 +280,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t double yGain, double iqMeanGain)\n> {\n> \tIPASessionConfiguration &configuration = context.configuration;\n> -\tIPAActiveState &activeState = context.activeState;\n> \n> \t/* Get the effective exposure and gain applied on the sensor. */\n> \tuint32_t exposure = frameContext.sensor.exposure;\n> @@ -300,10 +345,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \tLOG(RkISP1Agc, Debug) << \"Divided up shutter and gain are \"\n> \t\t\t << shutterTime << \" and \"\n> \t\t\t << stepGain;\n> -\n> -\t/* Update the estimated exposure and gain. */\n> -\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> -\tactiveState.agc.automatic.gain = stepGain;\n> }\n> \n> /**\n> @@ -383,6 +424,19 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,\n> \t\t\t\t\t context.hw->numHistogramBins));\n> }\n> \n> +double Agc::estimateLuminance(double gain)\n> +{\n> +\tdouble ySum = 0.0;\n> +\n> +\t/* Sum the averages, saturated to 255. */\n> +\tfor (uint8_t expMean : expMeans_)\n> +\t\tySum += std::min(expMean * gain, 255.0);\n> +\n> +\t/* \\todo Weight with the AWB gains */\n> +\n> +\treturn ySum / expMeans_.size() / 255;\n> +}\n> +\n> /**\n> * \\brief Process RkISP1 statistics, and run AGC operations\n> * \\param[in] context The shared IPA context\n> @@ -448,6 +502,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> \n> +\tparseStatistics(stats, context);\n\nRegarding my previous mail. Here expMean_ would be assigned...\n\n> +\n> +\t/*\n> +\t * The Agc algorithm needs to know the effective exposure value that was\n> +\t * applied to the sensor when the statistics were collected.\n> +\t */\n> +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> +\t\t\t\t * frameContext.sensor.exposure;\n> +\tdouble analogueGain = frameContext.sensor.gain;\n> +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> +\n> +\tutils::Duration shutterTime;\n> +\tdouble aGain, dGain;\n> +\tstd::tie(shutterTime, aGain, dGain) =\n> +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n> +\t\t\t context.activeState.agc.exposureMode, hist_,\n> +\t\t\t effectiveExposureValue);\n> +\n> +\tLOG(RkISP1Agc, Debug)\n> +\t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> +\t\t<< shutterTime << \", \" << aGain << \" and \" << dGain;\n> +\n> +\tIPAActiveState &activeState = context.activeState;\n> +\t/* Update the estimated exposure and gain. */\n> +\tactiveState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;\n> +\tactiveState.agc.automatic.gain = aGain;\n> +\n> \tfillMetadata(context, frameContext, metadata);\n\n...and here it would be reset.\n\nCheers,\nStefan\n\n> }\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index b0de4898..1271741e 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -14,6 +14,7 @@\n> \n> #include \n> \n> +#include \"libipa/agc.h\"\n> #include \"libipa/histogram.h\"\n> \n> #include \"algorithm.h\"\n> @@ -22,12 +23,13 @@ namespace libcamera {\n> \n> namespace ipa::rkisp1::algorithms {\n> \n> -class Agc : public Algorithm\n> +class Agc : public Algorithm, public MeanLuminanceAgc\n> {\n> public:\n> \tAgc();\n> \t~Agc() = default;\n> \n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> \tvoid queueRequest(IPAContext &context,\n> \t\t\t const uint32_t frame,\n> @@ -51,6 +53,7 @@ private:\n> \t\t\t ControlList &metadata);\n> \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n> \t\t\t IPAContext &context);\n> +\tdouble estimateLuminance(double gain) override;\n> \n> \tuint64_t frameCount_;\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 10d8f38c..256b75eb 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -12,6 +12,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -67,6 +68,8 @@ struct IPAActiveState {\n> \t\t} automatic;\n> \n> \t\tbool autoEnabled;\n> +\t\tuint32_t constraintMode;\n> +\t\tuint32_t exposureMode;\n> \t} agc;\n> \n> \tstruct {\n> @@ -151,6 +154,8 @@ struct IPAContext {\n> \tIPAActiveState activeState;\n> \n> \tFCQueue frameContexts;\n> +\n> +\tControlInfoMap::Map ctrlMap;\n> };\n> \n> } /* namespace ipa::rkisp1 */\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 9dc5f53c..5f74762f 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{\n> } /* namespace */\n> \n> IPARkISP1::IPARkISP1()\n> -\t: context_({ {}, {}, {}, { kMaxFrameContexts } })\n> +\t: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })\n> {\n> }\n> \n> -- \n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4E0E0C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 16:11:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 331F463340;\n\tWed, 27 Mar 2024 17:11:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4F746308D\n\tfor ;\n\tWed, 27 Mar 2024 17:11:06 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:da09:7e54:ae7f:d731])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5F4BF13AC;\n\tWed, 27 Mar 2024 17:10:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Tt3zO+MI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711555834;\n\tbh=nis3zNbKC+2944eDOYFSzXuV2rCrEPlHjyWG1S1R2yM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Tt3zO+MILvHnp5n3o5sSVfeUgOaVD5e9Ia/d5qyKdpDgVMVGhRjxRoDvpFdMUl6RD\n\txZSmrJscSY3D6ITWiVeRpm7GQkKg+MQsGBp464O/gocdEVSBx0oqQN7Bkwj+fu1Rch\n\tmtZfatFHMiSVFsXaYscjhclepKymk1s9xO7/yV0o=","Date":"Wed, 27 Mar 2024 17:11:04 +0100","From":"Stefan Klug ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"<20240327161104.n2ryp73fqdx2f2vk@jasper>","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-10-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240322131451.3092931-10-dan.scally@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29172,"web_url":"https://patchwork.libcamera.org/comment/29172/","msgid":"<20240406015630.GP12507@pendragon.ideasonboard.com>","date":"2024-04-06T01:56:30","subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Mar 27, 2024 at 05:11:04PM +0100, Stefan Klug wrote:\n> Hi Daniel,\n> \n> thanks for the patch.\n> \n> On Fri, Mar 22, 2024 at 01:14:50PM +0000, Daniel Scally wrote:\n> > Now that we have a MeanLuminanceAgc class that centralises our AEGC\n> > algorithm, derive the RkISP1's Agc class from it and plumb in the\n> > necessary framework to enable it to be used. For simplicities sake\n> > this commit switches the algorithm to use the derived class, but\n> > does not remove the bespoke functions at this time.\n> > \n> > Signed-off-by: Daniel Scally \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 91 +++++++++++++++++++++++++++++--\n> > src/ipa/rkisp1/algorithms/agc.h | 5 +-\n> > src/ipa/rkisp1/ipa_context.h | 5 ++\n> > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > 4 files changed, 96 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index d380194d..3389c471 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -64,6 +64,36 @@ Agc::Agc()\n> > \tsupportsRaw_ = true;\n> > }\n> > \n> > +/**\n> > + * \\brief Initialise the AGC algorithm from tuning files\n> > + *\n> > + * \\param[in] context The shared IPA context\n> > + * \\param[in] tuningData The YamlObject containing Agc tuning data\n> > + *\n> > + * This function calls the base class' tuningData parsers to discover which\n> > + * control values are supported.\n> > + *\n> > + * \\return 0 on success or errors from the base class\n> > + */\n> > +int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tparseRelativeLuminanceTarget(tuningData);\n> > +\n> > +\tret = parseConstraintModes(tuningData);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = parseExposureModes(tuningData);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> \n> Same comment as for ipu3\n> \n> > +\tcontext.ctrlMap.merge(controls());\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > /**\n> > * \\brief Configure the AGC given a configInfo\n> > * \\param[in] context The shared IPA context\n> > @@ -81,6 +111,13 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > \tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n> > \n> > +\t/*\n> > +\t* \\todo We should use the first available mode rather than assume that\n> > +\t* the \"Normal\" modes are present in tuning data.\n> > +\t*/\n> > +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n> > +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n> > +\n> > \t/*\n> > \t * Define the measurement window for AGC as a centered rectangle\n> > \t * covering 3/4 of the image width and height.\n> > @@ -95,6 +132,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \t * frame index\n> > \t */\n> > \tframeCount_ = 0;\n> > +\n> > +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n> > +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> > +\t\thelper->configure(context.configuration.sensor.minShutterSpeed,\n> > +\t\t\t\t context.configuration.sensor.maxShutterSpeed,\n> \n> Not part of this patch, but why are these called minShutterSpeed and not\n> minExposureTime?\n\nLet's bite the bullet and define a standard vocabulary.\n\n> > +\t\t\t\t context.configuration.sensor.minAnalogueGain,\n> > +\t\t\t\t context.configuration.sensor.maxAnalogueGain);\n> > +\t}\n> > +\n> > \treturn 0;\n> > }\n> > \n> > @@ -234,7 +280,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> > \t\t\t double yGain, double iqMeanGain)\n> > {\n> > \tIPASessionConfiguration &configuration = context.configuration;\n> > -\tIPAActiveState &activeState = context.activeState;\n> > \n> > \t/* Get the effective exposure and gain applied on the sensor. */\n> > \tuint32_t exposure = frameContext.sensor.exposure;\n> > @@ -300,10 +345,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> > \tLOG(RkISP1Agc, Debug) << \"Divided up shutter and gain are \"\n> > \t\t\t << shutterTime << \" and \"\n> > \t\t\t << stepGain;\n> > -\n> > -\t/* Update the estimated exposure and gain. */\n> > -\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> > -\tactiveState.agc.automatic.gain = stepGain;\n> > }\n> > \n> > /**\n> > @@ -383,6 +424,19 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,\n> > \t\t\t\t\t context.hw->numHistogramBins));\n> > }\n> > \n> > +double Agc::estimateLuminance(double gain)\n> > +{\n> > +\tdouble ySum = 0.0;\n> > +\n> > +\t/* Sum the averages, saturated to 255. */\n> > +\tfor (uint8_t expMean : expMeans_)\n> > +\t\tySum += std::min(expMean * gain, 255.0);\n> > +\n> > +\t/* \\todo Weight with the AWB gains */\n\nHow so ?\n\n> > +\n> > +\treturn ySum / expMeans_.size() / 255;\n> > +}\n> > +\n> > /**\n> > * \\brief Process RkISP1 statistics, and run AGC operations\n> > * \\param[in] context The shared IPA context\n> > @@ -448,6 +502,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> > \tframeCount_++;\n> > \n> > +\tparseStatistics(stats, context);\n> \n> Regarding my previous mail. Here expMean_ would be assigned...\n> \n> > +\n> > +\t/*\n> > +\t * The Agc algorithm needs to know the effective exposure value that was\n> > +\t * applied to the sensor when the statistics were collected.\n> > +\t */\n> > +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> > +\t\t\t\t * frameContext.sensor.exposure;\n> > +\tdouble analogueGain = frameContext.sensor.gain;\n> > +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> > +\n> > +\tutils::Duration shutterTime;\n> > +\tdouble aGain, dGain;\n> > +\tstd::tie(shutterTime, aGain, dGain) =\n> > +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n> > +\t\t\t context.activeState.agc.exposureMode, hist_,\n> > +\t\t\t effectiveExposureValue);\n> > +\n> > +\tLOG(RkISP1Agc, Debug)\n> > +\t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> > +\t\t<< shutterTime << \", \" << aGain << \" and \" << dGain;\n> > +\n> > +\tIPAActiveState &activeState = context.activeState;\n> > +\t/* Update the estimated exposure and gain. */\n> > +\tactiveState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;\n> > +\tactiveState.agc.automatic.gain = aGain;\n> > +\n> > \tfillMetadata(context, frameContext, metadata);\n> \n> ...and here it would be reset.\n> \n> > }\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index b0de4898..1271741e 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -14,6 +14,7 @@\n> > \n> > #include \n> > \n> > +#include \"libipa/agc.h\"\n> > #include \"libipa/histogram.h\"\n> > \n> > #include \"algorithm.h\"\n> > @@ -22,12 +23,13 @@ namespace libcamera {\n> > \n> > namespace ipa::rkisp1::algorithms {\n> > \n> > -class Agc : public Algorithm\n> > +class Agc : public Algorithm, public MeanLuminanceAgc\n> > {\n> > public:\n> > \tAgc();\n> > \t~Agc() = default;\n> > \n> > +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> > \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > \tvoid queueRequest(IPAContext &context,\n> > \t\t\t const uint32_t frame,\n> > @@ -51,6 +53,7 @@ private:\n> > \t\t\t ControlList &metadata);\n> > \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n> > \t\t\t IPAContext &context);\n> > +\tdouble estimateLuminance(double gain) override;\n> > \n> > \tuint64_t frameCount_;\n> > \n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 10d8f38c..256b75eb 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -12,6 +12,7 @@\n> > \n> > #include \n> > \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -67,6 +68,8 @@ struct IPAActiveState {\n> > \t\t} automatic;\n> > \n> > \t\tbool autoEnabled;\n> > +\t\tuint32_t constraintMode;\n> > +\t\tuint32_t exposureMode;\n> > \t} agc;\n> > \n> > \tstruct {\n> > @@ -151,6 +154,8 @@ struct IPAContext {\n> > \tIPAActiveState activeState;\n> > \n> > \tFCQueue frameContexts;\n> > +\n> > +\tControlInfoMap::Map ctrlMap;\n> > };\n> > \n> > } /* namespace ipa::rkisp1 */\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 9dc5f53c..5f74762f 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{\n> > } /* namespace */\n> > \n> > IPARkISP1::IPARkISP1()\n> > -\t: context_({ {}, {}, {}, { kMaxFrameContexts } })\n> > +\t: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })\n> > {\n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 18AF9BD16B\n\tfor ;\n\tSat, 6 Apr 2024 01:56:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FAB26333E;\n\tSat, 6 Apr 2024 03:56:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5D0661C15\n\tfor ;\n\tSat, 6 Apr 2024 03:56:41 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 96B184D4;\n\tSat, 6 Apr 2024 03:56:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rLwqwin/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712368562;\n\tbh=Cz+riU8QdwfzIb2Z4IuzSZ8WtnXzhVxQmil5cfwdgDs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rLwqwin/fjaKIsaQ2vdp25A+h4ETrEGNDYjzYOZUw0EPfKNd3Dh88SmamFUIjmrZK\n\tk6esOGrXecr3+y2yYXKQOcupRBxA7yLVRNl/cANvn/o1dV3fuYs/Wi+z+gwnPF/9ek\n\tzvTupyEHjWKXGtoMoQqLyTLEVMV35gpC64FzP63A=","Date":"Sat, 6 Apr 2024 04:56:30 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"<20240406015630.GP12507@pendragon.ideasonboard.com>","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-10-dan.scally@ideasonboard.com>\n\t<20240327161104.n2ryp73fqdx2f2vk@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240327161104.n2ryp73fqdx2f2vk@jasper>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29178,"web_url":"https://patchwork.libcamera.org/comment/29178/","msgid":"","date":"2024-04-08T10:08:20","subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Laurent\n\nOn 06/04/2024 02:56, Laurent Pinchart wrote:\n> On Wed, Mar 27, 2024 at 05:11:04PM +0100, Stefan Klug wrote:\n>> Hi Daniel,\n>>\n>> thanks for the patch.\n>>\n>> On Fri, Mar 22, 2024 at 01:14:50PM +0000, Daniel Scally wrote:\n>>> Now that we have a MeanLuminanceAgc class that centralises our AEGC\n>>> algorithm, derive the RkISP1's Agc class from it and plumb in the\n>>> necessary framework to enable it to be used. For simplicities sake\n>>> this commit switches the algorithm to use the derived class, but\n>>> does not remove the bespoke functions at this time.\n>>>\n>>> Signed-off-by: Daniel Scally \n>>> ---\n>>> src/ipa/rkisp1/algorithms/agc.cpp | 91 +++++++++++++++++++++++++++++--\n>>> src/ipa/rkisp1/algorithms/agc.h | 5 +-\n>>> src/ipa/rkisp1/ipa_context.h | 5 ++\n>>> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n>>> 4 files changed, 96 insertions(+), 7 deletions(-)\n>>>\n>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>>> index d380194d..3389c471 100644\n>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>>> @@ -64,6 +64,36 @@ Agc::Agc()\n>>> \tsupportsRaw_ = true;\n>>> }\n>>> \n>>> +/**\n>>> + * \\brief Initialise the AGC algorithm from tuning files\n>>> + *\n>>> + * \\param[in] context The shared IPA context\n>>> + * \\param[in] tuningData The YamlObject containing Agc tuning data\n>>> + *\n>>> + * This function calls the base class' tuningData parsers to discover which\n>>> + * control values are supported.\n>>> + *\n>>> + * \\return 0 on success or errors from the base class\n>>> + */\n>>> +int Agc::init(IPAContext &context, const YamlObject &tuningData)\n>>> +{\n>>> +\tint ret;\n>>> +\n>>> +\tparseRelativeLuminanceTarget(tuningData);\n>>> +\n>>> +\tret = parseConstraintModes(tuningData);\n>>> +\tif (ret)\n>>> +\t\treturn ret;\n>>> +\n>>> +\tret = parseExposureModes(tuningData);\n>>> +\tif (ret)\n>>> +\t\treturn ret;\n>>> +\n>> Same comment as for ipu3\n>>\n>>> +\tcontext.ctrlMap.merge(controls());\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> /**\n>>> * \\brief Configure the AGC given a configInfo\n>>> * \\param[in] context The shared IPA context\n>>> @@ -81,6 +111,13 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>>> \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n>>> \tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n>>> \n>>> +\t/*\n>>> +\t* \\todo We should use the first available mode rather than assume that\n>>> +\t* the \"Normal\" modes are present in tuning data.\n>>> +\t*/\n>>> +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n>>> +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n>>> +\n>>> \t/*\n>>> \t * Define the measurement window for AGC as a centered rectangle\n>>> \t * covering 3/4 of the image width and height.\n>>> @@ -95,6 +132,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>>> \t * frame index\n>>> \t */\n>>> \tframeCount_ = 0;\n>>> +\n>>> +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n>>> +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n>>> +\t\thelper->configure(context.configuration.sensor.minShutterSpeed,\n>>> +\t\t\t\t context.configuration.sensor.maxShutterSpeed,\n>> Not part of this patch, but why are these called minShutterSpeed and not\n>> minExposureTime?\n> Let's bite the bullet and define a standard vocabulary.\n\n\nActually thinking about it rather than equally applicable vocabulary that we need to choose between \nthey just seem to be named wrongly at the moment.  The variables are set in IPARkISP1::configure() \nlike so:\n\n         const auto itExp = sensorControls_.find(V4L2_CID_EXPOSURE);\n         int32_t minExposure = itExp->second.min().get();\n         int32_t maxExposure = itExp->second.max().get();\n\n...\n\n         context_.configuration.sensor.minShutterSpeed =\n                 minExposure * context_.configuration.sensor.lineDuration;\n         context_.configuration.sensor.maxShutterSpeed =\n                 maxExposure * context_.configuration.sensor.lineDuration;\n\n\nBut that means they're backwards - the minimum (i.e. slowest) shutter speed should produce the \n_longest_ exposure time and vice versa...we could reverse the min/max prefixes instead but that's \neven more confusing. So I think this definitely should be updated to minExposureTime and \nmaxExposureTime.\n\n>>> +\t\t\t\t context.configuration.sensor.minAnalogueGain,\n>>> +\t\t\t\t context.configuration.sensor.maxAnalogueGain);\n>>> +\t}\n>>> +\n>>> \treturn 0;\n>>> }\n>>> \n>>> @@ -234,7 +280,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n>>> \t\t\t double yGain, double iqMeanGain)\n>>> {\n>>> \tIPASessionConfiguration &configuration = context.configuration;\n>>> -\tIPAActiveState &activeState = context.activeState;\n>>> \n>>> \t/* Get the effective exposure and gain applied on the sensor. */\n>>> \tuint32_t exposure = frameContext.sensor.exposure;\n>>> @@ -300,10 +345,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n>>> \tLOG(RkISP1Agc, Debug) << \"Divided up shutter and gain are \"\n>>> \t\t\t << shutterTime << \" and \"\n>>> \t\t\t << stepGain;\n>>> -\n>>> -\t/* Update the estimated exposure and gain. */\n>>> -\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n>>> -\tactiveState.agc.automatic.gain = stepGain;\n>>> }\n>>> \n>>> /**\n>>> @@ -383,6 +424,19 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,\n>>> \t\t\t\t\t context.hw->numHistogramBins));\n>>> }\n>>> \n>>> +double Agc::estimateLuminance(double gain)\n>>> +{\n>>> +\tdouble ySum = 0.0;\n>>> +\n>>> +\t/* Sum the averages, saturated to 255. */\n>>> +\tfor (uint8_t expMean : expMeans_)\n>>> +\t\tySum += std::min(expMean * gain, 255.0);\n>>> +\n>>> +\t/* \\todo Weight with the AWB gains */\n> How so ?\n\n\nThe comment is copied from the existing implementation, but it's something that the IPU3 and RPi \nimplementations do as well. I assume that without the colour correction the luminance estimation \nfrom RGB values is corrupted. The RkISP1 doesn't use RGB values here, but internally it estimates Y \nhere using (R+G+B) x 0.332, and possibly those need correcting. I think that's only necessary \ndepending on the position in the pipeline that the statistics are drawn from though; possibly \nthey're post auto-white-balance anyway in which case it ought not matter, but I'm not sure where the \ntap points are for RkISP1.\n\n>\n>>> +\n>>> +\treturn ySum / expMeans_.size() / 255;\n>>> +}\n>>> +\n>>> /**\n>>> * \\brief Process RkISP1 statistics, and run AGC operations\n>>> * \\param[in] context The shared IPA context\n>>> @@ -448,6 +502,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>>> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n>>> \tframeCount_++;\n>>> \n>>> +\tparseStatistics(stats, context);\n>> Regarding my previous mail. Here expMean_ would be assigned...\n>>\n>>> +\n>>> +\t/*\n>>> +\t * The Agc algorithm needs to know the effective exposure value that was\n>>> +\t * applied to the sensor when the statistics were collected.\n>>> +\t */\n>>> +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n>>> +\t\t\t\t * frameContext.sensor.exposure;\n>>> +\tdouble analogueGain = frameContext.sensor.gain;\n>>> +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n>>> +\n>>> +\tutils::Duration shutterTime;\n>>> +\tdouble aGain, dGain;\n>>> +\tstd::tie(shutterTime, aGain, dGain) =\n>>> +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n>>> +\t\t\t context.activeState.agc.exposureMode, hist_,\n>>> +\t\t\t effectiveExposureValue);\n>>> +\n>>> +\tLOG(RkISP1Agc, Debug)\n>>> +\t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n>>> +\t\t<< shutterTime << \", \" << aGain << \" and \" << dGain;\n>>> +\n>>> +\tIPAActiveState &activeState = context.activeState;\n>>> +\t/* Update the estimated exposure and gain. */\n>>> +\tactiveState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;\n>>> +\tactiveState.agc.automatic.gain = aGain;\n>>> +\n>>> \tfillMetadata(context, frameContext, metadata);\n>> ...and here it would be reset.\n>>\n>>> }\n>>> \n>>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n>>> index b0de4898..1271741e 100644\n>>> --- a/src/ipa/rkisp1/algorithms/agc.h\n>>> +++ b/src/ipa/rkisp1/algorithms/agc.h\n>>> @@ -14,6 +14,7 @@\n>>> \n>>> #include \n>>> \n>>> +#include \"libipa/agc.h\"\n>>> #include \"libipa/histogram.h\"\n>>> \n>>> #include \"algorithm.h\"\n>>> @@ -22,12 +23,13 @@ namespace libcamera {\n>>> \n>>> namespace ipa::rkisp1::algorithms {\n>>> \n>>> -class Agc : public Algorithm\n>>> +class Agc : public Algorithm, public MeanLuminanceAgc\n>>> {\n>>> public:\n>>> \tAgc();\n>>> \t~Agc() = default;\n>>> \n>>> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n>>> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n>>> \tvoid queueRequest(IPAContext &context,\n>>> \t\t\t const uint32_t frame,\n>>> @@ -51,6 +53,7 @@ private:\n>>> \t\t\t ControlList &metadata);\n>>> \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n>>> \t\t\t IPAContext &context);\n>>> +\tdouble estimateLuminance(double gain) override;\n>>> \n>>> \tuint64_t frameCount_;\n>>> \n>>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n>>> index 10d8f38c..256b75eb 100644\n>>> --- a/src/ipa/rkisp1/ipa_context.h\n>>> +++ b/src/ipa/rkisp1/ipa_context.h\n>>> @@ -12,6 +12,7 @@\n>>> \n>>> #include \n>>> \n>>> +#include \n>>> #include \n>>> \n>>> #include \n>>> @@ -67,6 +68,8 @@ struct IPAActiveState {\n>>> \t\t} automatic;\n>>> \n>>> \t\tbool autoEnabled;\n>>> +\t\tuint32_t constraintMode;\n>>> +\t\tuint32_t exposureMode;\n>>> \t} agc;\n>>> \n>>> \tstruct {\n>>> @@ -151,6 +154,8 @@ struct IPAContext {\n>>> \tIPAActiveState activeState;\n>>> \n>>> \tFCQueue frameContexts;\n>>> +\n>>> +\tControlInfoMap::Map ctrlMap;\n>>> };\n>>> \n>>> } /* namespace ipa::rkisp1 */\n>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>> index 9dc5f53c..5f74762f 100644\n>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{\n>>> } /* namespace */\n>>> \n>>> IPARkISP1::IPARkISP1()\n>>> -\t: context_({ {}, {}, {}, { kMaxFrameContexts } })\n>>> +\t: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })\n>>> {\n>>> }\n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0366BBE08B\n\tfor ;\n\tMon, 8 Apr 2024 10:08:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E482D63352;\n\tMon, 8 Apr 2024 12:08:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8BB1161C28\n\tfor ;\n\tMon, 8 Apr 2024 12:08:23 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 61912A27;\n\tMon, 8 Apr 2024 12:07:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nVdZb0jZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712570862;\n\tbh=NzNGtVPrgppYVXF3amnH8Xahu4W2MQZOsmdyv8r7rgU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=nVdZb0jZkWRHiZskmiMWt4CFNGeyLbOh6t5pE/iCmbXMRmNvFFuYXHayiaqiRI6xJ\n\tqpJxNjLtgkwHajii8FaYWLf69apYAlMlA+PoS49vU3GbiAgIKIiEi38GCCnCLBp7BR\n\tgK0EztuMcJvzwV6buAsh0FG945ehPvrAcHKM+0Ec=","Message-ID":"","Date":"Mon, 8 Apr 2024 11:08:20 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","To":"Laurent Pinchart ,\n\tStefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-10-dan.scally@ideasonboard.com>\n\t<20240327161104.n2ryp73fqdx2f2vk@jasper>\n\t<20240406015630.GP12507@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240406015630.GP12507@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29185,"web_url":"https://patchwork.libcamera.org/comment/29185/","msgid":"","date":"2024-04-09T10:13:25","subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Fri, Mar 22, 2024 at 01:14:50PM +0000, Daniel Scally wrote:\n> Now that we have a MeanLuminanceAgc class that centralises our AEGC\n> algorithm, derive the RkISP1's Agc class from it and plumb in the\n> necessary framework to enable it to be used. For simplicities sake\n> this commit switches the algorithm to use the derived class, but\n> does not remove the bespoke functions at this time.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 91 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/agc.h | 5 +-\n> src/ipa/rkisp1/ipa_context.h | 5 ++\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> 4 files changed, 96 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index d380194d..3389c471 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -64,6 +64,36 @@ Agc::Agc()\n> \tsupportsRaw_ = true;\n> }\n> \n> +/**\n> + * \\brief Initialise the AGC algorithm from tuning files\n> + *\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] tuningData The YamlObject containing Agc tuning data\n> + *\n> + * This function calls the base class' tuningData parsers to discover which\n> + * control values are supported.\n> + *\n> + * \\return 0 on success or errors from the base class\n> + */\n> +int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> +{\n> +\tint ret;\n> +\n> +\tparseRelativeLuminanceTarget(tuningData);\n> +\n> +\tret = parseConstraintModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = parseExposureModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tcontext.ctrlMap.merge(controls());\n\nAh, is this how you gather the available controls + values from the\nalgorithms so that the IPA can report then? That might arguably be\nbetter than what I did instead on top.\n\n\nPaul\n\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Configure the AGC given a configInfo\n> * \\param[in] context The shared IPA context\n> @@ -81,6 +111,13 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> \tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n> \n> +\t/*\n> +\t* \\todo We should use the first available mode rather than assume that\n> +\t* the \"Normal\" modes are present in tuning data.\n> +\t*/\n> +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n> +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n> +\n> \t/*\n> \t * Define the measurement window for AGC as a centered rectangle\n> \t * covering 3/4 of the image width and height.\n> @@ -95,6 +132,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t * frame index\n> \t */\n> \tframeCount_ = 0;\n> +\n> +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n> +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> +\t\thelper->configure(context.configuration.sensor.minShutterSpeed,\n> +\t\t\t\t context.configuration.sensor.maxShutterSpeed,\n> +\t\t\t\t context.configuration.sensor.minAnalogueGain,\n> +\t\t\t\t context.configuration.sensor.maxAnalogueGain);\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -234,7 +280,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t double yGain, double iqMeanGain)\n> {\n> \tIPASessionConfiguration &configuration = context.configuration;\n> -\tIPAActiveState &activeState = context.activeState;\n> \n> \t/* Get the effective exposure and gain applied on the sensor. */\n> \tuint32_t exposure = frameContext.sensor.exposure;\n> @@ -300,10 +345,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \tLOG(RkISP1Agc, Debug) << \"Divided up shutter and gain are \"\n> \t\t\t << shutterTime << \" and \"\n> \t\t\t << stepGain;\n> -\n> -\t/* Update the estimated exposure and gain. */\n> -\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> -\tactiveState.agc.automatic.gain = stepGain;\n> }\n> \n> /**\n> @@ -383,6 +424,19 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,\n> \t\t\t\t\t context.hw->numHistogramBins));\n> }\n> \n> +double Agc::estimateLuminance(double gain)\n> +{\n> +\tdouble ySum = 0.0;\n> +\n> +\t/* Sum the averages, saturated to 255. */\n> +\tfor (uint8_t expMean : expMeans_)\n> +\t\tySum += std::min(expMean * gain, 255.0);\n> +\n> +\t/* \\todo Weight with the AWB gains */\n> +\n> +\treturn ySum / expMeans_.size() / 255;\n> +}\n> +\n> /**\n> * \\brief Process RkISP1 statistics, and run AGC operations\n> * \\param[in] context The shared IPA context\n> @@ -448,6 +502,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> \n> +\tparseStatistics(stats, context);\n> +\n> +\t/*\n> +\t * The Agc algorithm needs to know the effective exposure value that was\n> +\t * applied to the sensor when the statistics were collected.\n> +\t */\n> +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> +\t\t\t\t * frameContext.sensor.exposure;\n> +\tdouble analogueGain = frameContext.sensor.gain;\n> +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> +\n> +\tutils::Duration shutterTime;\n> +\tdouble aGain, dGain;\n> +\tstd::tie(shutterTime, aGain, dGain) =\n> +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n> +\t\t\t context.activeState.agc.exposureMode, hist_,\n> +\t\t\t effectiveExposureValue);\n> +\n> +\tLOG(RkISP1Agc, Debug)\n> +\t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> +\t\t<< shutterTime << \", \" << aGain << \" and \" << dGain;\n> +\n> +\tIPAActiveState &activeState = context.activeState;\n> +\t/* Update the estimated exposure and gain. */\n> +\tactiveState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;\n> +\tactiveState.agc.automatic.gain = aGain;\n> +\n> \tfillMetadata(context, frameContext, metadata);\n> }\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index b0de4898..1271741e 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -14,6 +14,7 @@\n> \n> #include \n> \n> +#include \"libipa/agc.h\"\n> #include \"libipa/histogram.h\"\n> \n> #include \"algorithm.h\"\n> @@ -22,12 +23,13 @@ namespace libcamera {\n> \n> namespace ipa::rkisp1::algorithms {\n> \n> -class Agc : public Algorithm\n> +class Agc : public Algorithm, public MeanLuminanceAgc\n> {\n> public:\n> \tAgc();\n> \t~Agc() = default;\n> \n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> \tvoid queueRequest(IPAContext &context,\n> \t\t\t const uint32_t frame,\n> @@ -51,6 +53,7 @@ private:\n> \t\t\t ControlList &metadata);\n> \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n> \t\t\t IPAContext &context);\n> +\tdouble estimateLuminance(double gain) override;\n> \n> \tuint64_t frameCount_;\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 10d8f38c..256b75eb 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -12,6 +12,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -67,6 +68,8 @@ struct IPAActiveState {\n> \t\t} automatic;\n> \n> \t\tbool autoEnabled;\n> +\t\tuint32_t constraintMode;\n> +\t\tuint32_t exposureMode;\n> \t} agc;\n> \n> \tstruct {\n> @@ -151,6 +154,8 @@ struct IPAContext {\n> \tIPAActiveState activeState;\n> \n> \tFCQueue frameContexts;\n> +\n> +\tControlInfoMap::Map ctrlMap;\n> };\n> \n> } /* namespace ipa::rkisp1 */\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 9dc5f53c..5f74762f 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{\n> } /* namespace */\n> \n> IPARkISP1::IPARkISP1()\n> -\t: context_({ {}, {}, {}, { kMaxFrameContexts } })\n> +\t: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })\n> {\n> }\n> \n> -- \n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 631A6C0DA4\n\tfor ;\n\tTue, 9 Apr 2024 10:13:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4638B63352;\n\tTue, 9 Apr 2024 12:13:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C4E56333B\n\tfor ;\n\tTue, 9 Apr 2024 12:13:32 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA6E08B9;\n\tTue, 9 Apr 2024 12:12:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uAzfBdVr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712657570;\n\tbh=j6YYRC5mvsJN4oVO0r4Iat3uPN2AAByt1Ap4McMctC0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uAzfBdVreWSLgWNgUd6yOpRwrcfXvJXCrATedg5jqjVdazCSewxbrVLNGsXh4tfI2\n\tmCkcMRRkzIQLI8mkjCmUm1JPgPFtqS6cwI47mnkN43Oaxh80HInaYxyxqypvLD+THO\n\tW2DBWRq0i0nC1XZDqVfp37zysribdWm5H38Ks2OQ=","Date":"Tue, 9 Apr 2024 19:13:25 +0900","From":"Paul Elder ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-10-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240322131451.3092931-10-dan.scally@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29187,"web_url":"https://patchwork.libcamera.org/comment/29187/","msgid":"","date":"2024-04-09T10:29:23","subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul\n\nOn 09/04/2024 11:13, Paul Elder wrote:\n> On Fri, Mar 22, 2024 at 01:14:50PM +0000, Daniel Scally wrote:\n>> Now that we have a MeanLuminanceAgc class that centralises our AEGC\n>> algorithm, derive the RkISP1's Agc class from it and plumb in the\n>> necessary framework to enable it to be used. For simplicities sake\n>> this commit switches the algorithm to use the derived class, but\n>> does not remove the bespoke functions at this time.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> src/ipa/rkisp1/algorithms/agc.cpp | 91 +++++++++++++++++++++++++++++--\n>> src/ipa/rkisp1/algorithms/agc.h | 5 +-\n>> src/ipa/rkisp1/ipa_context.h | 5 ++\n>> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n>> 4 files changed, 96 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>> index d380194d..3389c471 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>> @@ -64,6 +64,36 @@ Agc::Agc()\n>> \tsupportsRaw_ = true;\n>> }\n>> \n>> +/**\n>> + * \\brief Initialise the AGC algorithm from tuning files\n>> + *\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] tuningData The YamlObject containing Agc tuning data\n>> + *\n>> + * This function calls the base class' tuningData parsers to discover which\n>> + * control values are supported.\n>> + *\n>> + * \\return 0 on success or errors from the base class\n>> + */\n>> +int Agc::init(IPAContext &context, const YamlObject &tuningData)\n>> +{\n>> +\tint ret;\n>> +\n>> +\tparseRelativeLuminanceTarget(tuningData);\n>> +\n>> +\tret = parseConstraintModes(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tret = parseExposureModes(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tcontext.ctrlMap.merge(controls());\n> Ah, is this how you gather the available controls + values from the\n> algorithms so that the IPA can report then? That might arguably be\n> better than what I did instead on top.\n\n\nYes...although not actually implemented here (I'll do it properly in the v2) I just added a \nctrlMap.merge(context_.ctrlMap) to the IPA's ::init() to include these in the ControlInfoMap that it \nconstructs.\n\n>\n>\n> Paul\n>\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> /**\n>> * \\brief Configure the AGC given a configInfo\n>> * \\param[in] context The shared IPA context\n>> @@ -81,6 +111,13 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>> \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n>> \tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n>> \n>> +\t/*\n>> +\t* \\todo We should use the first available mode rather than assume that\n>> +\t* the \"Normal\" modes are present in tuning data.\n>> +\t*/\n>> +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n>> +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n>> +\n>> \t/*\n>> \t * Define the measurement window for AGC as a centered rectangle\n>> \t * covering 3/4 of the image width and height.\n>> @@ -95,6 +132,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>> \t * frame index\n>> \t */\n>> \tframeCount_ = 0;\n>> +\n>> +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n>> +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n>> +\t\thelper->configure(context.configuration.sensor.minShutterSpeed,\n>> +\t\t\t\t context.configuration.sensor.maxShutterSpeed,\n>> +\t\t\t\t context.configuration.sensor.minAnalogueGain,\n>> +\t\t\t\t context.configuration.sensor.maxAnalogueGain);\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>> \n>> @@ -234,7 +280,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n>> \t\t\t double yGain, double iqMeanGain)\n>> {\n>> \tIPASessionConfiguration &configuration = context.configuration;\n>> -\tIPAActiveState &activeState = context.activeState;\n>> \n>> \t/* Get the effective exposure and gain applied on the sensor. */\n>> \tuint32_t exposure = frameContext.sensor.exposure;\n>> @@ -300,10 +345,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n>> \tLOG(RkISP1Agc, Debug) << \"Divided up shutter and gain are \"\n>> \t\t\t << shutterTime << \" and \"\n>> \t\t\t << stepGain;\n>> -\n>> -\t/* Update the estimated exposure and gain. */\n>> -\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n>> -\tactiveState.agc.automatic.gain = stepGain;\n>> }\n>> \n>> /**\n>> @@ -383,6 +424,19 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,\n>> \t\t\t\t\t context.hw->numHistogramBins));\n>> }\n>> \n>> +double Agc::estimateLuminance(double gain)\n>> +{\n>> +\tdouble ySum = 0.0;\n>> +\n>> +\t/* Sum the averages, saturated to 255. */\n>> +\tfor (uint8_t expMean : expMeans_)\n>> +\t\tySum += std::min(expMean * gain, 255.0);\n>> +\n>> +\t/* \\todo Weight with the AWB gains */\n>> +\n>> +\treturn ySum / expMeans_.size() / 255;\n>> +}\n>> +\n>> /**\n>> * \\brief Process RkISP1 statistics, and run AGC operations\n>> * \\param[in] context The shared IPA context\n>> @@ -448,6 +502,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n>> \tframeCount_++;\n>> \n>> +\tparseStatistics(stats, context);\n>> +\n>> +\t/*\n>> +\t * The Agc algorithm needs to know the effective exposure value that was\n>> +\t * applied to the sensor when the statistics were collected.\n>> +\t */\n>> +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n>> +\t\t\t\t * frameContext.sensor.exposure;\n>> +\tdouble analogueGain = frameContext.sensor.gain;\n>> +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n>> +\n>> +\tutils::Duration shutterTime;\n>> +\tdouble aGain, dGain;\n>> +\tstd::tie(shutterTime, aGain, dGain) =\n>> +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n>> +\t\t\t context.activeState.agc.exposureMode, hist_,\n>> +\t\t\t effectiveExposureValue);\n>> +\n>> +\tLOG(RkISP1Agc, Debug)\n>> +\t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n>> +\t\t<< shutterTime << \", \" << aGain << \" and \" << dGain;\n>> +\n>> +\tIPAActiveState &activeState = context.activeState;\n>> +\t/* Update the estimated exposure and gain. */\n>> +\tactiveState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;\n>> +\tactiveState.agc.automatic.gain = aGain;\n>> +\n>> \tfillMetadata(context, frameContext, metadata);\n>> }\n>> \n>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n>> index b0de4898..1271741e 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.h\n>> +++ b/src/ipa/rkisp1/algorithms/agc.h\n>> @@ -14,6 +14,7 @@\n>> \n>> #include \n>> \n>> +#include \"libipa/agc.h\"\n>> #include \"libipa/histogram.h\"\n>> \n>> #include \"algorithm.h\"\n>> @@ -22,12 +23,13 @@ namespace libcamera {\n>> \n>> namespace ipa::rkisp1::algorithms {\n>> \n>> -class Agc : public Algorithm\n>> +class Agc : public Algorithm, public MeanLuminanceAgc\n>> {\n>> public:\n>> \tAgc();\n>> \t~Agc() = default;\n>> \n>> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n>> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n>> \tvoid queueRequest(IPAContext &context,\n>> \t\t\t const uint32_t frame,\n>> @@ -51,6 +53,7 @@ private:\n>> \t\t\t ControlList &metadata);\n>> \tvoid parseStatistics(const rkisp1_stat_buffer *stats,\n>> \t\t\t IPAContext &context);\n>> +\tdouble estimateLuminance(double gain) override;\n>> \n>> \tuint64_t frameCount_;\n>> \n>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n>> index 10d8f38c..256b75eb 100644\n>> --- a/src/ipa/rkisp1/ipa_context.h\n>> +++ b/src/ipa/rkisp1/ipa_context.h\n>> @@ -12,6 +12,7 @@\n>> \n>> #include \n>> \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -67,6 +68,8 @@ struct IPAActiveState {\n>> \t\t} automatic;\n>> \n>> \t\tbool autoEnabled;\n>> +\t\tuint32_t constraintMode;\n>> +\t\tuint32_t exposureMode;\n>> \t} agc;\n>> \n>> \tstruct {\n>> @@ -151,6 +154,8 @@ struct IPAContext {\n>> \tIPAActiveState activeState;\n>> \n>> \tFCQueue frameContexts;\n>> +\n>> +\tControlInfoMap::Map ctrlMap;\n>> };\n>> \n>> } /* namespace ipa::rkisp1 */\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 9dc5f53c..5f74762f 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{\n>> } /* namespace */\n>> \n>> IPARkISP1::IPARkISP1()\n>> -\t: context_({ {}, {}, {}, { kMaxFrameContexts } })\n>> +\t: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })\n>> {\n>> }\n>> \n>> -- \n>> 2.34.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D833BC0DA4\n\tfor ;\n\tTue, 9 Apr 2024 10:29:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 218F26333F;\n\tTue, 9 Apr 2024 12:29:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E6FC6333B\n\tfor ;\n\tTue, 9 Apr 2024 12:29:27 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E1E38B9;\n\tTue, 9 Apr 2024 12:28:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GyvZp9pI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712658525;\n\tbh=RILsQUxX8r1g8CBnLkgvMju2qcCngG48edLgcINbFUs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=GyvZp9pIL3m3yAsyzflnWBP6MRjb6QLzTAr2VV1yI1XOH51uD7qwQLy6If25xptqH\n\txnaRMnsoha/PWAch5Bd/XXxw6XaNCrQg6egGdaiDCRr76Kiv5de24BrBf8cr4LOL2D\n\txmXZ+DHJaau7v2Dd/zcOt0Qu2R8sydnpof3llei4=","Message-ID":"","Date":"Tue, 9 Apr 2024 11:29:23 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 09/10] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-10-dan.scally@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]