[{"id":26768,"web_url":"https://patchwork.libcamera.org/comment/26768/","msgid":"","date":"2023-03-27T13:31:06","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure\n\tshutter speed and gain are clipped in the AGC","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 patch.\n\nOn Mon, 27 Mar 2023 at 10:34, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Make a copy of the CameraMode structure on a switch mode call. This\n> replaces the existing lastSensitivity_ field.\n>\n> Limit the AGC gain calculations to the minimum value given by the\n> CameraMode structure. The maximum value remains unclipped as any gain\n> over the sensor maximum will be made up by digital gain.\n>\n> Rename clipShutter to limitShutter for consistency, and have the latter\n> limit the shutter speed to both upper and lower bounds.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 55 +++++++++++++++++-----\n> src/ipa/raspberrypi/controller/rpi/agc.h | 4 +-\n> 2 files changed, 45 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 4ea0dd41e66c..ed7ecbe05a8e 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -309,14 +309,14 @@ void Agc::setFixedShutter(Duration fixedShutter)\n> {\n> fixedShutter_ = fixedShutter;\n> /* Set this in case someone calls disableAuto() straight after. */\n> - status_.shutterTime = clipShutter(fixedShutter_);\n> + status_.shutterTime = limitShutter(fixedShutter_);\n> }\n>\n> void Agc::setFixedAnalogueGain(double fixedAnalogueGain)\n> {\n> fixedAnalogueGain_ = fixedAnalogueGain;\n> /* Set this in case someone calls disableAuto() straight after. */\n> - status_.analogueGain = fixedAnalogueGain;\n> + status_.analogueGain = limitGain(fixedAnalogueGain);\n> }\n>\n> void Agc::setMeteringMode(std::string const &meteringModeName)\n> @@ -342,7 +342,7 @@ void Agc::switchMode(CameraMode const &cameraMode,\n>\n> housekeepConfig();\n>\n> - Duration fixedShutter = clipShutter(fixedShutter_);\n> + Duration fixedShutter = limitShutter(fixedShutter_);\n> if (fixedShutter && fixedAnalogueGain_) {\n> /* We're going to reset the algorithm here with these fixed values. */\n>\n> @@ -371,7 +371,7 @@ void Agc::switchMode(CameraMode const &cameraMode,\n> * current exposure profile, which takes care of everything else.\n> */\n>\n> - double ratio = lastSensitivity_ / cameraMode.sensitivity;\n> + double ratio = mode_.sensitivity / cameraMode.sensitivity;\n> target_.totalExposureNoDG *= ratio;\n> target_.totalExposure *= ratio;\n> filtered_.totalExposureNoDG *= ratio;\n> @@ -393,8 +393,11 @@ void Agc::switchMode(CameraMode const &cameraMode,\n>\n> writeAndFinish(metadata, false);\n>\n> - /* We must remember the sensitivity of this mode for the next SwitchMode. */\n> - lastSensitivity_ = cameraMode.sensitivity;\n> + /*\n> + * Store the more in the local state. We must remember the sensitivity\n\ns/more/mode/ I guess.\n\n> + * of this mode for the next SwitchMode.\n> + */\n> + mode_ = cameraMode;\n> }\n>\n> void Agc::prepare(Metadata *imageMetadata)\n> @@ -516,7 +519,7 @@ void Agc::housekeepConfig()\n> {\n> /* First fetch all the up-to-date settings, so no one else has to do it. */\n> status_.ev = ev_;\n> - status_.fixedShutter = clipShutter(fixedShutter_);\n> + status_.fixedShutter = limitShutter(fixedShutter_);\n> status_.fixedAnalogueGain = fixedAnalogueGain_;\n> status_.flickerPeriod = flickerPeriod_;\n> LOG(RPiAgc, Debug) << \"ev \" << status_.ev << \" fixedShutter \"\n> @@ -703,7 +706,7 @@ void Agc::computeTargetExposure(double gain)\n> Duration maxShutter = status_.fixedShutter\n> ? status_.fixedShutter\n> : exposureMode_->shutter.back();\n> - maxShutter = clipShutter(maxShutter);\n> + maxShutter = limitShutter(maxShutter);\n> Duration maxTotalExposure =\n> maxShutter *\n> (status_.fixedAnalogueGain != 0.0\n> @@ -803,15 +806,16 @@ void Agc::divideUpExposure()\n> double analogueGain;\n> shutterTime = status_.fixedShutter ? status_.fixedShutter\n> : exposureMode_->shutter[0];\n> - shutterTime = clipShutter(shutterTime);\n> + shutterTime = limitShutter(shutterTime);\n> analogueGain = status_.fixedAnalogueGain != 0.0 ? status_.fixedAnalogueGain\n> : exposureMode_->gain[0];\n> + analogueGain = limitGain(analogueGain);\n> if (shutterTime * analogueGain < exposureValue) {\n> for (unsigned int stage = 1;\n> stage < exposureMode_->gain.size(); stage++) {\n> if (!status_.fixedShutter) {\n> Duration stageShutter =\n> - clipShutter(exposureMode_->shutter[stage]);\n> + limitShutter(exposureMode_->shutter[stage]);\n> if (stageShutter * analogueGain >= exposureValue) {\n> shutterTime = exposureValue / analogueGain;\n> break;\n> @@ -824,6 +828,7 @@ void Agc::divideUpExposure()\n> break;\n> }\n> analogueGain = exposureMode_->gain[stage];\n> + analogueGain = limitGain(analogueGain);\n> }\n> }\n> }\n> @@ -846,6 +851,7 @@ void Agc::divideUpExposure()\n> * gain as a side-effect.\n> */\n> analogueGain = std::min(analogueGain, exposureMode_->gain.back());\n> + analogueGain = limitGain(analogueGain);\n> shutterTime = newShutterTime;\n> }\n> LOG(RPiAgc, Debug) << \"After flicker avoidance, shutter \"\n> @@ -872,13 +878,36 @@ void Agc::writeAndFinish(Metadata *imageMetadata, bool desaturate)\n> << \" analogue gain \" << filtered_.analogueGain;\n> }\n>\n> -Duration Agc::clipShutter(Duration shutter)\n> +Duration Agc::limitShutter(Duration shutter)\n> {\n> - if (maxShutter_)\n> - shutter = std::min(shutter, maxShutter_);\n> + /*\n> + * shutter == 0 is a special case for fixed shutter values, and must pass\n> + * through unchanged\n> + */\n> + if (!shutter)\n> + return shutter;\n> +\n> + shutter = std::clamp(shutter, mode_.minShutter, maxShutter_);\n> return shutter;\n> }\n>\n> +double Agc::limitGain(double gain) const\n> +{\n> + /*\n> + * Only limit the lower bounds of the gain value to what the sensor limits.\n> + * The upper bound on analogue gain will be made up with additional digital\n> + * gain applied by the ISP.\n> + *\n> + * gain == 0.0 is a special case for fixed shutter values, and must pass\n> + * through unchanged\n> + */\n> + if (!gain)\n> + return gain;\n> +\n> + gain = std::max(gain, mode_.minAnalogueGain);\n> + return gain;\n> +}\n> +\n> /* Register algorithm with the system. */\n> static Algorithm *create(Controller *controller)\n> {\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h\n> index f04896ca25ad..661d8b08cea2 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> @@ -103,10 +103,12 @@ private:\n> void filterExposure(bool desaturate);\n> void divideUpExposure();\n> void writeAndFinish(Metadata *imageMetadata, bool desaturate);\n> - libcamera::utils::Duration clipShutter(libcamera::utils::Duration shutter);\n> + libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);\n> + double limitGain(double gain) const;\n> AgcMeteringMode *meteringMode_;\n> AgcExposureMode *exposureMode_;\n> AgcConstraintMode *constraintMode_;\n> + CameraMode mode_;\n> uint64_t frameCount_;\n> AwbStatus awb_;\n> struct ExposureValues {\n> --\n\nJust wondering if there's a lastSensitivity_ field in here that wants\nremoving now?\n\nOther than these trivial things:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1CCDFBD160\n\tfor ;\n\tMon, 27 Mar 2023 13:31:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7717F62724;\n\tMon, 27 Mar 2023 15:31:21 +0200 (CEST)","from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com\n\t[IPv6:2607:f8b0:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E837B626D7\n\tfor ;\n\tMon, 27 Mar 2023 15:31:19 +0200 (CEST)","by mail-oi1-x22b.google.com with SMTP id r16so6241218oij.5\n\tfor ;\n\tMon, 27 Mar 2023 06:31:19 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679923881;\n\tbh=H528gp7YkGgRU/7750skp/+E2Zgd+IEsu3sIlqI2jxw=;\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=Kp5lsVMtPqMYHNJMg+Jj4J9XkhUeUGa0tEryd9j9X1AGeBSLCZGiNKfit/334Ggjy\n\tkfDyPAeIpw4vLMQ2YJ0cCbqfEiiKmaxG3122pzH+aeZiHf0SYGbIsONzxuybprvqU3\n\tyWbutDmNPFhWfcVf5gWqddMxiLnT0Jug7MWcQAKMouZi3FON2TR8dHbV7Cnw+MKgBW\n\tkSgwFOqUdkh3BbZWdaxSLFgAR6Ww5t2g5rudtT7ZcXdnk7HvUi4LiHm2jVFiGlIjvi\n\t/CjbwfFA9rGcH6RJj6CFtMU3TZ6pQV2ZNT4rjxtulFfqeKDshWo3xs0Y3zesOdOy45\n\tglOMFhZ+D6u1g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679923878;\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=fXzOgtYbUvojeEjyi5wpFEBTY11l81avPFmryX0Nufo=;\n\tb=Q0rpaHyQyvTY+WH5+pLOp37Bifc+BoJXKc/iZOdJHzPWrO268OWZQr34XjhudHku8m\n\tTk+aGjoa+4KZ5Zjhjp2mOu1/aXX8Rv21FzntNN9a2xxBSbRpPgsVhqfn8HQaRHviECVW\n\tWVxBKAv6zhXOqnSz5bTm5UBPltJjA0oocBrzpimlkQfvpFBd2VnpZZgwE+kCitdM7jdI\n\tQppgwiWWBnuSLGGILtPYIBmmyZEcW8MY/Q+YYfGExj7RtDmciSCK3FnEyw2bFyf/D2Gr\n\tKrSOCAI5WoCsIdzWOS+RM4K8RBrGHEfLA9zLHSVuiQy8RWA96WS+D4CRFTV0wFLNg8hS\n\tTOCw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Q0rpaHyQ\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679923878;\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=fXzOgtYbUvojeEjyi5wpFEBTY11l81avPFmryX0Nufo=;\n\tb=mqwUTIC9Iy7a0QW/HZdvyWK2nRV/FE8QWKhbLlhX+3wsjxenUXDmmAnAl5vJO6XHk4\n\tv532LnB6HNuZssgQJ4DHtJ87CvvbyhTf8WbOFEXiImN89UrKpEzzACbKOHxPciduq7+3\n\tvTWRf5MLB7+eGpjHAqXZKH483u7f2at3prjV91rL+DZffHu45mrgGX+wTM1KIMo+4bSE\n\tRg177/kfvCcrEE+V8Jpk3p3NCTNpXy24NlWrON4FkzVRcAiJkbOCqatexT3QyWMO4p3b\n\tgUfwu63YY0R18SXClcL5YIjlG8o1xiwQn5OmeUXYD6K2o7z9p13wxYJja+ewkt3QfwmM\n\tIHUw==","X-Gm-Message-State":"AO0yUKU2DfGKGRqDYjBg3WSmra6SzOk7wKqzVhnmIhtKP7EtcbgaQ+FI\n\t/6xNRHhKnNVMlcO4tVxzJW4XQSJn3cybkc4MyW9ii2Gtmbk9+pgFcpA=","X-Google-Smtp-Source":"AK7set9ErL215LGSaNFsrU+9UHacqDpcDLsFXZVAbixm9eohiWT07qk4WrGWn9aearp5Lw36zO/GaRtkrTjwI0kR6vY=","X-Received":"by 2002:a54:4792:0:b0:386:bb7a:5c85 with SMTP id\n\to18-20020a544792000000b00386bb7a5c85mr3468458oic.11.1679923877267;\n\tMon, 27 Mar 2023 06:31:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20230327093439.24670-1-naush@raspberrypi.com>\n\t<20230327093439.24670-4-naush@raspberrypi.com>","In-Reply-To":"<20230327093439.24670-4-naush@raspberrypi.com>","Date":"Mon, 27 Mar 2023 14:31:06 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure\n\tshutter speed and gain are clipped in the AGC","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":26773,"web_url":"https://patchwork.libcamera.org/comment/26773/","msgid":"<20230327134758.432i56xhpe2r4fis@uno.localdomain>","date":"2023-03-27T13:47:58","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure\n\tshutter speed and gain are clipped in the AGC","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Mon, Mar 27, 2023 at 10:34:38AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Make a copy of the CameraMode structure on a switch mode call. This\n> replaces the existing lastSensitivity_ field.\n>\n> Limit the AGC gain calculations to the minimum value given by the\n> CameraMode structure. The maximum value remains unclipped as any gain\n> over the sensor maximum will be made up by digital gain.\n>\n> Rename clipShutter to limitShutter for consistency, and have the latter\n> limit the shutter speed to both upper and lower bounds.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 55 +++++++++++++++++-----\n> src/ipa/raspberrypi/controller/rpi/agc.h | 4 +-\n> 2 files changed, 45 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 4ea0dd41e66c..ed7ecbe05a8e 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -309,14 +309,14 @@ void Agc::setFixedShutter(Duration fixedShutter)\n> {\n> \tfixedShutter_ = fixedShutter;\n> \t/* Set this in case someone calls disableAuto() straight after. */\n> -\tstatus_.shutterTime = clipShutter(fixedShutter_);\n> +\tstatus_.shutterTime = limitShutter(fixedShutter_);\n> }\n>\n> void Agc::setFixedAnalogueGain(double fixedAnalogueGain)\n> {\n> \tfixedAnalogueGain_ = fixedAnalogueGain;\n> \t/* Set this in case someone calls disableAuto() straight after. */\n> -\tstatus_.analogueGain = fixedAnalogueGain;\n> +\tstatus_.analogueGain = limitGain(fixedAnalogueGain);\n> }\n>\n> void Agc::setMeteringMode(std::string const &meteringModeName)\n> @@ -342,7 +342,7 @@ void Agc::switchMode(CameraMode const &cameraMode,\n>\n> \thousekeepConfig();\n>\n> -\tDuration fixedShutter = clipShutter(fixedShutter_);\n> +\tDuration fixedShutter = limitShutter(fixedShutter_);\n> \tif (fixedShutter && fixedAnalogueGain_) {\n> \t\t/* We're going to reset the algorithm here with these fixed values. */\n>\n> @@ -371,7 +371,7 @@ void Agc::switchMode(CameraMode const &cameraMode,\n> \t\t * current exposure profile, which takes care of everything else.\n> \t\t */\n>\n> -\t\tdouble ratio = lastSensitivity_ / cameraMode.sensitivity;\n> +\t\tdouble ratio = mode_.sensitivity / cameraMode.sensitivity;\n> \t\ttarget_.totalExposureNoDG *= ratio;\n> \t\ttarget_.totalExposure *= ratio;\n> \t\tfiltered_.totalExposureNoDG *= ratio;\n> @@ -393,8 +393,11 @@ void Agc::switchMode(CameraMode const &cameraMode,\n>\n> \twriteAndFinish(metadata, false);\n>\n> -\t/* We must remember the sensitivity of this mode for the next SwitchMode. */\n> -\tlastSensitivity_ = cameraMode.sensitivity;\n> +\t/*\n> +\t * Store the more in the local state. We must remember the sensitivity\n> +\t * of this mode for the next SwitchMode.\n> +\t */\n> +\tmode_ = cameraMode;\n\nI see that mode_.sensitivity is accessed in the else if() clause\nabove, as well as limitShutter() is called before the whole if() {}\nblock, but mode_ is only updatd here. I presume this is intentional as\nit was already like this with lastSensitivity_ ?\n\nBeside David's comments:\nReviewed-by: Jacopo Mondi \n\n\n> }\n>\n> void Agc::prepare(Metadata *imageMetadata)\n> @@ -516,7 +519,7 @@ void Agc::housekeepConfig()\n> {\n> \t/* First fetch all the up-to-date settings, so no one else has to do it. */\n> \tstatus_.ev = ev_;\n> -\tstatus_.fixedShutter = clipShutter(fixedShutter_);\n> +\tstatus_.fixedShutter = limitShutter(fixedShutter_);\n> \tstatus_.fixedAnalogueGain = fixedAnalogueGain_;\n> \tstatus_.flickerPeriod = flickerPeriod_;\n> \tLOG(RPiAgc, Debug) << \"ev \" << status_.ev << \" fixedShutter \"\n> @@ -703,7 +706,7 @@ void Agc::computeTargetExposure(double gain)\n> \t\tDuration maxShutter = status_.fixedShutter\n> \t\t\t\t\t ? status_.fixedShutter\n> \t\t\t\t\t : exposureMode_->shutter.back();\n> -\t\tmaxShutter = clipShutter(maxShutter);\n> +\t\tmaxShutter = limitShutter(maxShutter);\n> \t\tDuration maxTotalExposure =\n> \t\t\tmaxShutter *\n> \t\t\t(status_.fixedAnalogueGain != 0.0\n> @@ -803,15 +806,16 @@ void Agc::divideUpExposure()\n> \tdouble analogueGain;\n> \tshutterTime = status_.fixedShutter ? status_.fixedShutter\n> \t\t\t\t\t : exposureMode_->shutter[0];\n> -\tshutterTime = clipShutter(shutterTime);\n> +\tshutterTime = limitShutter(shutterTime);\n> \tanalogueGain = status_.fixedAnalogueGain != 0.0 ? status_.fixedAnalogueGain\n> \t\t\t\t\t\t\t: exposureMode_->gain[0];\n> +\tanalogueGain = limitGain(analogueGain);\n> \tif (shutterTime * analogueGain < exposureValue) {\n> \t\tfor (unsigned int stage = 1;\n> \t\t stage < exposureMode_->gain.size(); stage++) {\n> \t\t\tif (!status_.fixedShutter) {\n> \t\t\t\tDuration stageShutter =\n> -\t\t\t\t\tclipShutter(exposureMode_->shutter[stage]);\n> +\t\t\t\t\tlimitShutter(exposureMode_->shutter[stage]);\n> \t\t\t\tif (stageShutter * analogueGain >= exposureValue) {\n> \t\t\t\t\tshutterTime = exposureValue / analogueGain;\n> \t\t\t\t\tbreak;\n> @@ -824,6 +828,7 @@ void Agc::divideUpExposure()\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t\t\tanalogueGain = exposureMode_->gain[stage];\n> +\t\t\t\tanalogueGain = limitGain(analogueGain);\n> \t\t\t}\n> \t\t}\n> \t}\n> @@ -846,6 +851,7 @@ void Agc::divideUpExposure()\n> \t\t\t * gain as a side-effect.\n> \t\t\t */\n> \t\t\tanalogueGain = std::min(analogueGain, exposureMode_->gain.back());\n> +\t\t\tanalogueGain = limitGain(analogueGain);\n> \t\t\tshutterTime = newShutterTime;\n> \t\t}\n> \t\tLOG(RPiAgc, Debug) << \"After flicker avoidance, shutter \"\n> @@ -872,13 +878,36 @@ void Agc::writeAndFinish(Metadata *imageMetadata, bool desaturate)\n> \t\t\t << \" analogue gain \" << filtered_.analogueGain;\n> }\n>\n> -Duration Agc::clipShutter(Duration shutter)\n> +Duration Agc::limitShutter(Duration shutter)\n> {\n> -\tif (maxShutter_)\n> -\t\tshutter = std::min(shutter, maxShutter_);\n> +\t/*\n> +\t * shutter == 0 is a special case for fixed shutter values, and must pass\n> +\t * through unchanged\n> +\t */\n> +\tif (!shutter)\n> +\t\treturn shutter;\n> +\n> +\tshutter = std::clamp(shutter, mode_.minShutter, maxShutter_);\n> \treturn shutter;\n> }\n>\n> +double Agc::limitGain(double gain) const\n> +{\n> +\t/*\n> +\t * Only limit the lower bounds of the gain value to what the sensor limits.\n> +\t * The upper bound on analogue gain will be made up with additional digital\n> +\t * gain applied by the ISP.\n> +\t *\n> +\t * gain == 0.0 is a special case for fixed shutter values, and must pass\n> +\t * through unchanged\n> +\t */\n> +\tif (!gain)\n> +\t\treturn gain;\n> +\n> +\tgain = std::max(gain, mode_.minAnalogueGain);\n> +\treturn gain;\n> +}\n> +\n> /* Register algorithm with the system. */\n> static Algorithm *create(Controller *controller)\n> {\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h\n> index f04896ca25ad..661d8b08cea2 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> @@ -103,10 +103,12 @@ private:\n> \tvoid filterExposure(bool desaturate);\n> \tvoid divideUpExposure();\n> \tvoid writeAndFinish(Metadata *imageMetadata, bool desaturate);\n> -\tlibcamera::utils::Duration clipShutter(libcamera::utils::Duration shutter);\n> +\tlibcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);\n> +\tdouble limitGain(double gain) const;\n> \tAgcMeteringMode *meteringMode_;\n> \tAgcExposureMode *exposureMode_;\n> \tAgcConstraintMode *constraintMode_;\n> +\tCameraMode mode_;\n> \tuint64_t frameCount_;\n> \tAwbStatus awb_;\n> \tstruct ExposureValues {\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 077BFBD160\n\tfor ;\n\tMon, 27 Mar 2023 13:48:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46FB962724;\n\tMon, 27 Mar 2023 15:48:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 25757626D7\n\tfor ;\n\tMon, 27 Mar 2023 15:48:02 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 964C57FA;\n\tMon, 27 Mar 2023 15:48:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679924883;\n\tbh=8ruRPjvUW6aybvyZEvWeO1bm3fV9wynNCX6o2AGv5cc=;\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=C3Aiplh/wBjnyrBGvFvDqTuNcTXwMM5f1P/TyuR6U0vdWISlNtWlTKJlVPUlSqRb6\n\tc0iZJiZwA/5ibN9HGoyi/nddQF9WywgYm0TTtJyT8rsa0X3+Q8E8XG63FTXxsbof/p\n\txUnqWE9DDTv82ilhaMUIoN3EzTwJwid7lFgGJp/RQcpxnntxSnW5W4x7FHhus2HgRv\n\t37hUBOOMBjr6jcDLRPq4l7t5MfXSl1Mw0kaHYyr9kiKJKWhhVYqJuZ2tI6M3nfCj/v\n\tlH8hCYWYOPACTmXr5EspKcNUTYZ3+5S8LsV2QZeEHXN2xSUYEbfETkqzoKXJ4Z/eKw\n\tqqcF9f7SpHa4Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679924881;\n\tbh=8ruRPjvUW6aybvyZEvWeO1bm3fV9wynNCX6o2AGv5cc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c/wkXiA9Ilu6dx/AHzDFevFURRywu3GnQ47SZXj83PzOHWl6CXkO6/sAlHeey8X0Z\n\t0fs15D2BA6Z2fNYzVjyNAhJFb3VkRUg+B1FzUyHXjxUPWi4jYZzJ34JEvDe0+Yng+U\n\tf1JlKG4CCoVNWPTeI2lNKzrmUXARn0NK9Q65gLwQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"c/wkXiA9\"; dkim-atps=neutral","Date":"Mon, 27 Mar 2023 15:47:58 +0200","To":"Naushir Patuck ","Message-ID":"<20230327134758.432i56xhpe2r4fis@uno.localdomain>","References":"<20230327093439.24670-1-naush@raspberrypi.com>\n\t<20230327093439.24670-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230327093439.24670-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure\n\tshutter speed and gain are clipped in the AGC","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":26776,"web_url":"https://patchwork.libcamera.org/comment/26776/","msgid":"","date":"2023-03-27T13:57:18","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure\n\tshutter speed and gain are clipped in the AGC","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\n\nOn Mon, 27 Mar 2023 at 14:48, Jacopo Mondi \nwrote:\n\n> Hi Naush\n>\n> On Mon, Mar 27, 2023 at 10:34:38AM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > Make a copy of the CameraMode structure on a switch mode call. This\n> > replaces the existing lastSensitivity_ field.\n> >\n> > Limit the AGC gain calculations to the minimum value given by the\n> > CameraMode structure. The maximum value remains unclipped as any gain\n> > over the sensor maximum will be made up by digital gain.\n> >\n> > Rename clipShutter to limitShutter for consistency, and have the latter\n> > limit the shutter speed to both upper and lower bounds.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 55 +++++++++++++++++-----\n> > src/ipa/raspberrypi/controller/rpi/agc.h | 4 +-\n> > 2 files changed, 45 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 4ea0dd41e66c..ed7ecbe05a8e 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -309,14 +309,14 @@ void Agc::setFixedShutter(Duration fixedShutter)\n> > {\n> > fixedShutter_ = fixedShutter;\n> > /* Set this in case someone calls disableAuto() straight after. */\n> > - status_.shutterTime = clipShutter(fixedShutter_);\n> > + status_.shutterTime = limitShutter(fixedShutter_);\n> > }\n> >\n> > void Agc::setFixedAnalogueGain(double fixedAnalogueGain)\n> > {\n> > fixedAnalogueGain_ = fixedAnalogueGain;\n> > /* Set this in case someone calls disableAuto() straight after. */\n> > - status_.analogueGain = fixedAnalogueGain;\n> > + status_.analogueGain = limitGain(fixedAnalogueGain);\n> > }\n> >\n> > void Agc::setMeteringMode(std::string const &meteringModeName)\n> > @@ -342,7 +342,7 @@ void Agc::switchMode(CameraMode const &cameraMode,\n> >\n> > housekeepConfig();\n> >\n> > - Duration fixedShutter = clipShutter(fixedShutter_);\n> > + Duration fixedShutter = limitShutter(fixedShutter_);\n> > if (fixedShutter && fixedAnalogueGain_) {\n> > /* We're going to reset the algorithm here with these\n> fixed values. */\n> >\n> > @@ -371,7 +371,7 @@ void Agc::switchMode(CameraMode const &cameraMode,\n> > * current exposure profile, which takes care of\n> everything else.\n> > */\n> >\n> > - double ratio = lastSensitivity_ / cameraMode.sensitivity;\n> > + double ratio = mode_.sensitivity / cameraMode.sensitivity;\n> > target_.totalExposureNoDG *= ratio;\n> > target_.totalExposure *= ratio;\n> > filtered_.totalExposureNoDG *= ratio;\n> > @@ -393,8 +393,11 @@ void Agc::switchMode(CameraMode const &cameraMode,\n> >\n> > writeAndFinish(metadata, false);\n> >\n> > - /* We must remember the sensitivity of this mode for the next\n> SwitchMode. */\n> > - lastSensitivity_ = cameraMode.sensitivity;\n> > + /*\n> > + * Store the more in the local state. We must remember the\n> sensitivity\n> > + * of this mode for the next SwitchMode.\n> > + */\n> > + mode_ = cameraMode;\n>\n> I see that mode_.sensitivity is accessed in the else if() clause\n> above, as well as limitShutter() is called before the whole if() {}\n> block, but mode_ is only updatd here. I presume this is intentional as\n> it was already like this with lastSensitivity_ ?\n>\n\nYou are right! I really ought to move the mode_ = cameraMode to the top of\nthis\nfunction. The consequence of doing it this way is minimal, we will use a 0\nvalue for the minimum shutter time when clipping. But it is wrong, so\nshould be\nfix.\n\nI'll post an update to this patch in-line and fix David's reported typo as\nwell.\n\nRegards,\nNaush\n\n\n>\n> Beside David's comments:\n> Reviewed-by: Jacopo Mondi \n>\n>\n> > }\n> >\n> > void Agc::prepare(Metadata *imageMetadata)\n> > @@ -516,7 +519,7 @@ void Agc::housekeepConfig()\n> > {\n> > /* First fetch all the up-to-date settings, so no one else has to\n> do it. */\n> > status_.ev = ev_;\n> > - status_.fixedShutter = clipShutter(fixedShutter_);\n> > + status_.fixedShutter = limitShutter(fixedShutter_);\n> > status_.fixedAnalogueGain = fixedAnalogueGain_;\n> > status_.flickerPeriod = flickerPeriod_;\n> > LOG(RPiAgc, Debug) << \"ev \" << status_.ev << \" fixedShutter \"\n> > @@ -703,7 +706,7 @@ void Agc::computeTargetExposure(double gain)\n> > Duration maxShutter = status_.fixedShutter\n> > ? status_.fixedShutter\n> > :\n> exposureMode_->shutter.back();\n> > - maxShutter = clipShutter(maxShutter);\n> > + maxShutter = limitShutter(maxShutter);\n> > Duration maxTotalExposure =\n> > maxShutter *\n> > (status_.fixedAnalogueGain != 0.0\n> > @@ -803,15 +806,16 @@ void Agc::divideUpExposure()\n> > double analogueGain;\n> > shutterTime = status_.fixedShutter ? status_.fixedShutter\n> > : exposureMode_->shutter[0];\n> > - shutterTime = clipShutter(shutterTime);\n> > + shutterTime = limitShutter(shutterTime);\n> > analogueGain = status_.fixedAnalogueGain != 0.0 ?\n> status_.fixedAnalogueGain\n> > :\n> exposureMode_->gain[0];\n> > + analogueGain = limitGain(analogueGain);\n> > if (shutterTime * analogueGain < exposureValue) {\n> > for (unsigned int stage = 1;\n> > stage < exposureMode_->gain.size(); stage++) {\n> > if (!status_.fixedShutter) {\n> > Duration stageShutter =\n> > -\n> clipShutter(exposureMode_->shutter[stage]);\n> > +\n> limitShutter(exposureMode_->shutter[stage]);\n> > if (stageShutter * analogueGain >=\n> exposureValue) {\n> > shutterTime = exposureValue /\n> analogueGain;\n> > break;\n> > @@ -824,6 +828,7 @@ void Agc::divideUpExposure()\n> > break;\n> > }\n> > analogueGain = exposureMode_->gain[stage];\n> > + analogueGain = limitGain(analogueGain);\n> > }\n> > }\n> > }\n> > @@ -846,6 +851,7 @@ void Agc::divideUpExposure()\n> > * gain as a side-effect.\n> > */\n> > analogueGain = std::min(analogueGain,\n> exposureMode_->gain.back());\n> > + analogueGain = limitGain(analogueGain);\n> > shutterTime = newShutterTime;\n> > }\n> > LOG(RPiAgc, Debug) << \"After flicker avoidance, shutter \"\n> > @@ -872,13 +878,36 @@ void Agc::writeAndFinish(Metadata *imageMetadata,\n> bool desaturate)\n> > << \" analogue gain \" << filtered_.analogueGain;\n> > }\n> >\n> > -Duration Agc::clipShutter(Duration shutter)\n> > +Duration Agc::limitShutter(Duration shutter)\n> > {\n> > - if (maxShutter_)\n> > - shutter = std::min(shutter, maxShutter_);\n> > + /*\n> > + * shutter == 0 is a special case for fixed shutter values, and\n> must pass\n> > + * through unchanged\n> > + */\n> > + if (!shutter)\n> > + return shutter;\n> > +\n> > + shutter = std::clamp(shutter, mode_.minShutter, maxShutter_);\n> > return shutter;\n> > }\n> >\n> > +double Agc::limitGain(double gain) const\n> > +{\n> > + /*\n> > + * Only limit the lower bounds of the gain value to what the\n> sensor limits.\n> > + * The upper bound on analogue gain will be made up with\n> additional digital\n> > + * gain applied by the ISP.\n> > + *\n> > + * gain == 0.0 is a special case for fixed shutter values, and\n> must pass\n> > + * through unchanged\n> > + */\n> > + if (!gain)\n> > + return gain;\n> > +\n> > + gain = std::max(gain, mode_.minAnalogueGain);\n> > + return gain;\n> > +}\n> > +\n> > /* Register algorithm with the system. */\n> > static Algorithm *create(Controller *controller)\n> > {\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h\n> b/src/ipa/raspberrypi/controller/rpi/agc.h\n> > index f04896ca25ad..661d8b08cea2 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> > @@ -103,10 +103,12 @@ private:\n> > void filterExposure(bool desaturate);\n> > void divideUpExposure();\n> > void writeAndFinish(Metadata *imageMetadata, bool desaturate);\n> > - libcamera::utils::Duration clipShutter(libcamera::utils::Duration\n> shutter);\n> > + libcamera::utils::Duration limitShutter(libcamera::utils::Duration\n> shutter);\n> > + double limitGain(double gain) const;\n> > AgcMeteringMode *meteringMode_;\n> > AgcExposureMode *exposureMode_;\n> > AgcConstraintMode *constraintMode_;\n> > + CameraMode mode_;\n> > uint64_t frameCount_;\n> > AwbStatus awb_;\n> > struct ExposureValues {\n> > --\n> > 2.34.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9EB0DBD160\n\tfor ;\n\tMon, 27 Mar 2023 13:57:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 049DA62724;\n\tMon, 27 Mar 2023 15:57:35 +0200 (CEST)","from mail-yb1-xb31.google.com (mail-yb1-xb31.google.com\n\t[IPv6:2607:f8b0:4864:20::b31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82730626D7\n\tfor ;\n\tMon, 27 Mar 2023 15:57:33 +0200 (CEST)","by mail-yb1-xb31.google.com with SMTP id e65so10631214ybh.10\n\tfor ;\n\tMon, 27 Mar 2023 06:57:33 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679925455;\n\tbh=UoORzBN/YfMFSVHjrj8l89RyQYqIVHE5+bXyAuxCyhA=;\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=BU89ik3yVaDsxZdqhfCIzqdtk8W05dO6ql8dI+BwPDnI34ntzko7wDnLARn8dgNmd\n\tw91jShpxxUzG/Yd32dMMKbdNJbdzORntUTsasC/u8vLrdG30mu+770KtRd2xOTDUp9\n\tx3dWxs7UAaQkX/UmnfixQMWRSvEyPHlx5Aj9k3ekGQs+8X/jIPnbqmiV3m4NRdsLV5\n\tsaiXm7NWvks2CsQAeY7t9HWXj2/J4JhUlvfM65oKDBwsxZ7Y9ewwUoFicE93N6V9Sw\n\tvI6Mr+oit7SxZ0UHD1bAkNp2cG9nigKei0kuQD8bwQlzos6hZGH++92aavBb1qSFJr\n\t/9U971crKeSLA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679925452;\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=5dPeJyn0njE5PyPK/Mf/Mk2Un50TLHDsaLjcc3ugPeM=;\n\tb=D1mwhYCBLzqA6yfP5aVNbrh0O4VWhZgunoyyc9YWz4M+Qewu/2ly/AqWFdlQMtDqmp\n\tZGyKFJtIf+BdPyyvs+/Qo15YLcI77Ytz/Zy0uSjWMf5UTuZ22HmNRA9t+AVyQELZkRCv\n\tRRkdHXq1n+FOqPrjQq3IWSxro9qpXUXj28oaS8ubYxmPl5yuhA6ZdxZK8R9qNG61NlRs\n\tY1hLDmkkfbNHgZBRzfi/gPyCuAHwQb+739btpi6CJ9KMpUbQBdtjC0nGG/2nP3TmOUfp\n\tB0JqCjBsoimeY24IVn2Qw46pcq6uM9WzjeBvOTqKTbhvRSVeGEHq2JEc0PN2jQ+McKx/\n\tVNPQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"D1mwhYCB\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679925452;\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=5dPeJyn0njE5PyPK/Mf/Mk2Un50TLHDsaLjcc3ugPeM=;\n\tb=lBv2XrrzK4UsdscRBJ1+SugAq4hEXS5SZX8vEgDjSc4HhXfC/7kupr0wFGGrWzE1cF\n\tc+wFo2CLsxkmvkYqnVT2BoWklBcwQBtGLzKXT045K6Yhg6Ui6cmjhmT/Ji+4wnk3s9ei\n\tf6lzRhD8ncZzxVEKHlsqAmjQpWpt2cBSIoHwj6oRCRkN044DT7Sw9R5EKhnxhULKeTg9\n\t9uewt93mSyFfazTDozjONNP6gfOeY0FZswzWc4pQo+XCpieyek+9PYnOF/H837XH6GLj\n\t2pnBrsYmIVxsuqSLRY2RYiv+dlRgp8kC16Kq34Rh/i8VaEX6zmUcfGuTMjeU9MeMHim7\n\tToEQ==","X-Gm-Message-State":"AAQBX9cgx+SSiTI3NVuTq/xI6ElKtETIkOrQpk9IU6/j3ooMa7NFpIbD\n\tq4p5BiaKdEZAWgkt0Iff89ElnWUqnPV3ciPXNJcPbpGjL6GSftarNCQeOg==","X-Google-Smtp-Source":"AKy350aHLLVy6TtSD6haeChfyDQbnry3OPKTrT4dd/LB7L6Xm4GbUPxgnef8wgE7DXVqoQJn4Sw/+FA84ppRJps6fN0=","X-Received":"by 2002:a05:6902:1201:b0:b6c:4d60:1bd6 with SMTP id\n\ts1-20020a056902120100b00b6c4d601bd6mr7664413ybu.9.1679925452312;\n\tMon, 27 Mar 2023 06:57:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20230327093439.24670-1-naush@raspberrypi.com>\n\t<20230327093439.24670-4-naush@raspberrypi.com>\n\t<20230327134758.432i56xhpe2r4fis@uno.localdomain>","In-Reply-To":"<20230327134758.432i56xhpe2r4fis@uno.localdomain>","Date":"Mon, 27 Mar 2023 14:57:18 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000d5a8fe05f7e21ffc\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure\n\tshutter speed and gain are clipped in the AGC","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\" "}}]