[{"id":29261,"web_url":"https://patchwork.libcamera.org/comment/29261/","msgid":"<20240418124113.6bjbyv2yhdmytqzz@jasper>","date":"2024-04-18T12:41:13","subject":"Re: [PATCH v2 7/8] 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.\nLooks good to me now.\n\nOn Wed, Apr 17, 2024 at 02:15:35PM +0100, 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\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> ---\n> Changes in v2:\n> \n> \t- Squashed the patch adding parseStatistics()\n> \t- Used the first available constraint and exposure modes rather than\n> \t assuming the \"Normal\" mode was available\n> \t- Stopped storing a Histogram in the class members when it's only needed\n> \t during ::process()\n> \t- Remembered to report the controls out to IPARkISP1::updateControls()\n> \n> src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n> src/ipa/rkisp1/ipa_context.h | 5 ++\n> src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> 4 files changed, 92 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 47a6f7b2..0d66fcca 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -64,6 +64,30 @@ 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> +\tret = parseTuningData(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\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 +105,9 @@ 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> +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t * frame index\n> \t */\n> \tframeCount_ = 0;\n> +\n> +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> +\tconfigureExposureModeHelpers(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> +\n> +\tresetFrameCount();\n> +\n> \treturn 0;\n> }\n> \n> @@ -234,7 +270,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 +335,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> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \tmetadata.set(controls::FrameDuration, frameDuration.get());\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> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> \n> +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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,\n> +\t\t\t Histogram(hist), 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> +\texpMeans_ = {};\n> }\n> \n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index fb82a33f..a8080228 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -14,18 +14,22 @@\n> \n> #include \n> \n> +#include \"libipa/agc_mean_luminance.h\"\n> +#include \"libipa/histogram.h\"\n> +\n> #include \"algorithm.h\"\n> \n> namespace libcamera {\n> \n> namespace ipa::rkisp1::algorithms {\n> \n> -class Agc : public Algorithm\n> +class Agc : public Algorithm, public AgcMeanLuminance\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> @@ -47,10 +51,13 @@ private:\n> \tdouble measureBrightness(Span hist) const;\n> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t ControlList &metadata);\n> +\tdouble estimateLuminance(double gain) override;\n> \n> \tuint64_t frameCount_;\n> \n> \tutils::Duration filteredExposure_;\n> +\n> +\tSpan expMeans_;\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t\t\t\t\t frameDurations[1],\n> \t\t\t\t\t\t\t frameDurations[2]);\n> \n> +\tctrlMap.merge(context_.ctrlMap);\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 67667BE08B\n\tfor ;\n\tThu, 18 Apr 2024 12:41:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32A11633F3;\n\tThu, 18 Apr 2024 14:41:18 +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 8A9A761B4F\n\tfor ;\n\tThu, 18 Apr 2024 14:41:16 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c257:f1f7:8fc8:140f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B844427C;\n\tThu, 18 Apr 2024 14:40:28 +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=\"F6OM0dgj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713444028;\n\tbh=7CCNXxYplQB1tsulQYOuoDqqw6v8gLpl//3rl1IpXao=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F6OM0dgjH35fOhkuAggR1fqG9p5TRrk1LRVq4SSLm11EvjLTqJLhtiRN34m6BCeEB\n\tJaq4OCmRaUep7Zul+D1EdlnWCXMPUylOLN6yvf+SZ+HVfmpmbaBGmbQkZl6R+MRG38\n\t2DdLkM1q7YcJokBijD3xgH9mS1eYeDdFUjLmZNoo=","Date":"Thu, 18 Apr 2024 14:41:13 +0200","From":"Stefan Klug ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"<20240418124113.6bjbyv2yhdmytqzz@jasper>","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240417131536.484129-8-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":29266,"web_url":"https://patchwork.libcamera.org/comment/29266/","msgid":"","date":"2024-04-19T02:53:51","subject":"Re: [PATCH v2 7/8] 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":"Hi Dan,\n\nOn Wed, Apr 17, 2024 at 02:15:35PM +0100, 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\nLooks good to me.\n\nReviewed-by: Paul Elder \n\n> ---\n> Changes in v2:\n> \n> \t- Squashed the patch adding parseStatistics()\n> \t- Used the first available constraint and exposure modes rather than\n> \t assuming the \"Normal\" mode was available\n> \t- Stopped storing a Histogram in the class members when it's only needed\n> \t during ::process()\n> \t- Remembered to report the controls out to IPARkISP1::updateControls()\n> \n> src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n> src/ipa/rkisp1/ipa_context.h | 5 ++\n> src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> 4 files changed, 92 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 47a6f7b2..0d66fcca 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -64,6 +64,30 @@ 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> +\tret = parseTuningData(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\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 +105,9 @@ 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> +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t * frame index\n> \t */\n> \tframeCount_ = 0;\n> +\n> +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> +\tconfigureExposureModeHelpers(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> +\n> +\tresetFrameCount();\n> +\n> \treturn 0;\n> }\n> \n> @@ -234,7 +270,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 +335,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> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \tmetadata.set(controls::FrameDuration, frameDuration.get());\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> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> \n> +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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,\n> +\t\t\t Histogram(hist), 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> +\texpMeans_ = {};\n> }\n> \n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index fb82a33f..a8080228 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -14,18 +14,22 @@\n> \n> #include \n> \n> +#include \"libipa/agc_mean_luminance.h\"\n> +#include \"libipa/histogram.h\"\n> +\n> #include \"algorithm.h\"\n> \n> namespace libcamera {\n> \n> namespace ipa::rkisp1::algorithms {\n> \n> -class Agc : public Algorithm\n> +class Agc : public Algorithm, public AgcMeanLuminance\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> @@ -47,10 +51,13 @@ private:\n> \tdouble measureBrightness(Span hist) const;\n> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t ControlList &metadata);\n> +\tdouble estimateLuminance(double gain) override;\n> \n> \tuint64_t frameCount_;\n> \n> \tutils::Duration filteredExposure_;\n> +\n> +\tSpan expMeans_;\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t\t\t\t\t frameDurations[1],\n> \t\t\t\t\t\t\t frameDurations[2]);\n> \n> +\tctrlMap.merge(context_.ctrlMap);\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 0EF64BE08B\n\tfor ;\n\tFri, 19 Apr 2024 02:54:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2855D633F3;\n\tFri, 19 Apr 2024 04:54:01 +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 44B4161B17\n\tfor ;\n\tFri, 19 Apr 2024 04:53:59 +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 EE01C63B;\n\tFri, 19 Apr 2024 04:53:09 +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=\"VAz3PTiT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713495191;\n\tbh=NueOz6p7N5ZeKTY0OOdFi01t4sILnc0oyMWQb049MNI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VAz3PTiTR1AA6fMKPeUaDA2jLKgJuQ1oxyblUpkRO/ccqLXtAvL6bmbRrzuvznSSh\n\t4VfKw5sJPLvKtTlIlFyw/PBDo5tQ7dK3rNTvw6RzKkQEc9Y31ofGuqbq4b0BsgWmpn\n\tEIL0LDV0+T5GxVpETC6kdrUWXJigCUYpGbXeTAOg=","Date":"Fri, 19 Apr 2024 11:53:51 +0900","From":"Paul Elder ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240417131536.484129-8-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":29291,"web_url":"https://patchwork.libcamera.org/comment/29291/","msgid":"","date":"2024-04-22T10:49:25","subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Wed, Apr 17, 2024 at 02:15:35PM +0100, 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> Changes in v2:\n>\n> \t- Squashed the patch adding parseStatistics()\n> \t- Used the first available constraint and exposure modes rather than\n> \t assuming the \"Normal\" mode was available\n> \t- Stopped storing a Histogram in the class members when it's only needed\n> \t during ::process()\n> \t- Remembered to report the controls out to IPARkISP1::updateControls()\n>\n> src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n> src/ipa/rkisp1/ipa_context.h | 5 ++\n> src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> 4 files changed, 92 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 47a6f7b2..0d66fcca 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -64,6 +64,30 @@ Agc::Agc()\n> \tsupportsRaw_ = true;\n> }\n>\n> +/**\n> + * \\brief Initialise the AGC algorithm from tuning files\n> + *\n\nno empty line\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> +\tret = parseTuningData(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\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 +105,9 @@ 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> +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t * frame index\n> \t */\n> \tframeCount_ = 0;\n> +\n> +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> +\tconfigureExposureModeHelpers(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> +\n\nI think I also commented this on the base class, but this should just\nbe \"setLimits\" or something similar (iow do not expose the\n\"ExposureModeHelpers\" name to the derived classes)\n\n> +\tresetFrameCount();\n> +\n\nAs this HAS to be run at configure time, I would also consider a base\nclass 'configure()' which sets limits and resets the frame counter and\nhave derived classes call it\n\n> \treturn 0;\n> }\n>\n> @@ -234,7 +270,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 +335,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> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \tmetadata.set(controls::FrameDuration, frameDuration.get());\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> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n>\n> +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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\nsame question, is this a duration ?\n\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,\n> +\t\t\t Histogram(hist), 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> +\texpMeans_ = {};\n\nisn't this reset at the beginning of this function ?\n\nOr am I confused ? I see two estimateLuminance() (this is expected)\none uses expMeans_ the other doesn't, but expMeans_ is assigned after\nthe call to estimateLuminance() ?\n\n> }\n>\n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index fb82a33f..a8080228 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -14,18 +14,22 @@\n>\n> #include \n>\n> +#include \"libipa/agc_mean_luminance.h\"\n> +#include \"libipa/histogram.h\"\n> +\n> #include \"algorithm.h\"\n>\n> namespace libcamera {\n>\n> namespace ipa::rkisp1::algorithms {\n>\n> -class Agc : public Algorithm\n> +class Agc : public Algorithm, public AgcMeanLuminance\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> @@ -47,10 +51,13 @@ private:\n> \tdouble measureBrightness(Span hist) const;\n> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t ControlList &metadata);\n> +\tdouble estimateLuminance(double gain) override;\n>\n> \tuint64_t frameCount_;\n>\n> \tutils::Duration filteredExposure_;\n> +\n> +\tSpan expMeans_;\n> };\n>\n> } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t\t\t\t\t frameDurations[1],\n> \t\t\t\t\t\t\t frameDurations[2]);\n>\n> +\tctrlMap.merge(context_.ctrlMap);\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 AFC1EC3200\n\tfor ;\n\tMon, 22 Apr 2024 10:49:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 946C6633FF;\n\tMon, 22 Apr 2024 12:49:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C12E8633ED\n\tfor ;\n\tMon, 22 Apr 2024 12:49:28 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A941142E;\n\tMon, 22 Apr 2024 12:48:38 +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=\"GAzId/8V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713782918;\n\tbh=X3M/ax7RVWYg6ygMBIefuLauvScC89W1PouFXlrcP2w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GAzId/8VDE8JqH5rLIhXF1HM08UBvTzbkdlUNSX3soR5lhyswpTl1M4eMW91d5PBP\n\tCqpELM7mAtA3MjQPp/eoCOWi9Q6AOsoisQaKDVBFJQ7E6If6njIRFmWSOVx1L7OmFm\n\taTiUWUxbUYgcDBW2jjPf1bMgqI2Pixy1xNjpbF7A=","Date":"Mon, 22 Apr 2024 12:49:25 +0200","From":"Jacopo Mondi ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240417131536.484129-8-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":29292,"web_url":"https://patchwork.libcamera.org/comment/29292/","msgid":"","date":"2024-04-22T10:54:10","subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi again\n\nOn Mon, Apr 22, 2024 at 12:49:28PM +0200, Jacopo Mondi wrote:\n> Hi Dan\n>\n> On Wed, Apr 17, 2024 at 02:15:35PM +0100, 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> > Changes in v2:\n> >\n> > \t- Squashed the patch adding parseStatistics()\n> > \t- Used the first available constraint and exposure modes rather than\n> > \t assuming the \"Normal\" mode was available\n> > \t- Stopped storing a Histogram in the class members when it's only needed\n> > \t during ::process()\n> > \t- Remembered to report the controls out to IPARkISP1::updateControls()\n> >\n> > src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n> > src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n> > src/ipa/rkisp1/ipa_context.h | 5 ++\n> > src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> > 4 files changed, 92 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 47a6f7b2..0d66fcca 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -64,6 +64,30 @@ Agc::Agc()\n> > \tsupportsRaw_ = true;\n> > }\n> >\n> > +/**\n> > + * \\brief Initialise the AGC algorithm from tuning files\n> > + *\n>\n> no empty line\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> > +\tret = parseTuningData(tuningData);\n> > +\tif (ret)\n> > +\t\treturn ret;\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 +105,9 @@ 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> > +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \t * frame index\n> > \t */\n> > \tframeCount_ = 0;\n> > +\n> > +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> > +\tconfigureExposureModeHelpers(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> > +\n>\n> I think I also commented this on the base class, but this should just\n> be \"setLimits\" or something similar (iow do not expose the\n> \"ExposureModeHelpers\" name to the derived classes)\n>\n> > +\tresetFrameCount();\n> > +\n>\n> As this HAS to be run at configure time, I would also consider a base\n> class 'configure()' which sets limits and resets the frame counter and\n> have derived classes call it\n>\n> > \treturn 0;\n> > }\n> >\n> > @@ -234,7 +270,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 +335,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> > @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > \tmetadata.set(controls::FrameDuration, frameDuration.get());\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> > @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> > \tframeCount_++;\n> >\n> > +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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> same question, is this a duration ?\n>\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,\n> > +\t\t\t Histogram(hist), 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> > +\texpMeans_ = {};\n>\n> isn't this reset at the beginning of this function ?\n>\n> Or am I confused ? I see two estimateLuminance() (this is expected)\n> one uses expMeans_ the other doesn't, but expMeans_ is assigned after\n> the call to estimateLuminance() ?\n\nSorry, I was confused, this is used by the base class and it is called\nby calculateNewEv(), which is called here after expMeans_ is\nassigned...\n\nI think the documentation of the pure virtual declaration in the base class\nshould maybe specify what is the call path and that derived classes\nshould prepare for the override to be called during calculateNewEv()\n\n>\n> > }\n> >\n> > REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index fb82a33f..a8080228 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -14,18 +14,22 @@\n> >\n> > #include \n> >\n> > +#include \"libipa/agc_mean_luminance.h\"\n> > +#include \"libipa/histogram.h\"\n> > +\n> > #include \"algorithm.h\"\n> >\n> > namespace libcamera {\n> >\n> > namespace ipa::rkisp1::algorithms {\n> >\n> > -class Agc : public Algorithm\n> > +class Agc : public Algorithm, public AgcMeanLuminance\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> > @@ -47,10 +51,13 @@ private:\n> > \tdouble measureBrightness(Span hist) const;\n> > \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > \t\t\t ControlList &metadata);\n> > +\tdouble estimateLuminance(double gain) override;\n> >\n> > \tuint64_t frameCount_;\n> >\n> > \tutils::Duration filteredExposure_;\n> > +\n> > +\tSpan expMeans_;\n> > };\n> >\n> > } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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> > @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > \t\t\t\t\t\t\t frameDurations[1],\n> > \t\t\t\t\t\t\t frameDurations[2]);\n> >\n> > +\tctrlMap.merge(context_.ctrlMap);\n> > \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 8F1C5BE08B\n\tfor ;\n\tMon, 22 Apr 2024 10:54:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90B4E6340A;\n\tMon, 22 Apr 2024 12:54:16 +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 E46F1633FF\n\tfor ;\n\tMon, 22 Apr 2024 12:54:13 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49E16142E;\n\tMon, 22 Apr 2024 12:53:23 +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=\"P1wv+CWT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713783203;\n\tbh=YGBUkIznKEt9SVfHA9V1cpZ0/5jzKrcpqzIOra0aB3E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P1wv+CWT/+++e3rlmP9ZpzfkJzt1xlbF+WtGGvoKJKhUTlO+CvLmr0ELk9obEACHv\n\tb9cI0NdkLs5QoE+bL3v5tSJ04VNPBjHVxyYk/Vn//lOUEO36kg9ZFji4Nz0l/HT4K1\n\tsdAhgTOrM66Ffy8ztkSwcGw2oOYI4py/5xVqlAdY=","Date":"Mon, 22 Apr 2024 12:54:10 +0200","From":"Jacopo Mondi ","To":"Jacopo Mondi ","Cc":"Daniel Scally , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-dan.scally@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":29311,"web_url":"https://patchwork.libcamera.org/comment/29311/","msgid":"<82d0a6a3-e634-49d3-bb37-88b3ab55e2ad@ideasonboard.com>","date":"2024-04-23T09:48:07","subject":"Re: [PATCH v2 7/8] 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 Jacopo\n\nOn 22/04/2024 11:49, Jacopo Mondi wrote:\n> Hi Dan\n>\n> On Wed, Apr 17, 2024 at 02:15:35PM +0100, 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>> Changes in v2:\n>>\n>> \t- Squashed the patch adding parseStatistics()\n>> \t- Used the first available constraint and exposure modes rather than\n>> \t assuming the \"Normal\" mode was available\n>> \t- Stopped storing a Histogram in the class members when it's only needed\n>> \t during ::process()\n>> \t- Remembered to report the controls out to IPARkISP1::updateControls()\n>>\n>> src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n>> src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n>> src/ipa/rkisp1/ipa_context.h | 5 ++\n>> src/ipa/rkisp1/rkisp1.cpp | 3 +-\n>> 4 files changed, 92 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>> index 47a6f7b2..0d66fcca 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>> @@ -64,6 +64,30 @@ Agc::Agc()\n>> \tsupportsRaw_ = true;\n>> }\n>>\n>> +/**\n>> + * \\brief Initialise the AGC algorithm from tuning files\n>> + *\n> no empty line\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>> +\tret = parseTuningData(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\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 +105,9 @@ 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>> +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n>> +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>> \t * frame index\n>> \t */\n>> \tframeCount_ = 0;\n>> +\n>> +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n>> +\tconfigureExposureModeHelpers(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>> +\n> I think I also commented this on the base class, but this should just\n> be \"setLimits\" or something similar (iow do not expose the\n> \"ExposureModeHelpers\" name to the derived classes)\n>\n>> +\tresetFrameCount();\n>> +\n> As this HAS to be run at configure time, I would also consider a base\n> class 'configure()' which sets limits and resets the frame counter and\n> have derived classes call it\n>\n>> \treturn 0;\n>> }\n>>\n>> @@ -234,7 +270,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 +335,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>> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>> \tmetadata.set(controls::FrameDuration, frameDuration.get());\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>> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n>> \tframeCount_++;\n>>\n>> +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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> same question, is this a duration ?\n>\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,\n>> +\t\t\t Histogram(hist), 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>> +\texpMeans_ = {};\n> isn't this reset at the beginning of this function ?\n>\n> Or am I confused ? I see two estimateLuminance() (this is expected)\n> one uses expMeans_ the other doesn't, but expMeans_ is assigned after\n> the call to estimateLuminance() ?\n\n\nIt's assigned after the call to the bespoke estimateLuminance() - the overriding function for the \nbase class is called later from calculateNewEv(). Resetting expMeans_ at the end of ::process() was \na suggestion from Stefan, since it's holding references to stats->params->ae that might not be valid \nafter process() is over.\n\n>\n>> }\n>>\n>> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n>> index fb82a33f..a8080228 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.h\n>> +++ b/src/ipa/rkisp1/algorithms/agc.h\n>> @@ -14,18 +14,22 @@\n>>\n>> #include \n>>\n>> +#include \"libipa/agc_mean_luminance.h\"\n>> +#include \"libipa/histogram.h\"\n>> +\n>> #include \"algorithm.h\"\n>>\n>> namespace libcamera {\n>>\n>> namespace ipa::rkisp1::algorithms {\n>>\n>> -class Agc : public Algorithm\n>> +class Agc : public Algorithm, public AgcMeanLuminance\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>> @@ -47,10 +51,13 @@ private:\n>> \tdouble measureBrightness(Span hist) const;\n>> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>> \t\t\t ControlList &metadata);\n>> +\tdouble estimateLuminance(double gain) override;\n>>\n>> \tuint64_t frameCount_;\n>>\n>> \tutils::Duration filteredExposure_;\n>> +\n>> +\tSpan expMeans_;\n>> };\n>>\n>> } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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>> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\t\t\t\t\t\t frameDurations[1],\n>> \t\t\t\t\t\t\t frameDurations[2]);\n>>\n>> +\tctrlMap.merge(context_.ctrlMap);\n>> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 5E759BE08B\n\tfor ;\n\tTue, 23 Apr 2024 09:48:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91A3D6341D;\n\tTue, 23 Apr 2024 11:48:11 +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 12CC063417\n\tfor ;\n\tTue, 23 Apr 2024 11:48:10 +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 B6A0E65D;\n\tTue, 23 Apr 2024 11:47:18 +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=\"rPoFKbrC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713865638;\n\tbh=c2ey+3MMCFIMHfLfdJRLHsWwFkyPtUJkPhkKTDBz+xQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=rPoFKbrCFlGTOwL4aefJf3eD7jOJ13tsJQxnJLx5fm7rsDu9Uf/GW2PTv8yGAA6p7\n\tcfFiPpZ/U9RAmzvGmf/zi+TtPHBv55nvGxGSimbrQHy6A4gRipVAeuxNgiMl/+oZI3\n\ta4jSLWuUpL7NJkPlI5CrJHvGevPvDyb7phcZju/w=","Message-ID":"<82d0a6a3-e634-49d3-bb37-88b3ab55e2ad@ideasonboard.com>","Date":"Tue, 23 Apr 2024 10:48:07 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-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\" "}},{"id":29330,"web_url":"https://patchwork.libcamera.org/comment/29330/","msgid":"<1ffff549-8348-46f9-8c60-e9448d134e8e@ideasonboard.com>","date":"2024-04-24T10:01:18","subject":"Re: [PATCH v2 7/8] 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 Jacopo\n\nOn 22/04/2024 11:49, Jacopo Mondi wrote:\n> Hi Dan\n>\n> On Wed, Apr 17, 2024 at 02:15:35PM +0100, 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>> Changes in v2:\n>>\n>> \t- Squashed the patch adding parseStatistics()\n>> \t- Used the first available constraint and exposure modes rather than\n>> \t assuming the \"Normal\" mode was available\n>> \t- Stopped storing a Histogram in the class members when it's only needed\n>> \t during ::process()\n>> \t- Remembered to report the controls out to IPARkISP1::updateControls()\n>>\n>> src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n>> src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n>> src/ipa/rkisp1/ipa_context.h | 5 ++\n>> src/ipa/rkisp1/rkisp1.cpp | 3 +-\n>> 4 files changed, 92 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>> index 47a6f7b2..0d66fcca 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>> @@ -64,6 +64,30 @@ Agc::Agc()\n>> \tsupportsRaw_ = true;\n>> }\n>>\n>> +/**\n>> + * \\brief Initialise the AGC algorithm from tuning files\n>> + *\n> no empty line\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>> +\tret = parseTuningData(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\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 +105,9 @@ 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>> +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n>> +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>> \t * frame index\n>> \t */\n>> \tframeCount_ = 0;\n>> +\n>> +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n>> +\tconfigureExposureModeHelpers(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>> +\n> I think I also commented this on the base class, but this should just\n> be \"setLimits\" or something similar (iow do not expose the\n> \"ExposureModeHelpers\" name to the derived classes)\nWhy not, particularly?  Deriving from AgcMeanLuminance inherently means relying on \nExposureModeHelpers, why would we have to hide them?\n>> +\tresetFrameCount();\n>> +\n> As this HAS to be run at configure time, I would also consider a base\n> class 'configure()' which sets limits and resets the frame counter and\n> have derived classes call it\n\nSure, we can do that\n>\n>> \treturn 0;\n>> }\n>>\n>> @@ -234,7 +270,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 +335,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>> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>> \tmetadata.set(controls::FrameDuration, frameDuration.get());\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>> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n>> \tframeCount_++;\n>>\n>> +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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> same question, is this a duration ?\n>\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,\n>> +\t\t\t Histogram(hist), 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>> +\texpMeans_ = {};\n> isn't this reset at the beginning of this function ?\n>\n> Or am I confused ? I see two estimateLuminance() (this is expected)\n> one uses expMeans_ the other doesn't, but expMeans_ is assigned after\n> the call to estimateLuminance() ?\n>\n>> }\n>>\n>> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n>> index fb82a33f..a8080228 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.h\n>> +++ b/src/ipa/rkisp1/algorithms/agc.h\n>> @@ -14,18 +14,22 @@\n>>\n>> #include \n>>\n>> +#include \"libipa/agc_mean_luminance.h\"\n>> +#include \"libipa/histogram.h\"\n>> +\n>> #include \"algorithm.h\"\n>>\n>> namespace libcamera {\n>>\n>> namespace ipa::rkisp1::algorithms {\n>>\n>> -class Agc : public Algorithm\n>> +class Agc : public Algorithm, public AgcMeanLuminance\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>> @@ -47,10 +51,13 @@ private:\n>> \tdouble measureBrightness(Span hist) const;\n>> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>> \t\t\t ControlList &metadata);\n>> +\tdouble estimateLuminance(double gain) override;\n>>\n>> \tuint64_t frameCount_;\n>>\n>> \tutils::Duration filteredExposure_;\n>> +\n>> +\tSpan expMeans_;\n>> };\n>>\n>> } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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>> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\t\t\t\t\t\t frameDurations[1],\n>> \t\t\t\t\t\t\t frameDurations[2]);\n>>\n>> +\tctrlMap.merge(context_.ctrlMap);\n>> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 2F51BBE08B\n\tfor ;\n\tWed, 24 Apr 2024 10:01:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A4A43633F3;\n\tWed, 24 Apr 2024 12:01:22 +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 2774861B43\n\tfor ;\n\tWed, 24 Apr 2024 12:01:21 +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 2FABE674;\n\tWed, 24 Apr 2024 12:00:29 +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=\"ZXkFpT33\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713952829;\n\tbh=84J3vChIbUrmWSHKmxiuQLBez/7vSIlvQKu2pKFV56k=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ZXkFpT33wRI/aKumLf5pIDuXeuRnbGbVU5kmLyFZl+cFnbZ+3sUcBgJJroEpae22+\n\tW7ZFiVoe2WQ2W2fXY/tXI3nELvfXtoSOYMMxlBCljHXq8oQWCwJiwTkVBYJaVGI8Qt\n\tE5hHu2HA1YSwm+AZMZOIekYlfV801c4qe3syvNfU=","Message-ID":"<1ffff549-8348-46f9-8c60-e9448d134e8e@ideasonboard.com>","Date":"Wed, 24 Apr 2024 11:01:18 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-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":"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":29331,"web_url":"https://patchwork.libcamera.org/comment/29331/","msgid":"<53502a81-681d-4940-9f01-1a9a490ebe2e@ideasonboard.com>","date":"2024-04-24T10:05:18","subject":"Re: [PATCH v2 7/8] 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 Jacopo\n\nOn 22/04/2024 11:49, Jacopo Mondi wrote:\n> Hi Dan\n>\n> On Wed, Apr 17, 2024 at 02:15:35PM +0100, 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>> Changes in v2:\n>>\n>> \t- Squashed the patch adding parseStatistics()\n>> \t- Used the first available constraint and exposure modes rather than\n>> \t assuming the \"Normal\" mode was available\n>> \t- Stopped storing a Histogram in the class members when it's only needed\n>> \t during ::process()\n>> \t- Remembered to report the controls out to IPARkISP1::updateControls()\n>>\n>> src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n>> src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n>> src/ipa/rkisp1/ipa_context.h | 5 ++\n>> src/ipa/rkisp1/rkisp1.cpp | 3 +-\n>> 4 files changed, 92 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>> index 47a6f7b2..0d66fcca 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>> @@ -64,6 +64,30 @@ Agc::Agc()\n>> \tsupportsRaw_ = true;\n>> }\n>>\n>> +/**\n>> + * \\brief Initialise the AGC algorithm from tuning files\n>> + *\n> no empty line\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>> +\tret = parseTuningData(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\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 +105,9 @@ 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>> +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n>> +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>> \t * frame index\n>> \t */\n>> \tframeCount_ = 0;\n>> +\n>> +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n>> +\tconfigureExposureModeHelpers(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>> +\n> I think I also commented this on the base class, but this should just\n> be \"setLimits\" or something similar (iow do not expose the\n> \"ExposureModeHelpers\" name to the derived classes)\n>\n>> +\tresetFrameCount();\n>> +\n> As this HAS to be run at configure time, I would also consider a base\n> class 'configure()' which sets limits and resets the frame counter and\n> have derived classes call it\n\n\nActually I'm vacillating on this, because we have to call configureExposureModeHelpers() (or \nsetLimits() or whatever it ends up to be) outside of the derived class' ::configure() call too, like \nin queueRequest() if FrameDurationLimits are set that will change the minimum and maximum shutter \nspeeds...is it worth bundling them into an AgcMeanLuminance::configure() function if it'll be called \noutside that too?\n\n>> \treturn 0;\n>> }\n>>\n>> @@ -234,7 +270,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 +335,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>> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>> \tmetadata.set(controls::FrameDuration, frameDuration.get());\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>> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n>> \tframeCount_++;\n>>\n>> +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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> same question, is this a duration ?\n>\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,\n>> +\t\t\t Histogram(hist), 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>> +\texpMeans_ = {};\n> isn't this reset at the beginning of this function ?\n>\n> Or am I confused ? I see two estimateLuminance() (this is expected)\n> one uses expMeans_ the other doesn't, but expMeans_ is assigned after\n> the call to estimateLuminance() ?\n>\n>> }\n>>\n>> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n>> index fb82a33f..a8080228 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.h\n>> +++ b/src/ipa/rkisp1/algorithms/agc.h\n>> @@ -14,18 +14,22 @@\n>>\n>> #include \n>>\n>> +#include \"libipa/agc_mean_luminance.h\"\n>> +#include \"libipa/histogram.h\"\n>> +\n>> #include \"algorithm.h\"\n>>\n>> namespace libcamera {\n>>\n>> namespace ipa::rkisp1::algorithms {\n>>\n>> -class Agc : public Algorithm\n>> +class Agc : public Algorithm, public AgcMeanLuminance\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>> @@ -47,10 +51,13 @@ private:\n>> \tdouble measureBrightness(Span hist) const;\n>> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>> \t\t\t ControlList &metadata);\n>> +\tdouble estimateLuminance(double gain) override;\n>>\n>> \tuint64_t frameCount_;\n>>\n>> \tutils::Duration filteredExposure_;\n>> +\n>> +\tSpan expMeans_;\n>> };\n>>\n>> } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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>> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\t\t\t\t\t\t frameDurations[1],\n>> \t\t\t\t\t\t\t frameDurations[2]);\n>>\n>> +\tctrlMap.merge(context_.ctrlMap);\n>> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 BFF74BE08B\n\tfor ;\n\tWed, 24 Apr 2024 10:05:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAC59633F5;\n\tWed, 24 Apr 2024 12:05:22 +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 27664633EC\n\tfor ;\n\tWed, 24 Apr 2024 12:05:21 +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 347A36B3;\n\tWed, 24 Apr 2024 12:04:29 +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=\"spZdWuOr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713953069;\n\tbh=XEfWcRm02GlVvITfoVY2OvnM4HmWbfPAVtvZp7jQ5Lc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=spZdWuOrPYIIvR78Y7sYuwQoAJ8l1oRzpWpyB+Nrcu1S9O4D45THGnGQhy1c+52Rj\n\tNfvVGQfCjSS/iiBFiOk7q7/ctkA/cR8OPW9KoMWHZTQZT92aeUOv79Kjpcm0XXq7fD\n\tpltRpVTu5vnwbFY57y62NmG70xPhvbnSJ2OXqgnI=","Message-ID":"<53502a81-681d-4940-9f01-1a9a490ebe2e@ideasonboard.com>","Date":"Wed, 24 Apr 2024 11:05:18 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-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\" "}},{"id":29332,"web_url":"https://patchwork.libcamera.org/comment/29332/","msgid":"","date":"2024-04-24T10:07:23","subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Wed, Apr 24, 2024 at 11:01:18AM +0100, Dan Scally wrote:\n> Hi Jacopo\n>\n> On 22/04/2024 11:49, Jacopo Mondi wrote:\n> > Hi Dan\n> >\n> > On Wed, Apr 17, 2024 at 02:15:35PM +0100, 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> > > Changes in v2:\n> > >\n> > > \t- Squashed the patch adding parseStatistics()\n> > > \t- Used the first available constraint and exposure modes rather than\n> > > \t assuming the \"Normal\" mode was available\n> > > \t- Stopped storing a Histogram in the class members when it's only needed\n> > > \t during ::process()\n> > > \t- Remembered to report the controls out to IPARkISP1::updateControls()\n> > >\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n> > > src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n> > > src/ipa/rkisp1/ipa_context.h | 5 ++\n> > > src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> > > 4 files changed, 92 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 47a6f7b2..0d66fcca 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -64,6 +64,30 @@ Agc::Agc()\n> > > \tsupportsRaw_ = true;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Initialise the AGC algorithm from tuning files\n> > > + *\n> > no empty line\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> > > +\tret = parseTuningData(tuningData);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\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 +105,9 @@ 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> > > +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > > +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > \t * frame index\n> > > \t */\n> > > \tframeCount_ = 0;\n> > > +\n> > > +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> > > +\tconfigureExposureModeHelpers(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> > > +\n> > I think I also commented this on the base class, but this should just\n> > be \"setLimits\" or something similar (iow do not expose the\n> > \"ExposureModeHelpers\" name to the derived classes)\n> Why not, particularly?  Deriving from AgcMeanLuminance inherently means\n> relying on ExposureModeHelpers, why would we have to hide them?\n\nThe base class uses it, the derived classes simply rely on the base\nclass calculateNewEv() function, right ?\n\nHow the base class does that it's opaque to the derived classes,\nhenche I don't see any benefits in mentioning the helper is a function\nname. Isn't 'setLimits()' enough ? What is the benfint of mentioning\nthe ExposureModeHelper here ?\n\n> > > +\tresetFrameCount();\n> > > +\n> > As this HAS to be run at configure time, I would also consider a base\n> > class 'configure()' which sets limits and resets the frame counter and\n> > have derived classes call it\n>\n> Sure, we can do that\n\nAnd that would also hide the helper\n\n> >\n> > > \treturn 0;\n> > > }\n> > >\n> > > @@ -234,7 +270,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 +335,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> > > @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > \tmetadata.set(controls::FrameDuration, frameDuration.get());\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> > > @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> > > \tframeCount_++;\n> > >\n> > > +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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> > same question, is this a duration ?\n> >\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,\n> > > +\t\t\t Histogram(hist), 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> > > +\texpMeans_ = {};\n> > isn't this reset at the beginning of this function ?\n> >\n> > Or am I confused ? I see two estimateLuminance() (this is expected)\n> > one uses expMeans_ the other doesn't, but expMeans_ is assigned after\n> > the call to estimateLuminance() ?\n> >\n> > > }\n> > >\n> > > REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > index fb82a33f..a8080228 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > @@ -14,18 +14,22 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"libipa/agc_mean_luminance.h\"\n> > > +#include \"libipa/histogram.h\"\n> > > +\n> > > #include \"algorithm.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > namespace ipa::rkisp1::algorithms {\n> > >\n> > > -class Agc : public Algorithm\n> > > +class Agc : public Algorithm, public AgcMeanLuminance\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> > > @@ -47,10 +51,13 @@ private:\n> > > \tdouble measureBrightness(Span hist) const;\n> > > \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > \t\t\t ControlList &metadata);\n> > > +\tdouble estimateLuminance(double gain) override;\n> > >\n> > > \tuint64_t frameCount_;\n> > >\n> > > \tutils::Duration filteredExposure_;\n> > > +\n> > > +\tSpan expMeans_;\n> > > };\n> > >\n> > > } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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> > > @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > > \t\t\t\t\t\t\t frameDurations[1],\n> > > \t\t\t\t\t\t\t frameDurations[2]);\n> > >\n> > > +\tctrlMap.merge(context_.ctrlMap);\n> > > \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 DBD18C3200\n\tfor ;\n\tWed, 24 Apr 2024 10:07:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0595E633F4;\n\tWed, 24 Apr 2024 12:07:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C0A661B43\n\tfor ;\n\tWed, 24 Apr 2024 12:07:26 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7DAFE6B3;\n\tWed, 24 Apr 2024 12:06:34 +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=\"rIRe9hQQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713953194;\n\tbh=rOyCSn4ZDvGfF74hdebh7ZZKFOxYFiNCJqDiuy4hdXo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rIRe9hQQoLaQY+BRB/P49q7o/ySijHJz2lSO44QjwcxJgvnzLCxvDf5VMabU08bPE\n\tbkqubCwQm5r1zhvK/tHW+qUsWLDp5Xp1RUjgbC9qofaXTMWYDILiqSt7tCuO1S0UA2\n\tXz9rDEYh+khO7FGrTSvCe/va7GMK1HVj3GN7a7Ug=","Date":"Wed, 24 Apr 2024 12:07:23 +0200","From":"Jacopo Mondi ","To":"Dan Scally ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-dan.scally@ideasonboard.com>\n\t\n\t<1ffff549-8348-46f9-8c60-e9448d134e8e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1ffff549-8348-46f9-8c60-e9448d134e8e@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":29333,"web_url":"https://patchwork.libcamera.org/comment/29333/","msgid":"","date":"2024-04-24T10:14:55","subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Wed, Apr 24, 2024 at 11:05:18AM +0100, Dan Scally wrote:\n> Hi Jacopo\n>\n> On 22/04/2024 11:49, Jacopo Mondi wrote:\n> > Hi Dan\n> >\n> > On Wed, Apr 17, 2024 at 02:15:35PM +0100, 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> > > Changes in v2:\n> > >\n> > > \t- Squashed the patch adding parseStatistics()\n> > > \t- Used the first available constraint and exposure modes rather than\n> > > \t assuming the \"Normal\" mode was available\n> > > \t- Stopped storing a Histogram in the class members when it's only needed\n> > > \t during ::process()\n> > > \t- Remembered to report the controls out to IPARkISP1::updateControls()\n> > >\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--\n> > > src/ipa/rkisp1/algorithms/agc.h | 9 +++-\n> > > src/ipa/rkisp1/ipa_context.h | 5 ++\n> > > src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> > > 4 files changed, 92 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 47a6f7b2..0d66fcca 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -64,6 +64,30 @@ Agc::Agc()\n> > > \tsupportsRaw_ = true;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Initialise the AGC algorithm from tuning files\n> > > + *\n> > no empty line\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> > > +\tret = parseTuningData(tuningData);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\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 +105,9 @@ 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> > > +\tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > > +\tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\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 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > \t * frame index\n> > > \t */\n> > > \tframeCount_ = 0;\n> > > +\n> > > +\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> > > +\tconfigureExposureModeHelpers(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> > > +\n> > I think I also commented this on the base class, but this should just\n> > be \"setLimits\" or something similar (iow do not expose the\n> > \"ExposureModeHelpers\" name to the derived classes)\n> >\n> > > +\tresetFrameCount();\n> > > +\n> > As this HAS to be run at configure time, I would also consider a base\n> > class 'configure()' which sets limits and resets the frame counter and\n> > have derived classes call it\n>\n>\n> Actually I'm vacillating on this, because we have to call\n> configureExposureModeHelpers() (or setLimits() or whatever it ends up to be)\n> outside of the derived class' ::configure() call too, like in queueRequest()\n> if FrameDurationLimits are set that will change the minimum and maximum\n> shutter speeds...is it worth bundling them into an\n> AgcMeanLuminance::configure() function if it'll be called outside that too?\n>\n\nOk, maybe not then, but be sure in the documentation of the base class\nto mention that derived classes have to set limits and reset the frame\ncounter during their configure() implementation.\n\n\n> > > \treturn 0;\n> > > }\n> > >\n> > > @@ -234,7 +270,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 +335,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> > > @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > \tmetadata.set(controls::FrameDuration, frameDuration.get());\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> > > @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> > > \tframeCount_++;\n> > >\n> > > +\texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\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> > same question, is this a duration ?\n> >\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,\n> > > +\t\t\t Histogram(hist), 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> > > +\texpMeans_ = {};\n> > isn't this reset at the beginning of this function ?\n> >\n> > Or am I confused ? I see two estimateLuminance() (this is expected)\n> > one uses expMeans_ the other doesn't, but expMeans_ is assigned after\n> > the call to estimateLuminance() ?\n> >\n> > > }\n> > >\n> > > REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > index fb82a33f..a8080228 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > @@ -14,18 +14,22 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"libipa/agc_mean_luminance.h\"\n> > > +#include \"libipa/histogram.h\"\n> > > +\n> > > #include \"algorithm.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > namespace ipa::rkisp1::algorithms {\n> > >\n> > > -class Agc : public Algorithm\n> > > +class Agc : public Algorithm, public AgcMeanLuminance\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> > > @@ -47,10 +51,13 @@ private:\n> > > \tdouble measureBrightness(Span hist) const;\n> > > \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > \t\t\t ControlList &metadata);\n> > > +\tdouble estimateLuminance(double gain) override;\n> > >\n> > > \tuint64_t frameCount_;\n> > >\n> > > \tutils::Duration filteredExposure_;\n> > > +\n> > > +\tSpan expMeans_;\n> > > };\n> > >\n> > > } /* namespace ipa::rkisp1::algorithms */\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..d8610095 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> > > @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > > \t\t\t\t\t\t\t frameDurations[1],\n> > > \t\t\t\t\t\t\t frameDurations[2]);\n> > >\n> > > +\tctrlMap.merge(context_.ctrlMap);\n> > > \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 4874EBE08B\n\tfor ;\n\tWed, 24 Apr 2024 10:15:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FB7C633F5;\n\tWed, 24 Apr 2024 12:15:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13EBB633F2\n\tfor ;\n\tWed, 24 Apr 2024 12:14:59 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BD416B3;\n\tWed, 24 Apr 2024 12:14:06 +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=\"PyxI+rPH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713953647;\n\tbh=gwjLsvsiN6cHsgz4mJOt9pbiBPAfjEmdd86T2szSxVE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PyxI+rPHhHvfv/i8ZDmiHcyWQu0szWHc7QWO5w+J9VqrOwgFs6jxV31lLf0awHPxg\n\toizQmnNJtjZi1CS9zlqQSoBL2HBH1hZA1AkySNWogn0c38Fi5HZax0bRbLEWY4R173\n\tx8K62nkmgWxgXdpb8khW26N8+bp9JS+LouHRY+FM=","Date":"Wed, 24 Apr 2024 12:14:55 +0200","From":"Jacopo Mondi ","To":"Dan Scally ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-8-dan.scally@ideasonboard.com>\n\t\n\t<53502a81-681d-4940-9f01-1a9a490ebe2e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<53502a81-681d-4940-9f01-1a9a490ebe2e@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\" "}}]