[{"id":27590,"web_url":"https://patchwork.libcamera.org/comment/27590/","msgid":"","date":"2023-07-19T12:36:51","subject":"Re: [libcamera-devel] [PATCH v5 2/2] ipa: rpi: common: Handle\n\tAEC/AGC flicker controls","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Tue, Jul 18, 2023 at 03:43:01PM +0100, David Plowman via libcamera-devel wrote:\n> We handle the flicker modes by passing the correct period to the\n> AEC/AGC algorithm which already contains the necessary code.\n>\n> The \"Auto\" mode, as well as reporting the detected flicker period via\n> the \"AeFlickerDetected\" metadata, are unsupported for now.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++-\n> src/ipa/rpi/common/ipa_base.h | 6 ++++\n> 2 files changed, 69 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index f40f2e71..4438ecd9 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -61,6 +61,10 @@ const ControlInfoMap::Map ipaControls{\n> \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> \t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> \t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> +\t{ &controls::AeFlickerMode, ControlInfo(static_cast(controls::FlickerOff),\n> +\t\t\t\t\t\tstatic_cast(controls::FlickerManual),\n> +\t\t\t\t\t\tstatic_cast(controls::FlickerOff)) },\n> +\t{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },\n> \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> @@ -97,7 +101,7 @@ namespace ipa::RPi {\n>\n> IpaBase::IpaBase()\n> \t: controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),\n> -\t firstStart_(true)\n> +\t firstStart_(true), flickerState_({ 0, 0s })\n> {\n> }\n>\n> @@ -812,6 +816,64 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n>\n> +\t\tcase controls::AE_FLICKER_MODE: {\n> +\t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AeFlickerMode - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tint32_t mode = ctrl.second.get();\n> +\t\t\tbool modeValid = true;\n> +\n> +\t\t\tswitch (mode) {\n> +\t\t\tcase controls::FlickerOff:\n> +\t\t\t\tagc->setFlickerPeriod(0us);\n> +\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tcase controls::FlickerManual:\n> +\t\t\t\tagc->setFlickerPeriod(flickerState_.manualPeriod);\n> +\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(IPARPI, Error) << \"Flicker mode \" << mode << \" is not supported\";\n> +\t\t\t\tmodeValid = false;\n> +\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tif (modeValid)\n> +\t\t\t\tflickerState_.mode = mode;\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tcase controls::AE_FLICKER_PERIOD: {\n> +\t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AeFlickerPeriod - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tuint32_t manualPeriod = ctrl.second.get();\n> +\t\t\tflickerState_.manualPeriod = manualPeriod * 1.0us;\n> +\n> +\t\t\t/*\n> +\t\t\t * We note that it makes no difference if the mode gets set to \"manual\"\n> +\t\t\t * first, and the period updated after, or vice versa.\n\nI guss this is fine, as you said, we're probably overthinking this,\nflicker correction is meant to be updated less frequently than\nexposure/gain\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\t\t\t */\n> +\t\t\tif (flickerState_.mode == controls::FlickerManual)\n> +\t\t\t\tagc->setFlickerPeriod(flickerState_.manualPeriod);\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tcase controls::AWB_ENABLE: {\n> \t\t\t/* Silently ignore this control for a mono sensor. */\n> \t\t\tif (monoSensor_)\n> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> index 39d00760..097f436a 100644\n> --- a/src/ipa/rpi/common/ipa_base.h\n> +++ b/src/ipa/rpi/common/ipa_base.h\n> @@ -116,6 +116,12 @@ private:\n> \t/* Frame duration (1/fps) limits. */\n> \tutils::Duration minFrameDuration_;\n> \tutils::Duration maxFrameDuration_;\n> +\n> +\t/* The current state of flicker avoidance. */\n> +\tstruct FlickerState {\n> +\t\tint32_t mode;\n> +\t\tutils::Duration manualPeriod;\n> +\t} flickerState_;\n> };\n>\n> } /* namespace ipa::RPi */\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 683D5BDB1C\n\tfor ;\n\tWed, 19 Jul 2023 12:36:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAF07628BC;\n\tWed, 19 Jul 2023 14:36:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BF9460381\n\tfor ;\n\tWed, 19 Jul 2023 14:36:54 +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 2418FAF2;\n\tWed, 19 Jul 2023 14:36:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689770215;\n\tbh=z2wYyFTi1jl1mTCix8JRGuCRlYmHpp+3J/q2KUvARfg=;\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=F+9OWcL2D6X+Y1e/yFtsyf23Kamw48Fw/Zym+bqIjPetzU14ZtTAPEMeycydrxUcu\n\taByFnb8XONj1M6MQZiTLBMf9qcHNl0qcfcSSGX3AnKXD8FR5rkYdHtJwE6rh4GaNTG\n\tnV+nNhw7/k5kZFrqyZLsa3vVq0/fttaop5m2t4ncvr4WSvuTyVOeieHfy86W3XwkUI\n\tTyH4TKf84698F3TLxO1pGg0powEUm469egT4xIWaMDjgErNXpbXICxdW05SWED2SUF\n\t5Q0iBgTjHkql6NDkgeK3HMyjVZdQNNroJw2kdgl3R8T5EQdAKt1GHSxWZ47Y2v4nPZ\n\t8jqS4kbRyi+7Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689770160;\n\tbh=z2wYyFTi1jl1mTCix8JRGuCRlYmHpp+3J/q2KUvARfg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nDwjrzthcbFqAZwXcC0dJvZtTqIyviV0lvMjBbgb+MXOw/znWO6lmRGdMC6o1EPyz\n\t6AuhnR3k5rO7OysOwNcIJM5Fnc10wgYhNfgj2ewLOM4esWbwONKuQuLq0ydvQy1Gt5\n\tB8pETg1oEiQnSVJa3+2ARSH5sCotmLYzXhoscoXA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nDwjrzth\"; dkim-atps=neutral","Date":"Wed, 19 Jul 2023 14:36:51 +0200","To":"David Plowman ","Message-ID":"","References":"<20230718144301.3612-1-david.plowman@raspberrypi.com>\n\t<20230718144301.3612-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230718144301.3612-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] ipa: rpi: common: Handle\n\tAEC/AGC flicker controls","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":27594,"web_url":"https://patchwork.libcamera.org/comment/27594/","msgid":"","date":"2023-07-21T04:23:05","subject":"Re: [libcamera-devel] [PATCH v5 2/2] ipa: rpi: common: Handle\n\tAEC/AGC flicker controls","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David\n\nOn 7/18/23 8:13 PM, David Plowman via libcamera-devel wrote:\n> We handle the flicker modes by passing the correct period to the\n> AEC/AGC algorithm which already contains the necessary code.\n>\n> The \"Auto\" mode, as well as reporting the detected flicker period via\n> the \"AeFlickerDetected\" metadata, are unsupported for now.\n>\n> Signed-off-by: David Plowman \n\nReviewed-by: Umang Jain \n\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 64 ++++++++++++++++++++++++++++++++-\n> src/ipa/rpi/common/ipa_base.h | 6 ++++\n> 2 files changed, 69 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index f40f2e71..4438ecd9 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -61,6 +61,10 @@ const ControlInfoMap::Map ipaControls{\n> \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> \t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> \t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> +\t{ &controls::AeFlickerMode, ControlInfo(static_cast(controls::FlickerOff),\n> +\t\t\t\t\t\tstatic_cast(controls::FlickerManual),\n> +\t\t\t\t\t\tstatic_cast(controls::FlickerOff)) },\n> +\t{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },\n> \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> @@ -97,7 +101,7 @@ namespace ipa::RPi {\n> \n> IpaBase::IpaBase()\n> \t: controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),\n> -\t firstStart_(true)\n> +\t firstStart_(true), flickerState_({ 0, 0s })\n> {\n> }\n> \n> @@ -812,6 +816,64 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::AE_FLICKER_MODE: {\n> +\t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AeFlickerMode - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tint32_t mode = ctrl.second.get();\n> +\t\t\tbool modeValid = true;\n> +\n> +\t\t\tswitch (mode) {\n> +\t\t\tcase controls::FlickerOff:\n> +\t\t\t\tagc->setFlickerPeriod(0us);\n> +\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tcase controls::FlickerManual:\n> +\t\t\t\tagc->setFlickerPeriod(flickerState_.manualPeriod);\n> +\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(IPARPI, Error) << \"Flicker mode \" << mode << \" is not supported\";\n> +\t\t\t\tmodeValid = false;\n> +\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tif (modeValid)\n> +\t\t\t\tflickerState_.mode = mode;\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tcase controls::AE_FLICKER_PERIOD: {\n> +\t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AeFlickerPeriod - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tuint32_t manualPeriod = ctrl.second.get();\n> +\t\t\tflickerState_.manualPeriod = manualPeriod * 1.0us;\n> +\n> +\t\t\t/*\n> +\t\t\t * We note that it makes no difference if the mode gets set to \"manual\"\n> +\t\t\t * first, and the period updated after, or vice versa.\n> +\t\t\t */\n> +\t\t\tif (flickerState_.mode == controls::FlickerManual)\n> +\t\t\t\tagc->setFlickerPeriod(flickerState_.manualPeriod);\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tcase controls::AWB_ENABLE: {\n> \t\t\t/* Silently ignore this control for a mono sensor. */\n> \t\t\tif (monoSensor_)\n> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> index 39d00760..097f436a 100644\n> --- a/src/ipa/rpi/common/ipa_base.h\n> +++ b/src/ipa/rpi/common/ipa_base.h\n> @@ -116,6 +116,12 @@ private:\n> \t/* Frame duration (1/fps) limits. */\n> \tutils::Duration minFrameDuration_;\n> \tutils::Duration maxFrameDuration_;\n> +\n> +\t/* The current state of flicker avoidance. */\n> +\tstruct FlickerState {\n> +\t\tint32_t mode;\n> +\t\tutils::Duration manualPeriod;\n> +\t} flickerState_;\n> };\n> \n> } /* namespace ipa::RPi */","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 716A1BDB1C\n\tfor ;\n\tFri, 21 Jul 2023 04:23:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D8A7628BD;\n\tFri, 21 Jul 2023 06:23:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D1A3600F7\n\tfor ;\n\tFri, 21 Jul 2023 06:23:10 +0200 (CEST)","from [192.168.1.108] (unknown [103.86.18.219])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6DC828CC;\n\tFri, 21 Jul 2023 06:22:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689913391;\n\tbh=N5+wkCwnDz2X0dPwrj/9mRzBJ8ZnMHVBadr6GsEri+I=;\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:\n\tFrom;\n\tb=2LJfyMKYSBBzOgQTxuwP8HDqvH+QVEn5CznPesB6TqM1kXsGls+4PziwMXhoEaqFa\n\tf9S+ELTg7jU7V+k6ep3PbqyolZvdD9C5GsOo0+kaD7KWMi1BWEYwVb2SJNI5+TYVuu\n\tOrExSWBibCpGTPFtApXcUDSosD3aK8udPgXwZRVfGD+tn7Q3hGMOwrxqhXbTf8D101\n\tsUNsI/b0XDQB7kqB8U4WS3vjxYrI0BI7nAjV5Bqvme0sjfohewmGenOjx3sdAetrfj\n\tjMdiF2bSGuIcLDU17n1KhXeKPGdCbF/26B5r+F+5Gvj7xH5kJJu+fCD/uEXuJdqPPx\n\twdfhKdEtVwWFQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689913335;\n\tbh=N5+wkCwnDz2X0dPwrj/9mRzBJ8ZnMHVBadr6GsEri+I=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=VovJiuwtR1XLzyVcOJV3/e3cwp6tVABp4lM8E5SCzknv2GxziEBF72zSmQEizSMgG\n\t3Y3S/a48qJ0rTeJWLmWrTXQY6vFYQ0L6UG+MvBDgsjKbrMLkIk8tHERsW4lCKIu7UE\n\t7cZZpaNv138q0//cueIhK6DO7IZcJGQGncKEP+ME="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VovJiuwt\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 21 Jul 2023 09:53:05 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230718144301.3612-1-david.plowman@raspberrypi.com>\n\t<20230718144301.3612-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20230718144301.3612-3-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] ipa: rpi: common: Handle\n\tAEC/AGC flicker controls","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]