[{"id":27685,"web_url":"https://patchwork.libcamera.org/comment/27685/","msgid":"","date":"2023-08-22T13:13:29","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: rpi: agc: Use channel\n\tconstraints in the AGC algorithm","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your patch.\n\nOn Mon, 31 Jul 2023 at 10:47, David Plowman via libcamera-devel\n wrote:\n>\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> ---\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 | 76 ++++++++++++++++++----\n> src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> src/ipa/rpi/vc4/data/imx477.json | 3 +-\n> 5 files changed, 90 insertions(+), 19 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index 7e627bba..7077fbff 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> + channelResults_.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> + channelResults_.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> 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, channelResults_);\n> + auto dur = setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> + if (dur)\n> + channelResults_[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..acd6dc2f 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> + AgcChannelResults channelResults_;\n\nIt took me a while to understand what channelResults_ was.\nPerhaps we can rename this to channelTotalExposure_ to be more explicit?\n\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 ed8a3b30..4e60802c 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> + channel = *channelValue;\n> +\n> std::string boundString = params[\"bound\"].get(\"\");\n> transform(boundString.begin(), boundString.end(),\n> boundString.begin(), ::toupper);\n> @@ -184,15 +189,15 @@ 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> return 0;\n> }\n>\n> -int readChannelConstraints(std::vector channelConstraints,\n> +int readChannelConstraints(std::vector &channelConstraints,\n> const libcamera::YamlObject ¶ms)\n> {\n> int ret = 0;\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>\n> ret = yTarget.read(params[\"y_target\"]);\n> @@ -489,7 +495,8 @@ 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, const AgcChannelResults &channelResults)\n\nIt feels like channelResults/totalExposure could/should be passed in\nthe imageMetdata, but I'm not too bothered with an extra parameter.\n\n> {\n> frameCount_++;\n> /*\n> @@ -508,12 +515,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(channelResults);\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 +804,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 AgcChannelResults &channelResults)\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> + 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 >= channelResults.size() ||\n> + !channelResults[constraint.channel]) {\n> + LOG(RPiAgc, Debug) << \"no such channel - skipped\";\n> + continue;\n> + }\n> +\n> + if (channelResults[constraint.channel] == 0s) {\n> + LOG(RPiAgc, Debug) << \"zero exposure - skipped\";\n> + continue;\n> + }\n> +\n> + libcamera::utils::Duration limitExposure =\n> + channelResults[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> + channelBound = true;\n> + } else\n> + LOG(RPiAgc, Debug) << \"Does not apply\";\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 +866,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..7ce34360 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 AgcChannelResults = 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 AgcChannelResults &channelResults);\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 AgcChannelResults &channelResults);\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> diff --git a/src/ipa/rpi/vc4/data/imx477.json b/src/ipa/rpi/vc4/data/imx477.json\n> index daffc268..bbe6da0d 100644\n> --- a/src/ipa/rpi/vc4/data/imx477.json\n> +++ b/src/ipa/rpi/vc4/data/imx477.json\n> @@ -515,4 +515,5 @@\n> \"rpi.sharpen\": { }\n> }\n> ]\n> -}\n> \\ No newline at end of file\n> +}\n> +\n\nAdded by accident?\n\nOther than that:\nReviewed-by: Naushir Patuck \n\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 9EF9EBF415\n\tfor ;\n\tTue, 22 Aug 2023 13:13:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D752F627E0;\n\tTue, 22 Aug 2023 15:13:38 +0200 (CEST)","from mail-yw1-x1134.google.com (mail-yw1-x1134.google.com\n\t[IPv6:2607:f8b0:4864:20::1134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6F076055E\n\tfor ;\n\tTue, 22 Aug 2023 15:13:36 +0200 (CEST)","by mail-yw1-x1134.google.com with SMTP id\n\t00721157ae682-58d9ba95c78so51401227b3.1\n\tfor ;\n\tTue, 22 Aug 2023 06:13:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692710018;\n\tbh=ep+4WU6WlgA2QHoTyAmqvqyDMzTakUAlahKhMKr0Hp4=;\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=xJ6stXuCOQiJ/v/iRINK9YDpgolprUNN0+0gvfbvlJKIfFORLvCBt312A/Rvc9cP0\n\tXyrKIvUEqV/lrnZgXaS7xlPpPxZ5y5fPj0EPIcFU44JOoN/H5YkB9vcu6GrzZPYKPs\n\tCtki8H3HMxke6YG6DxmS18iqnFO5Z9ZGtDeUuCZ6oYmf9CU7comqeJkZHsPTnfJeCY\n\tJNtzIFSJwYtxCol3dZXcX3ZbAKFD6CmH5OIxrpa2SCsOV8oVP5zGLVBKPNAArj1N1t\n\tpwYsHckwUnd7ZEoytjE+MXjQqxlIxRpq4cjbfQZOL/AmPPjfszxGYE9o19WYBb78ns\n\t00EAwl2uaIpBw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692710015; x=1693314815;\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=RMCwUxbd4A7mz3BmZxDy1EWpdMj1d49cRxkbHtcYbKQ=;\n\tb=fE+fsQ0l3TM/+ECRwySTnHH4AWZ6NipK8qLREBqEQvhz8ZMnkqkJvOtImRcWsZokbC\n\t51hb45vkzCUbm0ELnao3WieYMaPRhNxqcJqWtoWtawYwrBh1nKhq40aqk+eaPKrHvbkL\n\t710eZpBH35YcJ3JWh3R1PsyI2TMxkSy7UI0BUeCGv49sY3op7j/48KQyAEDG8S8cE18k\n\teKogLmsKrGH1mGkRTGAFzmGAWuy408XYhMPCKi/jVYB9XiKcaOJBa3GmRr+7xWF+O1zB\n\tWQEXpioYcQp7wScZpVXpiXHZks+NQ8a1c3ufALyUa+4YZKC2JAueXxb4LCKd5JD4EpJ2\n\te4/Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"fE+fsQ0l\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692710015; x=1693314815;\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=RMCwUxbd4A7mz3BmZxDy1EWpdMj1d49cRxkbHtcYbKQ=;\n\tb=k7ztW483DrG0HoRa/TVUJ4lCrSXWHHKVGexXrwXXoTmKvdKIticXTuAa2MzqEOZOsE\n\tKHGzSew56gb3H4iTeNoUMbxBtGIr4dU62WOBkDjygjxf0Enrz/KdPXRLoylJZRxyOQMu\n\tgRKLkIl8FhMdqmjHWpEPdRdUs9rRRNOanp5C+FslY96dXJPr0mMwjskJUa35avyV2oUC\n\tuHqr29tVQwvDU80/nG9hoPXXgIM5kf9hr/aMrW8EscJuEsH7S796Y1MnLJ30SDA413GL\n\tWUYvcJKNpHwcnyIRsHuemz171j7GLC0ihVX9GXEID8yB/7wlpNCCcOm3o9GzhtEm6ThV\n\tUwdQ==","X-Gm-Message-State":"AOJu0Yy73Mof7aFcSwNPyE/zt61lBdQk+L1I6J3AvWyDcW7xfso3tvrg\n\tJfxfSCx3dL4ZUFqUGv99lPyxyzHueCj2eNXMM5TbvQ==","X-Google-Smtp-Source":"AGHT+IE/AlzKdNwFkcKanNqWMPiIMVn8p6s1T+MzVcJuburdCnJI9xkqIxHWaxWYhryOAICFLSRvzLX145iLuz4doQw=","X-Received":"by 2002:a81:5245:0:b0:583:d9de:8509 with SMTP id\n\tg66-20020a815245000000b00583d9de8509mr8851758ywb.27.1692710015582;\n\tTue, 22 Aug 2023 06:13:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731094641.73646-1-david.plowman@raspberrypi.com>\n\t<20230731094641.73646-6-david.plowman@raspberrypi.com>","In-Reply-To":"<20230731094641.73646-6-david.plowman@raspberrypi.com>","Date":"Tue, 22 Aug 2023 14:13:29 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27689,"web_url":"https://patchwork.libcamera.org/comment/27689/","msgid":"","date":"2023-08-23T09:42:38","subject":"Re: [libcamera-devel] [PATCH 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 Naush\n\nThanks for the comments.\n\nOn Tue, 22 Aug 2023 at 14:13, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for your patch.\n>\n> On Mon, 31 Jul 2023 at 10:47, David Plowman via libcamera-devel\n> wrote:\n> >\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> > ---\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 | 76 ++++++++++++++++++----\n> > src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> > src/ipa/rpi/vc4/data/imx477.json | 3 +-\n> > 5 files changed, 90 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> > index 7e627bba..7077fbff 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> > + channelResults_.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> > + channelResults_.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> > 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, channelResults_);\n> > + auto dur = setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> > + if (dur)\n> > + channelResults_[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..acd6dc2f 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> > + AgcChannelResults channelResults_;\n>\n> It took me a while to understand what channelResults_ was.\n> Perhaps we can rename this to channelTotalExposure_ to be more explicit?\n\nYes, I guess that I initially thought there might be more stuff here,\nbut given that there isn't, renaming would make it clearer!\n\n>\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 ed8a3b30..4e60802c 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> > + channel = *channelValue;\n> > +\n> > std::string boundString = params[\"bound\"].get(\"\");\n> > transform(boundString.begin(), boundString.end(),\n> > boundString.begin(), ::toupper);\n> > @@ -184,15 +189,15 @@ 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> > return 0;\n> > }\n> >\n> > -int readChannelConstraints(std::vector channelConstraints,\n> > +int readChannelConstraints(std::vector &channelConstraints,\n> > const libcamera::YamlObject ¶ms)\n> > {\n> > int ret = 0;\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> >\n> > ret = yTarget.read(params[\"y_target\"]);\n> > @@ -489,7 +495,8 @@ 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, const AgcChannelResults &channelResults)\n>\n> It feels like channelResults/totalExposure could/should be passed in\n> the imageMetdata, but I'm not too bothered with an extra parameter.\n\nI'm somewhat inclined to the notion that metadata is for storing stuff\nabout _this_ image, not really just a means of passing extra stuff\naround. Though the line does get rather blurred, I admit! So I'm\nslightly veering towards leaving this one...\n\n>\n> > {\n> > frameCount_++;\n> > /*\n> > @@ -508,12 +515,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(channelResults);\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 +804,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 AgcChannelResults &channelResults)\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> > + 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 >= channelResults.size() ||\n> > + !channelResults[constraint.channel]) {\n> > + LOG(RPiAgc, Debug) << \"no such channel - skipped\";\n> > + continue;\n> > + }\n> > +\n> > + if (channelResults[constraint.channel] == 0s) {\n> > + LOG(RPiAgc, Debug) << \"zero exposure - skipped\";\n> > + continue;\n> > + }\n> > +\n> > + libcamera::utils::Duration limitExposure =\n> > + channelResults[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> > + channelBound = true;\n> > + } else\n> > + LOG(RPiAgc, Debug) << \"Does not apply\";\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 +866,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..7ce34360 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 AgcChannelResults = 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 AgcChannelResults &channelResults);\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 AgcChannelResults &channelResults);\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> > diff --git a/src/ipa/rpi/vc4/data/imx477.json b/src/ipa/rpi/vc4/data/imx477.json\n> > index daffc268..bbe6da0d 100644\n> > --- a/src/ipa/rpi/vc4/data/imx477.json\n> > +++ b/src/ipa/rpi/vc4/data/imx477.json\n> > @@ -515,4 +515,5 @@\n> > \"rpi.sharpen\": { }\n> > }\n> > ]\n> > -}\n> > \\ No newline at end of file\n> > +}\n> > +\n>\n> Added by accident?\n\nI blame my editor! I will fight with it until it relents...\n\nThanks\nDavid\n\n>\n> Other than that:\n> Reviewed-by: Naushir Patuck \n>\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 B3E21BF415\n\tfor ;\n\tWed, 23 Aug 2023 09:42:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7A7F627E0;\n\tWed, 23 Aug 2023 11:42:51 +0200 (CEST)","from mail-qv1-xf33.google.com (mail-qv1-xf33.google.com\n\t[IPv6:2607:f8b0:4864:20::f33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E95B61E06\n\tfor ;\n\tWed, 23 Aug 2023 11:42:50 +0200 (CEST)","by mail-qv1-xf33.google.com with SMTP id\n\t6a1803df08f44-64a5f9a165eso27893396d6.1\n\tfor ;\n\tWed, 23 Aug 2023 02:42:50 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692783771;\n\tbh=ljHzTRM20Rpr59snK44C1SjML1+3Wnr0LipiTMI2VIc=;\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=KmC9tGzWivDvh6OUWlqOMBXcG7HUuQr2juMJOawKdGfa6Vj3ur+u6650Mxqpvhzk0\n\tec7dz927phyF50EvEzplgnnjVOsaLh3RYI3Q3TjN/Trh2MKq9X6LjmU9QdYeV2ilor\n\tBQ4ZX2oyIQGyvlYsEoRBsHit3G/lQEpU85JVVjFNJR5E85b8F43yq7MV1jff0JCnMp\n\tS7CWmWSbNgTFEGbIMcXsiaYWfZeCSK11w2K6gV358uOj4sF6K6EV6Rgkjc+Kd6c/WF\n\t8eessj9MuxbXOR+IRkYIvdvHE4vAUFf2+faWefPjkDsm9AuSDzA+glrTOWgquYp8Hz\n\t8HP+2/01XPEtg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692783769; x=1693388569;\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=Zm5JBHhBZIqdkF0OWgbNhf1umNUpzZbkEDe+NT1Ts5Y=;\n\tb=k3tG7rDBBhP5n8ThpHuwpqpHp/pEBjmr2DWjlFnvdZLlfR/4/7YiRYoDR+BzJyAdCF\n\tVPAHyw+TqsHFNbPDVxEY+Zx2+q+ZmRiqFyLFLyyRmUF9xhkus9OvqaQX2UVwPmlT09PX\n\tyY+PInp87KX/D5rSuX73LcRMSMXHPt6+4B+Xr9C/0EvCsWroiXgmCVgdR4pSpBD3NzVD\n\teEKTyhhf00VgUb4LyDnI6UMWZIDJaj81tW+HwNzdiTmsWjX6iIrzd2HNpf56eAafPC+u\n\twPR0vg7VfpW0ONdI8Gc6fzefua8X/BPfq4CtL7Ccgky1ZdPx9RySZ0SYqKbpbigeYe68\n\tBc/w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"k3tG7rDB\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692783769; x=1693388569;\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=Zm5JBHhBZIqdkF0OWgbNhf1umNUpzZbkEDe+NT1Ts5Y=;\n\tb=WF/b4DIBAiiq9X9d8U8ch+mHnAiip9hjTAwiZ/mfVV2QS0a0qEEaBqkeMpZ2esXSJj\n\tG12GOnET2gH0fuhYdRXPulPtCFp7bkHsdClM+ycwvsEzCY9JH+JeEG3CQjOchXA5CYvm\n\tRUMIVOn7JMqi2XN5KELV4Ux3C6JGA78cTGCu6HtKcoGqYoTmNQRXExZ5/R24ITRrrXGe\n\t8bUg998Uedci+AjDqTaITwS3YEDJ+DqTo5IixJ7qmdB0eDAoyQDW6NDQdtTa84xFV2J9\n\tyxV18OLBdd21tDotiLoP3Pf0s9rYaRz08dir/dPCIAc/n4g2w7y77E6UQPAiMcm5d7Wt\n\tVh2A==","X-Gm-Message-State":"AOJu0YzeBQ5udCoGzTI1W0grB6kMXm3EI6E38eOk4thhoVs3aQzBRMsn\n\tEvmx8/sqM5fr59It9cIKUkz/RLu1Ax3xwWEzg8xZyA==","X-Google-Smtp-Source":"AGHT+IFbPnAuBJBPzqW+NgDvcMEnTgnURE52jChdekCnNwLksBRQTmzwRsnTTnBqBRw1H9kAZTxtNLs+dtTtD6eSIIE=","X-Received":"by 2002:a0c:aa02:0:b0:643:8aee:c957 with SMTP id\n\td2-20020a0caa02000000b006438aeec957mr9351110qvb.30.1692783768891;\n\tWed, 23 Aug 2023 02:42:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731094641.73646-1-david.plowman@raspberrypi.com>\n\t<20230731094641.73646-6-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Wed, 23 Aug 2023 10:42:38 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":27691,"web_url":"https://patchwork.libcamera.org/comment/27691/","msgid":"","date":"2023-08-23T09:50:42","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: rpi: agc: Use channel\n\tconstraints in the AGC algorithm","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\n\nOn Wed, 23 Aug 2023 at 10:42, David Plowman\n wrote:\n>\n> Hi Naush\n>\n> Thanks for the comments.\n>\n> On Tue, 22 Aug 2023 at 14:13, Naushir Patuck wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for your patch.\n> >\n> > On Mon, 31 Jul 2023 at 10:47, David Plowman via libcamera-devel\n> > wrote:\n> > >\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> > > ---\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 | 76 ++++++++++++++++++----\n> > > src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> > > src/ipa/rpi/vc4/data/imx477.json | 3 +-\n> > > 5 files changed, 90 insertions(+), 19 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> > > index 7e627bba..7077fbff 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> > > + channelResults_.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> > > + channelResults_.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> > > 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, channelResults_);\n> > > + auto dur = setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> > > + if (dur)\n> > > + channelResults_[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..acd6dc2f 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> > > + AgcChannelResults channelResults_;\n> >\n> > It took me a while to understand what channelResults_ was.\n> > Perhaps we can rename this to channelTotalExposure_ to be more explicit?\n>\n> Yes, I guess that I initially thought there might be more stuff here,\n> but given that there isn't, renaming would make it clearer!\n>\n> >\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 ed8a3b30..4e60802c 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> > > + channel = *channelValue;\n> > > +\n> > > std::string boundString = params[\"bound\"].get(\"\");\n> > > transform(boundString.begin(), boundString.end(),\n> > > boundString.begin(), ::toupper);\n> > > @@ -184,15 +189,15 @@ 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> > > return 0;\n> > > }\n> > >\n> > > -int readChannelConstraints(std::vector channelConstraints,\n> > > +int readChannelConstraints(std::vector &channelConstraints,\n> > > const libcamera::YamlObject ¶ms)\n> > > {\n> > > int ret = 0;\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> > >\n> > > ret = yTarget.read(params[\"y_target\"]);\n> > > @@ -489,7 +495,8 @@ 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, const AgcChannelResults &channelResults)\n> >\n> > It feels like channelResults/totalExposure could/should be passed in\n> > the imageMetdata, but I'm not too bothered with an extra parameter.\n>\n> I'm somewhat inclined to the notion that metadata is for storing stuff\n> about _this_ image, not really just a means of passing extra stuff\n> around. Though the line does get rather blurred, I admit! So I'm\n> slightly veering towards leaving this one...\n\nSure, let's leave it as-is.\n\n>\n> >\n> > > {\n> > > frameCount_++;\n> > > /*\n> > > @@ -508,12 +515,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(channelResults);\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 +804,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 AgcChannelResults &channelResults)\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> > > + 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 >= channelResults.size() ||\n> > > + !channelResults[constraint.channel]) {\n> > > + LOG(RPiAgc, Debug) << \"no such channel - skipped\";\n> > > + continue;\n> > > + }\n> > > +\n> > > + if (channelResults[constraint.channel] == 0s) {\n> > > + LOG(RPiAgc, Debug) << \"zero exposure - skipped\";\n> > > + continue;\n> > > + }\n> > > +\n> > > + libcamera::utils::Duration limitExposure =\n> > > + channelResults[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> > > + channelBound = true;\n> > > + } else\n> > > + LOG(RPiAgc, Debug) << \"Does not apply\";\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 +866,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..7ce34360 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 AgcChannelResults = 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 AgcChannelResults &channelResults);\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 AgcChannelResults &channelResults);\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> > > diff --git a/src/ipa/rpi/vc4/data/imx477.json b/src/ipa/rpi/vc4/data/imx477.json\n> > > index daffc268..bbe6da0d 100644\n> > > --- a/src/ipa/rpi/vc4/data/imx477.json\n> > > +++ b/src/ipa/rpi/vc4/data/imx477.json\n> > > @@ -515,4 +515,5 @@\n> > > \"rpi.sharpen\": { }\n> > > }\n> > > ]\n> > > -}\n> > > \\ No newline at end of file\n> > > +}\n> > > +\n> >\n> > Added by accident?\n>\n> I blame my editor! I will fight with it until it relents...\n>\n> Thanks\n> David\n>\n> >\n> > Other than that:\n> > Reviewed-by: Naushir Patuck \n> >\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 5A961BE080\n\tfor ;\n\tWed, 23 Aug 2023 09:51:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6726627DA;\n\tWed, 23 Aug 2023 11:51:01 +0200 (CEST)","from mail-yw1-x112f.google.com (mail-yw1-x112f.google.com\n\t[IPv6:2607:f8b0:4864:20::112f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A58661F19\n\tfor ;\n\tWed, 23 Aug 2023 11:50:59 +0200 (CEST)","by mail-yw1-x112f.google.com with SMTP id\n\t00721157ae682-58d41109351so88144417b3.1\n\tfor ;\n\tWed, 23 Aug 2023 02:50:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692784261;\n\tbh=oqRQ0i4tXlwwErJbB7j0XRF8rH/RZuV2lbPwYmYUqTk=;\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=oMoSagAigb53qUugoZR1dTFj5AckBObbBJE/FU5sAB2ZKTid6iXrXtbcUskda5Mc0\n\tm3gX2cX1vkltRelhcAdYjsJw3yJHwLFS2SCylqPGkAmUfwtShfKFuL8VqomRoB9he/\n\t5tHyDFKVx9FviQK+A7kb2ukBvsgrTVWPx5/QhEMXNpWYzGwZtxQPU0Ib41sOCGuCzE\n\twHKfP/3drVz8QZFwhxPkDvssS+hNU61axcwfdIfdsOHIG5SC89tn5DqllAqqDvxUzG\n\tfYdoORbbCO1aj27Y1Ks3vsTAee2ver9+WqKo1+3cU/vNa4Ro9jrCKauNINZ2bxMw3f\n\t45WpfWeaaUW1A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692784258; x=1693389058;\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=uOQHgJvbdvqHxI4PFzmuF/YNyJehgxUAZqKiMF0t0Gc=;\n\tb=blpJb7Oace0JLe4EDPQ4Ks7b2ZDiJXxZjiH5iMixT4Ty1gUwd+ntMH2j5xMFgSgTaI\n\tYJLEnDhcgxQnYvG5rw9HVK1Og92ujNo57x6Z0yezLJmZXYGFgd8rdFs7WKOsQbFkvfLF\n\tUvhSDO3u8+P9B8MwXqW6Em4Lun4e++WWUnWCVXMTU3NzVLi19fwrZSQZXDsUtVdbvRQX\n\tYICJTWgQ2JmxRSFVJmF4GRUu0ET7pnqh7PDla4AwMOa2FYY2dIBsLFnRQ6mO1OE0DBfe\n\tcixolwTh7m46IUmZRORfwp2jI+HrttCKusbu/lR9pBjP42mACFJOqS4MEmzGCTYjpQzP\n\tiggw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"blpJb7Oa\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692784258; x=1693389058;\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=uOQHgJvbdvqHxI4PFzmuF/YNyJehgxUAZqKiMF0t0Gc=;\n\tb=fVY8Y+fgbAi9a+70Tkqit0frkrqxRsWE75XijjyzzfRmtPjQ4M0eBYUPjfTx7IqiOE\n\trbPRM8iP8upR5KxKlJtYaJPG3QyTywRpoMAeUYQ+IVDkN4Z1kR6WrLGTEyOF8+1xtTsD\n\t8CPBsXVdUjhfVc6A8C4tN4YbUewEtorfXhYxTGZzREzXkCS6tEvnrYDD/f3sYlwVb+9C\n\tAhK0xBZ8l4ZDAnAVNxJmj/5rhFPX723YfxW6RszsXFjlpzuqYR2270PQpief/qphzRTx\n\tSASaSHLIjkZuMuCZ9MIyXQqmoiJ9aNqeGAzJJj9FIcq7O7n3D6cqQ+6qUXW1yU0LfIQP\n\trcsA==","X-Gm-Message-State":"AOJu0Yz04deqyvunw0xR4yPs9RfyZ4kNgqkGA4RiAaiXB6yBVqq6G2cn\n\teIrXRTpk6oQDgC98gaoBbRLzsaSMdlKiQl5n6up42g==","X-Google-Smtp-Source":"AGHT+IEUPy0xV9Pd7wyNtlWg7ogY97AgutwSRhARRSt2Xt0SusIFlVTBOZyy0jkHwYRzxCQBaz8JT6f8YLLhAnAJBPE=","X-Received":"by 2002:a0d:e897:0:b0:589:f9c3:8b2e with SMTP id\n\tr145-20020a0de897000000b00589f9c38b2emr11539971ywe.20.1692784257936;\n\tWed, 23 Aug 2023 02:50:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731094641.73646-1-david.plowman@raspberrypi.com>\n\t<20230731094641.73646-6-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","Date":"Wed, 23 Aug 2023 10:50:42 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]