[{"id":27765,"web_url":"https://patchwork.libcamera.org/comment/27765/","msgid":"","date":"2023-09-12T15:21:37","subject":"Re: [libcamera-devel] [PATCH v3 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":"On Tue, Sep 12, 2023 at 11:24:42AM +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 | 22 ++++++--\n> src/ipa/rpi/controller/rpi/agc.h | 1 +\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 58 +++++++++++++++++++---\n> src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> 4 files changed, 75 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index 58ba8839..170432c9 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> @@ -236,15 +239,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> +setChannelIndexGetExposure(Metadata *metadata, const char *message, unsigned int channelIndex)\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> @@ -308,8 +317,11 @@ 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 = setChannelIndexGetExposure(imageMetadata, \"process: no AGC status found\",\n> +\t\t\t\t\t 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 ee85c693..90890439 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -55,6 +55,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 7fce35b0..4669e8da 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -493,7 +493,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> @@ -512,12 +514,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> @@ -796,7 +803,44 @@ 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> +\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 or no exposure available- skipped\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tlibcamera::utils::Duration limitExposure =\n> +\t\t\tchannelTotalExposures[constraint.channel] * constraint.factor;\n\nI'm still having troubles visualizing how this gets specified in a config file,\nspecifically what guarantees the interdepencies between channels are\ncorrect. But that's likely not a problem for now.\n\nCode-wise this is fine\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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) << \"Constraint applies\";\n> +\t\t\tchannelBound = true;\n> +\t\t} else\n> +\t\t\tLOG(RPiAgc, Debug) << \"Constraint does not apply\";\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> @@ -816,8 +860,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 5d2d8a11..1745f2ef 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> @@ -21,6 +21,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> @@ -95,7 +97,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> @@ -107,7 +110,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 A5E9BBE080\n\tfor ;\n\tTue, 12 Sep 2023 15:21:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26DAF61DF5;\n\tTue, 12 Sep 2023 17:21:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1DAE61DEF\n\tfor ;\n\tTue, 12 Sep 2023 17:21:40 +0200 (CEST)","from ideasonboard.com (mob-5-90-67-213.net.vodafone.it\n\t[5.90.67.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F571512;\n\tTue, 12 Sep 2023 17:20:09 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694532102;\n\tbh=BAUctWcSJRf52qzl7m+sJXp/ajblWyGqrlcXc1fvbRA=;\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=bg765MageRMGm9NUKwgE1Luu450VpEjUnYtt6xdGcJBimg1UBdWZFnimdh3F+KaEz\n\tEHQatdYPWeQufUF78c187/+qff6E9hpV8ZSIEO8anvW+ozgUoGzjmqAejrnuNs1Tn4\n\tw/HhsOKouXd718wZoEjt1vLWMjeXRSk2XaKecRFj8YpagDvEuM9LPLn4E81QFKRtye\n\tJDcPe/VaQTNGcUCtRTzLTaWWKbjimzuBnfsJyu2LrBu8aHSV9oWOYbEYZCJJhu8kI6\n\tlPzWEIoMjMUvS5zlNh/Qny7Cufbi8qYhPlrWvoGlX6N514PKQ7dVzE2ypT+vMu7Z5z\n\t5uQTm4ju7ShAQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694532009;\n\tbh=BAUctWcSJRf52qzl7m+sJXp/ajblWyGqrlcXc1fvbRA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UF8ebsU+ne1gSqhQLC0GdYUl6/XDrO8Mmo5QYxnUGy5gR2IAMtnSAvqq0A1VtxVay\n\tilU0fR1Vpazg6q0ghJ6zMEBL98V1PoawOIYtP9jhJK0REYJAAr9uvK60+M5jZZoCWk\n\tWycdBtp/r6e7jsssUN+la0rFac3+Mfen+Yqzh5+k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UF8ebsU+\"; dkim-atps=neutral","Date":"Tue, 12 Sep 2023 17:21:37 +0200","To":"David Plowman ","Message-ID":"","References":"<20230912102442.169001-1-david.plowman@raspberrypi.com>\n\t<20230912102442.169001-6-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230912102442.169001-6-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]