[{"id":27713,"web_url":"https://patchwork.libcamera.org/comment/27713/","msgid":"","date":"2023-08-30T08:28:20","subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: rpi: agc: Use channel\n\tconstraints in the AGC algorithm","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Aug 23, 2023 at 01:09:15PM +0100, David Plowman via libcamera-devel wrote:\n> Whenever we run Agc::process(), we store the most recent total\n> exposure requested for each channel.\n>\n> With these values we can apply the channel constraints after\n> time-filtering the requested total exposure, but before working out\n> how much digital gain is needed.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> src/ipa/rpi/controller/rpi/agc.cpp | 21 ++++--\n> src/ipa/rpi/controller/rpi/agc.h | 1 +\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 75 +++++++++++++++++++---\n> src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> 4 files changed, 88 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index 7e627bba..3c730d4c 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -40,6 +40,7 @@ int Agc::read(const libcamera::YamlObject ¶ms)\n> \t */\n> \tif (!params.contains(\"channels\")) {\n> \t\tLOG(RPiAgc, Debug) << \"Single channel only\";\n> +\t\tchannelTotalExposures_.resize(1, 0s);\n> \t\tchannelData_.emplace_back();\n> \t\treturn channelData_.back().channel.read(params, getHardwareConfig());\n> \t}\n> @@ -59,6 +60,8 @@ int Agc::read(const libcamera::YamlObject ¶ms)\n> \t\treturn -1;\n> \t}\n>\n> +\tchannelTotalExposures_.resize(channelData_.size(), 0s);\n> +\n> \treturn 0;\n> }\n>\n> @@ -234,15 +237,21 @@ static void getChannelIndex(Metadata *metadata, const char *message, unsigned in\n> \t\tLOG(RPiAgc, Debug) << message;\n> }\n>\n> -static void setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n> +static libcamera::utils::Duration\n> +setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n\nThis function does a few different things.. but hey, you maintain this\ncode so...\n\n> {\n> \tstd::unique_lock lock(*metadata);\n> \tAgcStatus *status = metadata->getLocked(\"agc.status\");\n> -\tif (status)\n> +\tlibcamera::utils::Duration dur = 0s;\n> +\n> +\tif (status) {\n> \t\tstatus->channel = channelIndex;\n> -\telse\n> +\t\tdur = status->totalExposureValue;\n> +\t} else\n> \t\t/* This does happen at startup, otherwise it would be a Warning or Error. */\n> \t\tLOG(RPiAgc, Debug) << message;\n> +\n> +\treturn dur;\n> }\n>\n> void Agc::prepare(Metadata *imageMetadata)\n> @@ -306,8 +315,10 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> \t\t/* Can also happen when new channels start. */\n> \t\tLOG(RPiAgc, Debug) << \"process: channel \" << channelIndex << \" not seen yet\";\n>\n> -\tchannelData.channel.process(stats, &deviceStatus, imageMetadata);\n> -\tsetChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> +\tchannelData.channel.process(stats, &deviceStatus, imageMetadata, channelTotalExposures_);\n> +\tauto dur = setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> +\tif (dur)\n> +\t\tchannelTotalExposures_[channelIndex] = dur;\n>\n> \t/* And onto the next channel for the next call. */\n> \tindex_ = (index_ + 1) % activeChannels_.size();\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index 2eed2bab..e301c5cb 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -54,6 +54,7 @@ private:\n> \tstd::vector channelData_;\n> \tstd::vector activeChannels_;\n> \tunsigned int index_; /* index into the activeChannels_ */\n> +\tAgcChannelTotalExposures channelTotalExposures_;\n> };\n>\n> } /* namespace RPiController */\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index 44198c2c..54fd08e8 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -175,6 +175,11 @@ readConstraintModes(std::map &constraintModes,\n>\n> int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)\n> {\n> +\tauto channelValue = params[\"channel\"].get();\n> +\tif (!channelValue)\n> +\t\treturn -EINVAL;\n\nmalformed config needs a message ?\n\n> +\tchannel = *channelValue;\n> +\n\nShould this be part of the previous patch ?\n\n> \tstd::string boundString = params[\"bound\"].get(\"\");\n> \ttransform(boundString.begin(), boundString.end(),\n> \t\t boundString.begin(), ::toupper);\n> @@ -184,10 +189,10 @@ int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)\n> \t}\n> \tbound = boundString == \"UPPER\" ? Bound::UPPER : Bound::LOWER;\n>\n> -\tauto value = params[\"factor\"].get();\n> -\tif (!value)\n> +\tauto factorValue = params[\"factor\"].get();\n> +\tif (!factorValue)\n> \t\treturn -EINVAL;\n> -\tfactor = *value;\n> +\tfactor = *factorValue;\n\nMaybe squash in the previous one ?\n\n>\n> \treturn 0;\n> }\n> @@ -231,6 +236,7 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> \t\tret = readChannelConstraints(channelConstraints, params[\"channel_constraints\"]);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> +\t\tLOG(RPiAgc, Info) << \"Read \" << channelConstraints.size() << \" channel constraints\";\n\nditto\n\n> \t}\n>\n> \tret = yTarget.read(params[\"y_target\"]);\n> @@ -489,7 +495,9 @@ void AgcChannel::prepare(Metadata *imageMetadata)\n> \t}\n> }\n>\n> -void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata)\n> +void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus,\n> +\t\t\t Metadata *imageMetadata,\n> +\t\t\t const AgcChannelTotalExposures &channelTotalExposures)\n> {\n> \tframeCount_++;\n> \t/*\n> @@ -508,12 +516,17 @@ void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus,\n> \tcomputeTargetExposure(gain);\n> \t/* The results have to be filtered so as not to change too rapidly. */\n> \tfilterExposure();\n> +\t/*\n> +\t * We may be asked to limit the exposure using other channels. If another channel\n> +\t * determines our upper bound we may want to know this later.\n> +\t */\n> +\tbool channelBound = applyChannelConstraints(channelTotalExposures);\n> \t/*\n> \t * Some of the exposure has to be applied as digital gain, so work out\n> -\t * what that is. This function also tells us whether it's decided to\n> -\t * \"desaturate\" the image more quickly.\n> +\t * what that is. It also tells us whether it's trying to desaturate the image\n> +\t * more quickly, which can only happen when another channel is not limiting us.\n> \t */\n> -\tbool desaturate = applyDigitalGain(gain, targetY);\n> +\tbool desaturate = applyDigitalGain(gain, targetY, channelBound);\n> \t/*\n> \t * The last thing is to divide up the exposure value into a shutter time\n> \t * and analogue gain, according to the current exposure mode.\n> @@ -792,7 +805,49 @@ void AgcChannel::computeTargetExposure(double gain)\n> \tLOG(RPiAgc, Debug) << \"Target totalExposure \" << target_.totalExposure;\n> }\n>\n> -bool AgcChannel::applyDigitalGain(double gain, double targetY)\n> +bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures)\n> +{\n> +\tbool channelBound = false;\n> +\tLOG(RPiAgc, Debug)\n> +\t\t<< \"Total exposure before channel constraints \" << filtered_.totalExposure;\n> +\n> +\tfor (const auto &constraint : config_.channelConstraints) {\n\nWhat's the logic here ? We are operating on one channel, how many\nconstraints can we have for a single channel ? Why are we iterating\nconfig_.channelConstraints, shouldn't we consider only constraints for\nthis channel ?\n\n> +\t\tLOG(RPiAgc, Debug)\n> +\t\t\t<< \"Check constraint: channel \" << constraint.channel << \" bound \"\n> +\t\t\t<< (constraint.bound == AgcChannelConstraint::Bound::UPPER ? \"UPPER\" : \"LOWER\")\n> +\t\t\t<< \" factor \" << constraint.factor;\n> +\t\tif (constraint.channel >= channelTotalExposures.size() ||\n> +\t\t !channelTotalExposures[constraint.channel]) {\n> +\t\t\tLOG(RPiAgc, Debug) << \"no such channel - skipped\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (channelTotalExposures[constraint.channel] == 0s) {\n\nisn't this \"!channelTotalExposures[constraint.channel]\" tested in the\nprevious if () ?\n\n\n> +\t\t\tLOG(RPiAgc, Debug) << \"zero exposure - skipped\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tlibcamera::utils::Duration limitExposure =\n> +\t\t\tchannelTotalExposures[constraint.channel] * constraint.factor;\n> +\t\tLOG(RPiAgc, Debug) << \"Limit exposure \" << limitExposure;\n> +\t\tif ((constraint.bound == AgcChannelConstraint::Bound::UPPER &&\n> +\t\t filtered_.totalExposure > limitExposure) ||\n> +\t\t (constraint.bound == AgcChannelConstraint::Bound::LOWER &&\n> +\t\t filtered_.totalExposure < limitExposure)) {\n> +\t\t\tfiltered_.totalExposure = limitExposure;\n> +\t\t\tLOG(RPiAgc, Debug) << \"Applies\";\n\nWhat applies ? :)\n\n> +\t\t\tchannelBound = true;\n> +\t\t} else\n> +\t\t\tLOG(RPiAgc, Debug) << \"Does not apply\";\n\nand what doesn't apply ? :)\n\n> +\t}\n> +\n> +\tLOG(RPiAgc, Debug)\n> +\t\t<< \"Total exposure after channel constraints \" << filtered_.totalExposure;\n> +\n> +\treturn channelBound;\n> +}\n> +\n> +bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound)\n> {\n> \tdouble minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> \tASSERT(minColourGain != 0.0);\n> @@ -812,8 +867,8 @@ bool AgcChannel::applyDigitalGain(double gain, double targetY)\n> \t * quickly (and we then approach the correct value more quickly from\n> \t * below).\n> \t */\n> -\tbool desaturate = targetY > config_.fastReduceThreshold &&\n> -\t\t\t gain < sqrt(targetY);\n> +\tbool desaturate = !channelBound &&\n> +\t\t\t targetY > config_.fastReduceThreshold && gain < sqrt(targetY);\n> \tif (desaturate)\n> \t\tdg /= config_.fastReduceThreshold;\n> \tLOG(RPiAgc, Debug) << \"Digital gain \" << dg << \" desaturate? \" << desaturate;\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> index 446125ef..c3f701eb 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> @@ -19,6 +19,8 @@\n>\n> namespace RPiController {\n>\n> +using AgcChannelTotalExposures = std::vector;\n> +\n> struct AgcMeteringMode {\n> \tstd::vector weights;\n> \tint read(const libcamera::YamlObject ¶ms);\n> @@ -93,7 +95,8 @@ public:\n> \tvoid disableAuto();\n> \tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata);\n> \tvoid prepare(Metadata *imageMetadata);\n> -\tvoid process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata);\n> +\tvoid process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata,\n> +\t\t const AgcChannelTotalExposures &channelTotalExposures);\n>\n> private:\n> \tbool updateLockStatus(DeviceStatus const &deviceStatus);\n> @@ -105,7 +108,8 @@ private:\n> \t\t\t double &gain, double &targetY);\n> \tvoid computeTargetExposure(double gain);\n> \tvoid filterExposure();\n> -\tbool applyDigitalGain(double gain, double targetY);\n> +\tbool applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures);\n> +\tbool applyDigitalGain(double gain, double targetY, bool channelBound);\n> \tvoid divideUpExposure();\n> \tvoid writeAndFinish(Metadata *imageMetadata, bool desaturate);\n> \tlibcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);\n> --\n> 2.30.2\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 1EB2EC0F1B\n\tfor ;\n\tWed, 30 Aug 2023 08:28:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47698627E0;\n\tWed, 30 Aug 2023 10:28:25 +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 D733061E00\n\tfor ;\n\tWed, 30 Aug 2023 10:28:23 +0200 (CEST)","from ideasonboard.com (mob-5-90-48-184.net.vodafone.it\n\t[5.90.48.184])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 41EFC814;\n\tWed, 30 Aug 2023 10:27:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693384105;\n\tbh=nToZAd7x14c/aaeL3NFQz/CI0dfeqNqBvxEbfuOzOrA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yllEQLPyeuNYjKKs80dox/JfC/eAPFfdJgNErCto7H49P6sHRTEe0TkbP/7YPPCe1\n\t7/roKDFo0d46uA9oQ6c+PE94YiWgOSlDKhkPzP5pkhHu9zFDUsc6LDGJIsq0hZNCXm\n\t46tQR81gYUnE+vP0PFD9j/jMBjcJElhc+dQn1h8ByXz2vWqnDZ/Zrwnef2ydp8gWQy\n\tCwXro0kkpp6mfsXllVE83XlcG35H5yoBL1vKMmGcjNqqcZB6giTJtDGaRsTLoZU24I\n\tj5uI7Uzpp5bYg0rdzHv0LKCT86AuqjWoEfVrKxekoqsEvvZ/ODl/HgGW/QV2ylStMi\n\tU1H+RULjJZCTQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693384021;\n\tbh=nToZAd7x14c/aaeL3NFQz/CI0dfeqNqBvxEbfuOzOrA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RD447PrWS86YKTMElJg+LYYFUS5XR+F6Ts907++BHfZ2X2Uarso4z3I0nuqn9g1ew\n\tGAOrQGfgD8ZBSBChnY8vgBGWnrLRaGkAZLhROlf0dFvb2DcI3i/X+9jQOnYVVZ8GXi\n\t+KHcquraXqGVLtUkVFu9k+X5Aclfa0laQMbz6ROo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RD447PrW\"; dkim-atps=neutral","Date":"Wed, 30 Aug 2023 10:28:20 +0200","To":"David Plowman ","Message-ID":"","References":"<20230823120915.18621-1-david.plowman@raspberrypi.com>\n\t<20230823120915.18621-6-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230823120915.18621-6-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: rpi: agc: Use channel\n\tconstraints in the AGC algorithm","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27753,"web_url":"https://patchwork.libcamera.org/comment/27753/","msgid":"","date":"2023-09-11T15:22:53","subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: rpi: agc: Use channel\n\tconstraints in the AGC algorithm","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the review!\n\nOn Wed, 30 Aug 2023 at 09:28, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Wed, Aug 23, 2023 at 01:09:15PM +0100, David Plowman via libcamera-devel wrote:\n> > Whenever we run Agc::process(), we store the most recent total\n> > exposure requested for each channel.\n> >\n> > With these values we can apply the channel constraints after\n> > time-filtering the requested total exposure, but before working out\n> > how much digital gain is needed.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > src/ipa/rpi/controller/rpi/agc.cpp | 21 ++++--\n> > src/ipa/rpi/controller/rpi/agc.h | 1 +\n> > src/ipa/rpi/controller/rpi/agc_channel.cpp | 75 +++++++++++++++++++---\n> > src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> > 4 files changed, 88 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> > index 7e627bba..3c730d4c 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> > @@ -40,6 +40,7 @@ int Agc::read(const libcamera::YamlObject ¶ms)\n> > */\n> > if (!params.contains(\"channels\")) {\n> > LOG(RPiAgc, Debug) << \"Single channel only\";\n> > + channelTotalExposures_.resize(1, 0s);\n> > channelData_.emplace_back();\n> > return channelData_.back().channel.read(params, getHardwareConfig());\n> > }\n> > @@ -59,6 +60,8 @@ int Agc::read(const libcamera::YamlObject ¶ms)\n> > return -1;\n> > }\n> >\n> > + channelTotalExposures_.resize(channelData_.size(), 0s);\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -234,15 +237,21 @@ static void getChannelIndex(Metadata *metadata, const char *message, unsigned in\n> > LOG(RPiAgc, Debug) << message;\n> > }\n> >\n> > -static void setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n> > +static libcamera::utils::Duration\n> > +setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n>\n> This function does a few different things.. but hey, you maintain this\n> code so...\n>\n> > {\n> > std::unique_lock lock(*metadata);\n> > AgcStatus *status = metadata->getLocked(\"agc.status\");\n> > - if (status)\n> > + libcamera::utils::Duration dur = 0s;\n> > +\n> > + if (status) {\n> > status->channel = channelIndex;\n> > - else\n> > + dur = status->totalExposureValue;\n> > + } else\n> > /* This does happen at startup, otherwise it would be a Warning or Error. */\n> > LOG(RPiAgc, Debug) << message;\n> > +\n> > + return dur;\n> > }\n> >\n> > void Agc::prepare(Metadata *imageMetadata)\n> > @@ -306,8 +315,10 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> > /* Can also happen when new channels start. */\n> > LOG(RPiAgc, Debug) << \"process: channel \" << channelIndex << \" not seen yet\";\n> >\n> > - channelData.channel.process(stats, &deviceStatus, imageMetadata);\n> > - setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> > + channelData.channel.process(stats, &deviceStatus, imageMetadata, channelTotalExposures_);\n> > + auto dur = setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> > + if (dur)\n> > + channelTotalExposures_[channelIndex] = dur;\n> >\n> > /* And onto the next channel for the next call. */\n> > index_ = (index_ + 1) % activeChannels_.size();\n> > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> > index 2eed2bab..e301c5cb 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc.h\n> > +++ b/src/ipa/rpi/controller/rpi/agc.h\n> > @@ -54,6 +54,7 @@ private:\n> > std::vector channelData_;\n> > std::vector activeChannels_;\n> > unsigned int index_; /* index into the activeChannels_ */\n> > + AgcChannelTotalExposures channelTotalExposures_;\n> > };\n> >\n> > } /* namespace RPiController */\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > index 44198c2c..54fd08e8 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > @@ -175,6 +175,11 @@ readConstraintModes(std::map &constraintModes,\n> >\n> > int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)\n> > {\n> > + auto channelValue = params[\"channel\"].get();\n> > + if (!channelValue)\n> > + return -EINVAL;\n>\n> malformed config needs a message ?\n\nYes, agree.\n\n>\n> > + channel = *channelValue;\n> > +\n>\n> Should this be part of the previous patch ?\n\nIndeed, will move it.\n\n>\n> > std::string boundString = params[\"bound\"].get(\"\");\n> > transform(boundString.begin(), boundString.end(),\n> > boundString.begin(), ::toupper);\n> > @@ -184,10 +189,10 @@ int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)\n> > }\n> > bound = boundString == \"UPPER\" ? Bound::UPPER : Bound::LOWER;\n> >\n> > - auto value = params[\"factor\"].get();\n> > - if (!value)\n> > + auto factorValue = params[\"factor\"].get();\n> > + if (!factorValue)\n> > return -EINVAL;\n> > - factor = *value;\n> > + factor = *factorValue;\n>\n> Maybe squash in the previous one ?\n\nYes.\n\n>\n> >\n> > return 0;\n> > }\n> > @@ -231,6 +236,7 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> > ret = readChannelConstraints(channelConstraints, params[\"channel_constraints\"]);\n> > if (ret)\n> > return ret;\n> > + LOG(RPiAgc, Info) << \"Read \" << channelConstraints.size() << \" channel constraints\";\n>\n> ditto\n\nAgree. Or perhaps I should remove it altogether, none of the others\nprint out how many \"things\" they read...\n\n>\n> > }\n> >\n> > ret = yTarget.read(params[\"y_target\"]);\n> > @@ -489,7 +495,9 @@ void AgcChannel::prepare(Metadata *imageMetadata)\n> > }\n> > }\n> >\n> > -void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata)\n> > +void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus,\n> > + Metadata *imageMetadata,\n> > + const AgcChannelTotalExposures &channelTotalExposures)\n> > {\n> > frameCount_++;\n> > /*\n> > @@ -508,12 +516,17 @@ void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus,\n> > computeTargetExposure(gain);\n> > /* The results have to be filtered so as not to change too rapidly. */\n> > filterExposure();\n> > + /*\n> > + * We may be asked to limit the exposure using other channels. If another channel\n> > + * determines our upper bound we may want to know this later.\n> > + */\n> > + bool channelBound = applyChannelConstraints(channelTotalExposures);\n> > /*\n> > * Some of the exposure has to be applied as digital gain, so work out\n> > - * what that is. This function also tells us whether it's decided to\n> > - * \"desaturate\" the image more quickly.\n> > + * what that is. It also tells us whether it's trying to desaturate the image\n> > + * more quickly, which can only happen when another channel is not limiting us.\n> > */\n> > - bool desaturate = applyDigitalGain(gain, targetY);\n> > + bool desaturate = applyDigitalGain(gain, targetY, channelBound);\n> > /*\n> > * The last thing is to divide up the exposure value into a shutter time\n> > * and analogue gain, according to the current exposure mode.\n> > @@ -792,7 +805,49 @@ void AgcChannel::computeTargetExposure(double gain)\n> > LOG(RPiAgc, Debug) << \"Target totalExposure \" << target_.totalExposure;\n> > }\n> >\n> > -bool AgcChannel::applyDigitalGain(double gain, double targetY)\n> > +bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures)\n> > +{\n> > + bool channelBound = false;\n> > + LOG(RPiAgc, Debug)\n> > + << \"Total exposure before channel constraints \" << filtered_.totalExposure;\n> > +\n> > + for (const auto &constraint : config_.channelConstraints) {\n>\n> What's the logic here ? We are operating on one channel, how many\n> constraints can we have for a single channel ? Why are we iterating\n> config_.channelConstraints, shouldn't we consider only constraints for\n> this channel ?\n\nSo these are constraints where other AGC channels constrain this\nchannel. For example, this might be channel 2, and we might have one\nconstraint that says \"don't be more than 8x the exposure of channel 0\"\nand another that says \"you must less than half the exposure of channel\n1\".\n\nObviously I don't have any specific requirement for anything as\ncomplex as that myself, but it's infrastructure that folks can use if\nthey are very advanced!\n\n>\n> > + LOG(RPiAgc, Debug)\n> > + << \"Check constraint: channel \" << constraint.channel << \" bound \"\n> > + << (constraint.bound == AgcChannelConstraint::Bound::UPPER ? \"UPPER\" : \"LOWER\")\n> > + << \" factor \" << constraint.factor;\n> > + if (constraint.channel >= channelTotalExposures.size() ||\n> > + !channelTotalExposures[constraint.channel]) {\n> > + LOG(RPiAgc, Debug) << \"no such channel - skipped\";\n> > + continue;\n> > + }\n> > +\n> > + if (channelTotalExposures[constraint.channel] == 0s) {\n>\n> isn't this \"!channelTotalExposures[constraint.channel]\" tested in the\n> previous if () ?\n\nYou are right - well spotted!!\n\n>\n>\n> > + LOG(RPiAgc, Debug) << \"zero exposure - skipped\";\n> > + continue;\n> > + }\n> > +\n> > + libcamera::utils::Duration limitExposure =\n> > + channelTotalExposures[constraint.channel] * constraint.factor;\n> > + LOG(RPiAgc, Debug) << \"Limit exposure \" << limitExposure;\n> > + if ((constraint.bound == AgcChannelConstraint::Bound::UPPER &&\n> > + filtered_.totalExposure > limitExposure) ||\n> > + (constraint.bound == AgcChannelConstraint::Bound::LOWER &&\n> > + filtered_.totalExposure < limitExposure)) {\n> > + filtered_.totalExposure = limitExposure;\n> > + LOG(RPiAgc, Debug) << \"Applies\";\n>\n> What applies ? :)\n\n\"This constraint\", of course! :)\n\nBut I'll improve the message!\n\n>\n> > + channelBound = true;\n> > + } else\n> > + LOG(RPiAgc, Debug) << \"Does not apply\";\n>\n> and what doesn't apply ? :)\n\nAs above. Will improve the message.\n\nThanks again for all the reviewing, it's very helpful.\n\nDavid\n\n>\n> > + }\n> > +\n> > + LOG(RPiAgc, Debug)\n> > + << \"Total exposure after channel constraints \" << filtered_.totalExposure;\n> > +\n> > + return channelBound;\n> > +}\n> > +\n> > +bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound)\n> > {\n> > double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });\n> > ASSERT(minColourGain != 0.0);\n> > @@ -812,8 +867,8 @@ bool AgcChannel::applyDigitalGain(double gain, double targetY)\n> > * quickly (and we then approach the correct value more quickly from\n> > * below).\n> > */\n> > - bool desaturate = targetY > config_.fastReduceThreshold &&\n> > - gain < sqrt(targetY);\n> > + bool desaturate = !channelBound &&\n> > + targetY > config_.fastReduceThreshold && gain < sqrt(targetY);\n> > if (desaturate)\n> > dg /= config_.fastReduceThreshold;\n> > LOG(RPiAgc, Debug) << \"Digital gain \" << dg << \" desaturate? \" << desaturate;\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> > index 446125ef..c3f701eb 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> > @@ -19,6 +19,8 @@\n> >\n> > namespace RPiController {\n> >\n> > +using AgcChannelTotalExposures = std::vector;\n> > +\n> > struct AgcMeteringMode {\n> > std::vector weights;\n> > int read(const libcamera::YamlObject ¶ms);\n> > @@ -93,7 +95,8 @@ public:\n> > void disableAuto();\n> > void switchMode(CameraMode const &cameraMode, Metadata *metadata);\n> > void prepare(Metadata *imageMetadata);\n> > - void process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata);\n> > + void process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata,\n> > + const AgcChannelTotalExposures &channelTotalExposures);\n> >\n> > private:\n> > bool updateLockStatus(DeviceStatus const &deviceStatus);\n> > @@ -105,7 +108,8 @@ private:\n> > double &gain, double &targetY);\n> > void computeTargetExposure(double gain);\n> > void filterExposure();\n> > - bool applyDigitalGain(double gain, double targetY);\n> > + bool applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures);\n> > + bool applyDigitalGain(double gain, double targetY, bool channelBound);\n> > void divideUpExposure();\n> > void writeAndFinish(Metadata *imageMetadata, bool desaturate);\n> > libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);\n> > --\n> > 2.30.2\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 4BC1EBD160\n\tfor ;\n\tMon, 11 Sep 2023 15:23:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83A12628F2;\n\tMon, 11 Sep 2023 17:23:07 +0200 (CEST)","from mail-oo1-xc29.google.com (mail-oo1-xc29.google.com\n\t[IPv6:2607:f8b0:4864:20::c29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C71E461DF0\n\tfor ;\n\tMon, 11 Sep 2023 17:23:05 +0200 (CEST)","by mail-oo1-xc29.google.com with SMTP id\n\t006d021491bc7-57358a689d2so3041458eaf.2\n\tfor ;\n\tMon, 11 Sep 2023 08:23:05 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694445787;\n\tbh=dJH3RBDcQXBB3+WYQfHuhnYY3E1Wk8sLEd9QN/K58t0=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Xw7ytpC1BG6Gle/cGc4c51v5hFApkNvYzaxtkr/w47Hi6FVzk5IfhdNHY5SgtfnM7\n\tzzGntfIgZVyd0W6PvzwwAsCRGvUZJLyuVJHgyFXEL1xDijHLfYfq2HZY0C4CfHg2EC\n\tL/0zCgZiqTFJinbnTDPjweG3WRnSRhGchfUwfDa7hbjkvj5hDtN8RZK6ML2FkfSMPj\n\tRK2cCFl2tg+whzrncFC+HOLaqbYPcAsoRHifFoetuuUk1gbc9RikU7ftIFvQ22pD5G\n\tbGEPM2G1ZpZ0Y8KQh8CCo+wgm0iJ27fxcFsKsf3USGRsEZuapbXoV4jUGxnMDV05/E\n\tlOCtzRkfyB8qg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1694445784; x=1695050584;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Y0iE7tpc/JiP06wPhK7A+yVq9J6J1w3X+9wDbRC3JIQ=;\n\tb=pXMnSZnXy43uYGcEpCOVu1fERaYWsWQxl/nuabsyar/sX/W4yp+370dNDMATyuzKpa\n\tsitUVtwAoJj+AF7Fr5LJra19GBeFOCaY+aLr1FxIisV38DP8FNp/QDx0W7CtS9f/g31b\n\tCFXdrZYselSrsUuywxX+9wR8Szu5Ayh3rGt9OTAWI0EbFoENCiTi3aYqQiKFJ2Ny6V0F\n\tuP+g5tNY/pQVnknzbDDP9Hda2M3ToHqCulhyeKqDI1g/T8Y8shtkGSWiPljORTMqLJi8\n\tvdoPeomvzyq9dG0+2g4srpMsjRHuHHV8QgFL/OnKUUEvVjW1NaEW/GHvYQ0z+1r/hqL6\n\tvT8g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"pXMnSZnX\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1694445784; x=1695050584;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Y0iE7tpc/JiP06wPhK7A+yVq9J6J1w3X+9wDbRC3JIQ=;\n\tb=xQIxjM8zztyf/ueQJBKj+mdDj92+DiBwTSQWHOkZhm+p4tdy/Ea5MmjNOSeBBEgBm9\n\tIQ/646JFTs/s/LyttjnZYhtz6M5rueULVL4LLlST1kMgpqqg4cI8nyHsR6HrLFhqTtsL\n\tDHcLEg8FzncqJVZRHRbNuPYuIDJkjE/nUQnYCUECZ/M/62rq2H9L+UHlcbY5YNY65IZm\n\t6qraBM5QXywp2ygZsQf7vKeMKPpf8+My6C05I51TKX7/Zlg7RiB//YVMdqoBbSa2YL2V\n\tRtieChmiE0j0BS8ef6iVKzxRMlgzoMK/NTGNl2AZrLn4MW+RfmqXrYEKgqUnG2YIYnS2\n\thjOg==","X-Gm-Message-State":"AOJu0YyFDoCFYqfd5Rs68RBUFBjUunBoLAKJ9u9FgvQjeasz3hvUBOBI\n\tTCpwOZ2MGtat+1TsnyrrP5ZJMTbr6Tp2tHM3AFq+gA==","X-Google-Smtp-Source":"AGHT+IFosEgOm8H+OM8+Gjpp3C8obG6URrbJiT77gPaRbIzmhC53usq2aeeBRMz754d2CMt5d7bgmhLvHWADi4uRvUI=","X-Received":"by 2002:a05:6358:9494:b0:139:f5e9:4463 with SMTP id\n\ti20-20020a056358949400b00139f5e94463mr12460823rwb.2.1694445784064;\n\tMon, 11 Sep 2023 08:23:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20230823120915.18621-1-david.plowman@raspberrypi.com>\n\t<20230823120915.18621-6-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 11 Sep 2023 16:22:53 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: rpi: agc: Use channel\n\tconstraints in the AGC algorithm","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]