[{"id":29087,"web_url":"https://patchwork.libcamera.org/comment/29087/","msgid":"<20240327144832.n7kcqgpkuk2x4wly@jasper>","date":"2024-03-27T15:02:21","subject":"Re: [PATCH 06/10] ipa: ipu3: Derive ipu3::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Daniel,\n\nthanks for the patch. \n\nOn Fri, Mar 22, 2024 at 01:14:47PM +0000, Daniel Scally wrote:\n> Now that we have a MeanLuminanceAgc class that centralises our AEGC\n> algorithm, derive the IPU3's Agc class from it and plumb in the\n> necessary framework to enable it to be used. For simplicities sake\n> this commit switches the algorithm to use the derived class, but\n> does not remove the bespoke functions at this time.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 98 ++++++++++++++++++++++++++++++---\n> src/ipa/ipu3/algorithms/agc.h | 6 +-\n> src/ipa/ipu3/ipa_context.cpp | 3 +\n> src/ipa/ipu3/ipa_context.h | 5 ++\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> 5 files changed, 105 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index ee9a42cf..a84534ea 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -71,11 +71,41 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> static constexpr double kRelativeLuminanceTarget = 0.16;\n> \n> Agc::Agc()\n> -\t: frameCount_(0), minShutterSpeed_(0s),\n> -\t maxShutterSpeed_(0s), filteredExposure_(0s)\n> +\t: minShutterSpeed_(0s), maxShutterSpeed_(0s), context_(nullptr)\n> {\n> }\n> \n> +/**\n> + * \\brief Initialise the AGC algorith from tuning files\n> + *\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] tuningData The YamlObject containing Agc tuning data\n> + *\n> + * This function calls the base class' tuningData parsers to discover which\n> + * control values are supported.\n> + *\n> + * \\return 0 on success or errors from the base class\n> + */\n> +int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> +{\n> +\tint ret;\n> +\n> +\tparseRelativeLuminanceTarget(tuningData);\n> +\n> +\tret = parseConstraintModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = parseExposureModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n\nIf I got it right, the agc implementation in libipa requires all\nparseXXX to be called and even if the tuningData misses the relevant\nentries, dummy entries are generated. So I think these calls should be\nmoved to MeanLuminanceAgc::parseTuningData()\n\nCheers,\nStefan\n\n> +\tcontext.ctrlMap.merge(controls());\n> +\tcontext_ = &context;\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Configure the AGC given a configInfo\n> * \\param[in] context The shared IPA context\n> @@ -103,6 +133,20 @@ int Agc::configure(IPAContext &context,\n> \tactiveState.agc.exposure = 10ms / configuration.sensor.lineDuration;\n> \n> \tframeCount_ = 0;\n> +\n> +\t/*\n> +\t * \\todo We should use the first available mode rather than assume that\n> +\t * the \"Normal\" modes are present in tuning data.\n> +\t */\n> +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n> +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n> +\n> +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n> +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> +\t\thelper->configure(minShutterSpeed_, maxShutterSpeed_,\n> +\t\t\t\t minAnalogueGain_, maxAnalogueGain_);\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -280,11 +324,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \tLOG(IPU3Agc, Debug) << \"Divided up shutter and gain are \"\n> \t\t\t << shutterTime << \" and \"\n> \t\t\t << stepGain;\n> -\n> -\tIPAActiveState &activeState = context.activeState;\n> -\t/* Update the estimated exposure and gain. */\n> -\tactiveState.agc.exposure = shutterTime / configuration.sensor.lineDuration;\n> -\tactiveState.agc.gain = stepGain;\n> }\n> \n> /**\n> @@ -347,6 +386,26 @@ double Agc::estimateLuminance(IPAActiveState &activeState,\n> \treturn ySum / (grid.height * grid.width) / 255;\n> }\n> \n> +double Agc::estimateLuminance(double gain)\n> +{\n> +\tASSERT(reds_.size() == greens_.size());\n> +\tASSERT(greens_.size() == blues_.size());\n> +\tconst ipu3_uapi_grid_config &grid = context_->configuration.grid.bdsGrid;\n> +\tdouble redSum = 0, greenSum = 0, blueSum = 0;\n> +\n> +\tfor (unsigned int i = 0; i < reds_.size(); i++) {\n> +\t\tredSum += std::min(reds_[i] * gain, 255.0);\n> +\t\tgreenSum += std::min(greens_[i] * gain, 255.0);\n> +\t\tblueSum += std::min(blues_[i] * gain, 255.0);\n> +\t}\n> +\n> +\tdouble ySum = redSum * context_->activeState.awb.gains.red * 0.299\n> +\t\t+ greenSum * context_->activeState.awb.gains.green * 0.587\n> +\t\t+ blueSum * context_->activeState.awb.gains.blue * 0.114;\n> +\n> +\treturn ySum / (grid.height * grid.width) / 255;\n> +}\n> +\n> /**\n> * \\brief Process IPU3 statistics, and run AGC operations\n> * \\param[in] context The shared IPA context\n> @@ -399,8 +458,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> \n> +\tparseStatistics(stats, context.configuration.grid.bdsGrid);\n> +\n> +\t/*\n> +\t * The Agc algorithm needs to know the effective exposure value that was\n> +\t * applied to the sensor when the statistics were collected.\n> +\t */\n> \tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> \t\t\t\t * frameContext.sensor.exposure;\n> +\tdouble analogueGain = frameContext.sensor.gain;\n> +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> +\n> +\tutils::Duration shutterTime;\n> +\tdouble aGain, dGain;\n> +\tstd::tie(shutterTime, aGain, dGain) =\n> +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n> +\t\t\t context.activeState.agc.exposureMode, hist_,\n> +\t\t\t effectiveExposureValue);\n> +\n> +\tLOG(IPU3Agc, 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.exposure = shutterTime / context.configuration.sensor.lineDuration;\n> +\tactiveState.agc.gain = aGain;\n> +\n> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 7ed0ef7a..8405da9d 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -13,6 +13,7 @@\n> \n> #include \n> \n> +#include \"libipa/agc.h\"\n> #include \"libipa/histogram.h\"\n> \n> #include \"algorithm.h\"\n> @@ -23,12 +24,13 @@ struct IPACameraSensorInfo;\n> \n> namespace ipa::ipu3::algorithms {\n> \n> -class Agc : public Algorithm\n> +class Agc : public Algorithm, public MeanLuminanceAgc\n> {\n> public:\n> \tAgc();\n> \t~Agc() = default;\n> \n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> @@ -45,6 +47,7 @@ private:\n> \t\t\t\t const ipu3_uapi_grid_config &grid,\n> \t\t\t\t const ipu3_uapi_stats_3a *stats,\n> \t\t\t\t double gain);\n> +\tdouble estimateLuminance(double gain) override;\n> \tvoid parseStatistics(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> \n> @@ -59,6 +62,7 @@ private:\n> \tutils::Duration filteredExposure_;\n> \n> \tuint32_t stride_;\n> +\tIPAContext *context_;\n> \tstd::vector reds_;\n> \tstd::vector blues_;\n> \tstd::vector greens_;\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 959f314f..c4fb5642 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -47,6 +47,9 @@ namespace libcamera::ipa::ipu3 {\n> *\n> * \\var IPAContext::activeState\n> * \\brief The current state of IPA algorithms\n> + *\n> + * \\var IPAContext::ctrlMap\n> + * \\brief A ControlInfoMap::Map of controls populated by the algorithms\n> */\n> \n> /**\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index e9a3863b..a92cb6ce 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -12,6 +12,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -55,6 +56,8 @@ struct IPAActiveState {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> +\t\tuint32_t constraintMode;\n> +\t\tuint32_t exposureMode;\n> \t} agc;\n> \n> \tstruct {\n> @@ -85,6 +88,8 @@ struct IPAContext {\n> \tIPAActiveState activeState;\n> \n> \tFCQueue frameContexts;\n> +\n> +\tControlInfoMap::Map ctrlMap;\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 08ee6eb3..2fcc0c91 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -189,7 +189,7 @@ private:\n> };\n> \n> IPAIPU3::IPAIPU3()\n> -\t: context_({ {}, {}, { kMaxFrameContexts } })\n> +\t: context_({ {}, {}, { kMaxFrameContexts }, {} })\n> {\n> }\n> \n> -- \n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 23779BD160\n\tfor ;\n\tWed, 27 Mar 2024 15:02:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B317563331;\n\tWed, 27 Mar 2024 16:02:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0164D61C35\n\tfor ;\n\tWed, 27 Mar 2024 16:02:24 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:da09:7e54:ae7f:d731])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 643571571;\n\tWed, 27 Mar 2024 16:01:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rUwNKuWU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711551712;\n\tbh=kaknduIrIdNpUQ4JXicuFFZ3Tf3qaSrCMINGUE4YWnU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rUwNKuWUjE0wnOitcc6WM+GeRPyDrUmLmGEJjzoQWOwnxneyMzFhio3aYJUQXEKd4\n\twmbpkhiVaXm/a20ccvB8ZQKnk9sd+rbEGXcyF0d/Vw3gC2dHQhxakjhPhADLML5UQq\n\tH4wU0QPLHv13kObovU+DTRzdKnXbvAxOOGn8m4aM=","Date":"Wed, 27 Mar 2024 16:02:21 +0100","From":"Stefan Klug ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 06/10] ipa: ipu3: Derive ipu3::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"<20240327144832.n7kcqgpkuk2x4wly@jasper>","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-7-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240322131451.3092931-7-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":29088,"web_url":"https://patchwork.libcamera.org/comment/29088/","msgid":"","date":"2024-03-27T15:04:39","subject":"Re: [PATCH 06/10] ipa: ipu3: Derive ipu3::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 Stefan\n\nOn 27/03/2024 15:02, Stefan Klug wrote:\n> Hi Daniel,\n>\n> thanks for the patch.\n>\n> On Fri, Mar 22, 2024 at 01:14:47PM +0000, Daniel Scally wrote:\n>> Now that we have a MeanLuminanceAgc class that centralises our AEGC\n>> algorithm, derive the IPU3's Agc class from it and plumb in the\n>> necessary framework to enable it to be used. For simplicities sake\n>> this commit switches the algorithm to use the derived class, but\n>> does not remove the bespoke functions at this time.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 98 ++++++++++++++++++++++++++++++---\n>> src/ipa/ipu3/algorithms/agc.h | 6 +-\n>> src/ipa/ipu3/ipa_context.cpp | 3 +\n>> src/ipa/ipu3/ipa_context.h | 5 ++\n>> src/ipa/ipu3/ipu3.cpp | 2 +-\n>> 5 files changed, 105 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index ee9a42cf..a84534ea 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -71,11 +71,41 @@ static constexpr uint32_t kNumStartupFrames = 10;\n>> static constexpr double kRelativeLuminanceTarget = 0.16;\n>> \n>> Agc::Agc()\n>> -\t: frameCount_(0), minShutterSpeed_(0s),\n>> -\t maxShutterSpeed_(0s), filteredExposure_(0s)\n>> +\t: minShutterSpeed_(0s), maxShutterSpeed_(0s), context_(nullptr)\n>> {\n>> }\n>> \n>> +/**\n>> + * \\brief Initialise the AGC algorith from tuning files\n>> + *\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] tuningData The YamlObject containing Agc tuning data\n>> + *\n>> + * This function calls the base class' tuningData parsers to discover which\n>> + * control values are supported.\n>> + *\n>> + * \\return 0 on success or errors from the base class\n>> + */\n>> +int Agc::init(IPAContext &context, const YamlObject &tuningData)\n>> +{\n>> +\tint ret;\n>> +\n>> +\tparseRelativeLuminanceTarget(tuningData);\n>> +\n>> +\tret = parseConstraintModes(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tret = parseExposureModes(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n> If I got it right, the agc implementation in libipa requires all\n> parseXXX to be called\nCorrect\n> and even if the tuningData misses the relevant\n> entries, dummy entries are generated.\nCorrect\n> So I think these calls should be\n> moved to MeanLuminanceAgc::parseTuningData()\n\n\nOK, will do - thanks for the review!\n\n>\n> Cheers,\n> Stefan\n>\n>> +\tcontext.ctrlMap.merge(controls());\n>> +\tcontext_ = &context;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> /**\n>> * \\brief Configure the AGC given a configInfo\n>> * \\param[in] context The shared IPA context\n>> @@ -103,6 +133,20 @@ int Agc::configure(IPAContext &context,\n>> \tactiveState.agc.exposure = 10ms / configuration.sensor.lineDuration;\n>> \n>> \tframeCount_ = 0;\n>> +\n>> +\t/*\n>> +\t * \\todo We should use the first available mode rather than assume that\n>> +\t * the \"Normal\" modes are present in tuning data.\n>> +\t */\n>> +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n>> +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n>> +\n>> +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n>> +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n>> +\t\thelper->configure(minShutterSpeed_, maxShutterSpeed_,\n>> +\t\t\t\t minAnalogueGain_, maxAnalogueGain_);\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>> \n>> @@ -280,11 +324,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n>> \tLOG(IPU3Agc, Debug) << \"Divided up shutter and gain are \"\n>> \t\t\t << shutterTime << \" and \"\n>> \t\t\t << stepGain;\n>> -\n>> -\tIPAActiveState &activeState = context.activeState;\n>> -\t/* Update the estimated exposure and gain. */\n>> -\tactiveState.agc.exposure = shutterTime / configuration.sensor.lineDuration;\n>> -\tactiveState.agc.gain = stepGain;\n>> }\n>> \n>> /**\n>> @@ -347,6 +386,26 @@ double Agc::estimateLuminance(IPAActiveState &activeState,\n>> \treturn ySum / (grid.height * grid.width) / 255;\n>> }\n>> \n>> +double Agc::estimateLuminance(double gain)\n>> +{\n>> +\tASSERT(reds_.size() == greens_.size());\n>> +\tASSERT(greens_.size() == blues_.size());\n>> +\tconst ipu3_uapi_grid_config &grid = context_->configuration.grid.bdsGrid;\n>> +\tdouble redSum = 0, greenSum = 0, blueSum = 0;\n>> +\n>> +\tfor (unsigned int i = 0; i < reds_.size(); i++) {\n>> +\t\tredSum += std::min(reds_[i] * gain, 255.0);\n>> +\t\tgreenSum += std::min(greens_[i] * gain, 255.0);\n>> +\t\tblueSum += std::min(blues_[i] * gain, 255.0);\n>> +\t}\n>> +\n>> +\tdouble ySum = redSum * context_->activeState.awb.gains.red * 0.299\n>> +\t\t+ greenSum * context_->activeState.awb.gains.green * 0.587\n>> +\t\t+ blueSum * context_->activeState.awb.gains.blue * 0.114;\n>> +\n>> +\treturn ySum / (grid.height * grid.width) / 255;\n>> +}\n>> +\n>> /**\n>> * \\brief Process IPU3 statistics, and run AGC operations\n>> * \\param[in] context The shared IPA context\n>> @@ -399,8 +458,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n>> \tframeCount_++;\n>> \n>> +\tparseStatistics(stats, context.configuration.grid.bdsGrid);\n>> +\n>> +\t/*\n>> +\t * The Agc algorithm needs to know the effective exposure value that was\n>> +\t * applied to the sensor when the statistics were collected.\n>> +\t */\n>> \tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n>> \t\t\t\t * frameContext.sensor.exposure;\n>> +\tdouble analogueGain = frameContext.sensor.gain;\n>> +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n>> +\n>> +\tutils::Duration shutterTime;\n>> +\tdouble aGain, dGain;\n>> +\tstd::tie(shutterTime, aGain, dGain) =\n>> +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n>> +\t\t\t context.activeState.agc.exposureMode, hist_,\n>> +\t\t\t effectiveExposureValue);\n>> +\n>> +\tLOG(IPU3Agc, 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.exposure = shutterTime / context.configuration.sensor.lineDuration;\n>> +\tactiveState.agc.gain = aGain;\n>> +\n>> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n>> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n>> \n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index 7ed0ef7a..8405da9d 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -13,6 +13,7 @@\n>> \n>> #include \n>> \n>> +#include \"libipa/agc.h\"\n>> #include \"libipa/histogram.h\"\n>> \n>> #include \"algorithm.h\"\n>> @@ -23,12 +24,13 @@ struct IPACameraSensorInfo;\n>> \n>> namespace ipa::ipu3::algorithms {\n>> \n>> -class Agc : public Algorithm\n>> +class Agc : public Algorithm, public MeanLuminanceAgc\n>> {\n>> public:\n>> \tAgc();\n>> \t~Agc() = default;\n>> \n>> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> \tvoid process(IPAContext &context, const uint32_t frame,\n>> \t\t IPAFrameContext &frameContext,\n>> @@ -45,6 +47,7 @@ private:\n>> \t\t\t\t const ipu3_uapi_grid_config &grid,\n>> \t\t\t\t const ipu3_uapi_stats_3a *stats,\n>> \t\t\t\t double gain);\n>> +\tdouble estimateLuminance(double gain) override;\n>> \tvoid parseStatistics(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t const ipu3_uapi_grid_config &grid);\n>> \n>> @@ -59,6 +62,7 @@ private:\n>> \tutils::Duration filteredExposure_;\n>> \n>> \tuint32_t stride_;\n>> +\tIPAContext *context_;\n>> \tstd::vector reds_;\n>> \tstd::vector blues_;\n>> \tstd::vector greens_;\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index 959f314f..c4fb5642 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -47,6 +47,9 @@ namespace libcamera::ipa::ipu3 {\n>> *\n>> * \\var IPAContext::activeState\n>> * \\brief The current state of IPA algorithms\n>> + *\n>> + * \\var IPAContext::ctrlMap\n>> + * \\brief A ControlInfoMap::Map of controls populated by the algorithms\n>> */\n>> \n>> /**\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index e9a3863b..a92cb6ce 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -12,6 +12,7 @@\n>> \n>> #include \n>> \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -55,6 +56,8 @@ struct IPAActiveState {\n>> \tstruct {\n>> \t\tuint32_t exposure;\n>> \t\tdouble gain;\n>> +\t\tuint32_t constraintMode;\n>> +\t\tuint32_t exposureMode;\n>> \t} agc;\n>> \n>> \tstruct {\n>> @@ -85,6 +88,8 @@ struct IPAContext {\n>> \tIPAActiveState activeState;\n>> \n>> \tFCQueue frameContexts;\n>> +\n>> +\tControlInfoMap::Map ctrlMap;\n>> };\n>> \n>> } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 08ee6eb3..2fcc0c91 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -189,7 +189,7 @@ private:\n>> };\n>> \n>> IPAIPU3::IPAIPU3()\n>> -\t: context_({ {}, {}, { kMaxFrameContexts } })\n>> +\t: context_({ {}, {}, { kMaxFrameContexts }, {} })\n>> {\n>> }\n>> \n>> -- \n>> 2.34.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id ECA11BD160\n\tfor ;\n\tWed, 27 Mar 2024 15:04:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0732363331;\n\tWed, 27 Mar 2024 16:04:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C45DB61C35\n\tfor ;\n\tWed, 27 Mar 2024 16:04:42 +0100 (CET)","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 59BFE2CFB;\n\tWed, 27 Mar 2024 16:04:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Nhlcr3sA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711551850;\n\tbh=1mQix0QDLJD9GreQj1tuaay+6w2cfIZoWnGYwYDCbEw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Nhlcr3sAex0VvTsjYPH2fNEGLG+o/bXSwK/4rsvkYPK8WQKDEO1Zs1gFOEtvetRXc\n\tKcwufAbVGTWz4d2omQdV6SCB3IpGHIv6Imj1FaVWlwC1lo23xSl/NCn5EberQWmaqH\n\tVQQcc1lFEjTImV4AVC5q6GSIhBYZ1g6HPCY+WHv0=","Message-ID":"","Date":"Wed, 27 Mar 2024 15:04:39 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 06/10] ipa: ipu3: Derive ipu3::algorithms::Agc from\n\tMeanLuminanceAgc","Content-Language":"en-US","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-7-dan.scally@ideasonboard.com>\n\t<20240327144832.n7kcqgpkuk2x4wly@jasper>","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":"<20240327144832.n7kcqgpkuk2x4wly@jasper>","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":29169,"web_url":"https://patchwork.libcamera.org/comment/29169/","msgid":"<20240406013138.GM12507@pendragon.ideasonboard.com>","date":"2024-04-06T01:31:38","subject":"Re: [PATCH 06/10] ipa: ipu3: Derive ipu3::algorithms::Agc from\n\tMeanLuminanceAgc","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dan,\n\nThank you for the patch.\n\nOn Fri, Mar 22, 2024 at 01:14:47PM +0000, Daniel Scally wrote:\n> Now that we have a MeanLuminanceAgc class that centralises our AEGC\n> algorithm, derive the IPU3's Agc class from it and plumb in the\n> necessary framework to enable it to be used. For simplicities sake\n\ns/simplicities/simplicity's/\n\n> this commit switches the algorithm to use the derived class, but\n> does not remove the bespoke functions at this time.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 98 ++++++++++++++++++++++++++++++---\n> src/ipa/ipu3/algorithms/agc.h | 6 +-\n> src/ipa/ipu3/ipa_context.cpp | 3 +\n> src/ipa/ipu3/ipa_context.h | 5 ++\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> 5 files changed, 105 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index ee9a42cf..a84534ea 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -71,11 +71,41 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> static constexpr double kRelativeLuminanceTarget = 0.16;\n> \n> Agc::Agc()\n> -\t: frameCount_(0), minShutterSpeed_(0s),\n> -\t maxShutterSpeed_(0s), filteredExposure_(0s)\n> +\t: minShutterSpeed_(0s), maxShutterSpeed_(0s), context_(nullptr)\n> {\n> }\n> \n> +/**\n> + * \\brief Initialise the AGC algorith from tuning files\n> + *\n\nNo blank 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> +\tparseRelativeLuminanceTarget(tuningData);\n> +\n> +\tret = parseConstraintModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = parseExposureModes(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n\nTurn all this into a single function.\n\n> +\n> +\tcontext.ctrlMap.merge(controls());\n> +\tcontext_ = &context;\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Configure the AGC given a configInfo\n> * \\param[in] context The shared IPA context\n> @@ -103,6 +133,20 @@ int Agc::configure(IPAContext &context,\n> \tactiveState.agc.exposure = 10ms / configuration.sensor.lineDuration;\n> \n> \tframeCount_ = 0;\n> +\n> +\t/*\n> +\t * \\todo We should use the first available mode rather than assume that\n> +\t * the \"Normal\" modes are present in tuning data.\n> +\t */\n> +\tcontext.activeState.agc.constraintMode = controls::ConstraintNormal;\n\nThis sounds crash-prone if the tuning data file doesn't have a normal\nmode. Some more work is needed to define what tuning files need to\nprovide.\n\n> +\tcontext.activeState.agc.exposureMode = controls::ExposureNormal;\n> +\n> +\tfor (auto &[id, helper] : exposureModeHelpers()) {\n> +\t\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> +\t\thelper->configure(minShutterSpeed_, maxShutterSpeed_,\n> +\t\t\t\t minAnalogueGain_, maxAnalogueGain_);\n> +\t}\n\nCan you move this loop to the base class ?\n\n> +\n> \treturn 0;\n> }\n> \n> @@ -280,11 +324,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \tLOG(IPU3Agc, Debug) << \"Divided up shutter and gain are \"\n> \t\t\t << shutterTime << \" and \"\n> \t\t\t << stepGain;\n> -\n> -\tIPAActiveState &activeState = context.activeState;\n> -\t/* Update the estimated exposure and gain. */\n> -\tactiveState.agc.exposure = shutterTime / configuration.sensor.lineDuration;\n> -\tactiveState.agc.gain = stepGain;\n> }\n> \n> /**\n> @@ -347,6 +386,26 @@ double Agc::estimateLuminance(IPAActiveState &activeState,\n> \treturn ySum / (grid.height * grid.width) / 255;\n> }\n> \n> +double Agc::estimateLuminance(double gain)\n> +{\n> +\tASSERT(reds_.size() == greens_.size());\n> +\tASSERT(greens_.size() == blues_.size());\n\nIf you store the values in a single vector of RGB triplets the error\nwon't be possible anymore.\n\n> +\tconst ipu3_uapi_grid_config &grid = context_->configuration.grid.bdsGrid;\n> +\tdouble redSum = 0, greenSum = 0, blueSum = 0;\n> +\n> +\tfor (unsigned int i = 0; i < reds_.size(); i++) {\n> +\t\tredSum += std::min(reds_[i] * gain, 255.0);\n> +\t\tgreenSum += std::min(greens_[i] * gain, 255.0);\n> +\t\tblueSum += std::min(blues_[i] * gain, 255.0);\n> +\t}\n> +\n> +\tdouble ySum = redSum * context_->activeState.awb.gains.red * 0.299\n> +\t\t+ greenSum * context_->activeState.awb.gains.green * 0.587\n> +\t\t+ blueSum * context_->activeState.awb.gains.blue * 0.114;\n\nAlign the + with the =.\n\n> +\n> +\treturn ySum / (grid.height * grid.width) / 255;\n> +}\n> +\n> /**\n> * \\brief Process IPU3 statistics, and run AGC operations\n> * \\param[in] context The shared IPA context\n> @@ -399,8 +458,33 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> \n> +\tparseStatistics(stats, context.configuration.grid.bdsGrid);\n> +\n> +\t/*\n> +\t * The Agc algorithm needs to know the effective exposure value that was\n> +\t * applied to the sensor when the statistics were collected.\n> +\t */\n> \tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> \t\t\t\t * frameContext.sensor.exposure;\n> +\tdouble analogueGain = frameContext.sensor.gain;\n> +\tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> +\n> +\tutils::Duration shutterTime;\n> +\tdouble aGain, dGain;\n> +\tstd::tie(shutterTime, aGain, dGain) =\n> +\t\tcalculateNewEv(context.activeState.agc.constraintMode,\n> +\t\t\t context.activeState.agc.exposureMode, hist_,\n> +\t\t\t effectiveExposureValue);\n> +\n> +\tLOG(IPU3Agc, 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.exposure = shutterTime / context.configuration.sensor.lineDuration;\n> +\tactiveState.agc.gain = aGain;\n> +\n> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 7ed0ef7a..8405da9d 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -13,6 +13,7 @@\n> \n> #include \n> \n> +#include \"libipa/agc.h\"\n> #include \"libipa/histogram.h\"\n> \n> #include \"algorithm.h\"\n> @@ -23,12 +24,13 @@ struct IPACameraSensorInfo;\n> \n> namespace ipa::ipu3::algorithms {\n> \n> -class Agc : public Algorithm\n> +class Agc : public Algorithm, public MeanLuminanceAgc\n> {\n> public:\n> \tAgc();\n> \t~Agc() = default;\n> \n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> @@ -45,6 +47,7 @@ private:\n> \t\t\t\t const ipu3_uapi_grid_config &grid,\n> \t\t\t\t const ipu3_uapi_stats_3a *stats,\n> \t\t\t\t double gain);\n> +\tdouble estimateLuminance(double gain) override;\n\nThis function should be private in the base class.\n\n> \tvoid parseStatistics(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> \n> @@ -59,6 +62,7 @@ private:\n> \tutils::Duration filteredExposure_;\n> \n> \tuint32_t stride_;\n> +\tIPAContext *context_;\n\nNo please. The context is meant to be passed to the algorithm. Don't\nretain a pointer. You can store a copy of the grid size, the same way we\nalready have a copy of the stride.\n\n> \tstd::vector reds_;\n> \tstd::vector blues_;\n> \tstd::vector greens_;\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 959f314f..c4fb5642 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -47,6 +47,9 @@ namespace libcamera::ipa::ipu3 {\n> *\n> * \\var IPAContext::activeState\n> * \\brief The current state of IPA algorithms\n> + *\n> + * \\var IPAContext::ctrlMap\n> + * \\brief A ControlInfoMap::Map of controls populated by the algorithms\n> */\n> \n> /**\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index e9a3863b..a92cb6ce 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -12,6 +12,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -55,6 +56,8 @@ struct IPAActiveState {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> +\t\tuint32_t constraintMode;\n> +\t\tuint32_t exposureMode;\n> \t} agc;\n> \n> \tstruct {\n> @@ -85,6 +88,8 @@ struct IPAContext {\n> \tIPAActiveState activeState;\n> \n> \tFCQueue frameContexts;\n> +\n> +\tControlInfoMap::Map ctrlMap;\n\nPopulated but never used, AFAICT.\n\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 08ee6eb3..2fcc0c91 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -189,7 +189,7 @@ private:\n> };\n> \n> IPAIPU3::IPAIPU3()\n> -\t: context_({ {}, {}, { kMaxFrameContexts } })\n> +\t: context_({ {}, {}, { kMaxFrameContexts }, {} })\n> {\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5E821BD16B\n\tfor ;\n\tSat, 6 Apr 2024 01:31:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76AE163354;\n\tSat, 6 Apr 2024 03:31:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A65016333E\n\tfor ;\n\tSat, 6 Apr 2024 03:31:49 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 385EA231;\n\tSat, 6 Apr 2024 03:31:10 +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=\"RcUc0AF0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712367070;\n\tbh=tZ7L/xcIpBP4vkajQDqeH5ZcI5L6GCV5q+MZHeciPwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RcUc0AF0F+h9wvmxdVbPcx73SKlE321K2aHIIIpGBTVioCG1RzbDHwrGLzLRR4MD1\n\t0RcagGcdnMdxZ6OeB03jy5h3BuyTNAhq+KPuB0Tnf6jX3QRawVIYuqVCoOkVBTi4Dm\n\tKmJIMpGFcJ4eSwD02vvBnbPt0dgAK6hjjs0LkVFY=","Date":"Sat, 6 Apr 2024 04:31:38 +0300","From":"Laurent Pinchart ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 06/10] ipa: ipu3: Derive ipu3::algorithms::Agc from\n\tMeanLuminanceAgc","Message-ID":"<20240406013138.GM12507@pendragon.ideasonboard.com>","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-7-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240322131451.3092931-7-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\" "}}]