[{"id":26807,"web_url":"https://patchwork.libcamera.org/comment/26807/","msgid":"","date":"2023-04-03T08:06:35","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Handle\n\tAEC/AGC flicker controls","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 this work.\n\nOn Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel\n wrote:\n>\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/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++\n> 1 file changed, 80 insertions(+)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 13757955..d485b851 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -91,6 +91,8 @@ static const ControlInfoMap::Map ipaControls{\n> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) },\n> { &controls::AwbEnable, ControlInfo(false, true) },\n> { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> @@ -230,6 +232,12 @@ private:\n> /* Track the frame length times over FrameLengthsQueueSize frames. */\n> std::deque frameLengths_;\n> Duration lastTimeout_;\n> +\n> + /* The current state of flicker avoidance. */\n> + struct FlickerState {\n> + int32_t mode;\n> + Duration customPeriod;\n> + } flickerState_;\n> };\n>\n> int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> @@ -962,6 +970,78 @@ void IPARPi::queueRequest(const ControlList &controls)\n> break;\n> }\n>\n> + case controls::AE_FLICKER_MODE: {\n> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> + controller_.getAlgorithm(\"agc\"));\n> + if (!agc) {\n> + LOG(IPARPI, Warning)\n> + << \"Could not set AeFlickerMode - no AGC algorithm\";\n> + break;\n> + }\n> +\n> + int32_t mode = ctrl.second.get();\n> + bool modeValid = true;\n> +\n> + switch (mode) {\n> + case controls::FlickerOff: {\n> + agc->setFlickerPeriod(0us);\n> +\n> + break;\n> + }\n> +\n> + case controls::FlickerFreq50Hz: {\n> + agc->setFlickerPeriod(10000 * 1.0us);\n> +\n> + break;\n> + }\n> +\n> + case controls::FlickerFreq60Hz: {\n> + agc->setFlickerPeriod(8333.333 * 1.0us);\n> +\n> + break;\n> + }\n> +\n> + case controls::FlickerCustom: {\n> + agc->setFlickerPeriod(flickerState_.customPeriod);\n> +\n> + break;\n> + }\n> +\n> + default:\n> + LOG(IPARPI, Error) << \"Flicker mode \" << mode << \" is not supported\";\n> + modeValid = false;\n> +\n> + break;\n> + }\n> +\n> + if (modeValid)\n> + flickerState_.mode = mode;\n> +\n> + break;\n> + }\n\nPerhaps remove the blank lines above the break statements above?\n\n> +\n> + case controls::AE_FLICKER_CUSTOM: {\n> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> + controller_.getAlgorithm(\"agc\"));\n> + if (!agc) {\n> + LOG(IPARPI, Warning)\n> + << \"Could not set AeFlickerCustom - no AGC algorithm\";\n> + break;\n> + }\n> +\n> + uint32_t customPeriod = ctrl.second.get();\n> + flickerState_.customPeriod = customPeriod * 1.0us;\n> +\n> + /*\n> + * We note that it makes no difference if the mode gets set to \"custom\"\n> + * first, and the period updated after, or vice versa.\n> + */\n> + if (flickerState_.mode == controls::FlickerCustom)\n> + agc->setFlickerPeriod(flickerState_.customPeriod);\n> +\n> + break;\n> + }\n\nThis is not something to do for this change, but for controls like\nAeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom,\nperhaps we can mandate that both controls *must* be set in the same control list\nto be valid? This would avoid having mostly useless state information stored in\nthe IPA class. Ditto for some of the focus, AGC and AWB controls as well. What\ndo folks think? Is that too restrictive on the application?\n\nOther than that:\nReviewed-by: Naushir Patuck \n\n> +\n> case controls::AWB_ENABLE: {\n> RPiController::AwbAlgorithm *awb = dynamic_cast(\n> controller_.getAlgorithm(\"awb\"));\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 A9AD0C0F2A\n\tfor ;\n\tMon, 3 Apr 2023 08:06:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19B7662726;\n\tMon, 3 Apr 2023 10:06:40 +0200 (CEST)","from mail-yw1-x1129.google.com (mail-yw1-x1129.google.com\n\t[IPv6:2607:f8b0:4864:20::1129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4443B61EC6\n\tfor ;\n\tMon, 3 Apr 2023 10:06:38 +0200 (CEST)","by mail-yw1-x1129.google.com with SMTP id\n\t00721157ae682-5445009c26bso536140227b3.8\n\tfor ;\n\tMon, 03 Apr 2023 01:06:38 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680509200;\n\tbh=+5n8K/PFXNhomcib4LlHAIa+Udxqg6IABeWNjt5WZcY=;\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=iZbVt+uEm1ALbN6VR3Is95k49O7aEha0R7NsIiXoABz7an/udDFp5cRSmizT0VX+z\n\t2k3n/BNvTCTwkf3z6cdWAqfWQVF7SOP8ZSo3b4jCjYRpS3W0fmACftN53BODtQAwwq\n\twWuUqe27Q7SzbYDCS37sQM80edlAaHcIyUfSpDd/pdgkxzxZ862wNyuENWLwbP9MFK\n\tTUczsjpyZ+XCgZQxLXPyEf1gG0ZTf+QE18mPFCPHP4PFRN0y6AActOgQsih55u5L3q\n\tijH9MPG1e7BuKnP50fN1meINVghC8tMU/HJzAB39FQ1MyeiSekJHwMQ/aSQq3ZSh/m\n\tegeCjjwo9QcOA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1680509197;\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=K0HYmUaXSl+0+t5XXdKuwNaVrT3JRcbf0LFrYJWyg3M=;\n\tb=EMbSnI5qq/DN1RcfRn2j8DMyf7fwsf1FKURd1AiLGvq3MBIRFbF41vP6hEACioryKH\n\tbHZIX8y7dSQlEAWbddp86oU6CVvQGONAtgN3eQOcOWNy74w8N+LI4J8YXkzOmOd+IlGi\n\tsH4oAR4gjBx5clMGYW6hOu++5Pit5b5ZZukCPceIqvenlT8/Tp1XQbO4xiuv1J2Q9aho\n\tcK/b7WqxofCHPPjzIGf2oB3KV7+osudNYTPIreV3SmJlu2ROOFzrILzv1P5HgmAKVGhy\n\tUQIysYAYAhSNtFP4cIEH1wJZ4LPLvMBhQOM8LZk+TF5DvDL4ifrDoIWgVJCa7Z8RQWgv\n\tIsKw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EMbSnI5q\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680509197;\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=K0HYmUaXSl+0+t5XXdKuwNaVrT3JRcbf0LFrYJWyg3M=;\n\tb=z7d1f0WvCphWFEDK8FZaf/S2LsTixaPM+7WCp7H7lHk+Ap/rHLGosx1wWsSMFqp3dH\n\tCr611vZhk0VLr9Oe8kXKw/D645soW2pzGHCZOhmbEd/LrUTChghxsxt4JvVr/tC35Bwx\n\tbE76jJ66t6wyuTTr4pKtvJtMtdHVNcJrTCznWFunu0tjfH+06yYhAS/sPahBhrqPBgam\n\tjNBwhGUqnIh1b551PBFD1ifWRiF9dCL0Z8N4NAsiaN6DbXu4JRJLubz18dlYrjmIYojQ\n\tYk84bbhqq2YZ03KA2uZ7GWH7SuRx70tTdmMcUsTpR817MRpQJYJcL2sbeDOKRvGb7Jzs\n\tKktg==","X-Gm-Message-State":"AAQBX9dh6nX+X3vZHEmfow7qkQALQtd5z7ntu2+VGETScmBc6+6ruw6P\n\tI0HKFtOH+jJpf9CgQ9GQiTwywKQrJEa4L8AkJdEcdg==","X-Google-Smtp-Source":"AKy350YMw7eqW8zLC+IjijqO0ae08LE7GnFk20C8TvHj2LdDd1i9KnRVAIn8RNfY2whJtpTjh68Z0ZJdvtVkjAUWyz8=","X-Received":"by 2002:a81:a742:0:b0:541:a0cc:2a09 with SMTP id\n\te63-20020a81a742000000b00541a0cc2a09mr17999627ywh.7.1680509196913;\n\tMon, 03 Apr 2023 01:06:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20230328085521.7409-3-david.plowman@raspberrypi.com>","Date":"Mon, 3 Apr 2023 09:06:35 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: 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":"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":26821,"web_url":"https://patchwork.libcamera.org/comment/26821/","msgid":"<20230403143602.642m34stf3jiyst5@uno.localdomain>","date":"2023-04-03T14:36:02","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: 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 Naush\n\nOn Mon, Apr 03, 2023 at 09:06:35AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Hi David,\n>\n> Thank you for this work.\n>\n> On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel\n> wrote:\n> >\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/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++\n> > 1 file changed, 80 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 13757955..d485b851 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -91,6 +91,8 @@ static const ControlInfoMap::Map ipaControls{\n> > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) },\n> > { &controls::AwbEnable, ControlInfo(false, true) },\n> > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > @@ -230,6 +232,12 @@ private:\n> > /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > std::deque frameLengths_;\n> > Duration lastTimeout_;\n> > +\n> > + /* The current state of flicker avoidance. */\n> > + struct FlickerState {\n> > + int32_t mode;\n> > + Duration customPeriod;\n> > + } flickerState_;\n> > };\n> >\n> > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > break;\n> > }\n> >\n> > + case controls::AE_FLICKER_MODE: {\n> > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > + controller_.getAlgorithm(\"agc\"));\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AeFlickerMode - no AGC algorithm\";\n> > + break;\n> > + }\n> > +\n> > + int32_t mode = ctrl.second.get();\n> > + bool modeValid = true;\n> > +\n> > + switch (mode) {\n> > + case controls::FlickerOff: {\n> > + agc->setFlickerPeriod(0us);\n> > +\n> > + break;\n> > + }\n> > +\n> > + case controls::FlickerFreq50Hz: {\n> > + agc->setFlickerPeriod(10000 * 1.0us);\n> > +\n> > + break;\n> > + }\n> > +\n> > + case controls::FlickerFreq60Hz: {\n> > + agc->setFlickerPeriod(8333.333 * 1.0us);\n> > +\n> > + break;\n> > + }\n> > +\n> > + case controls::FlickerCustom: {\n> > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > +\n> > + break;\n> > + }\n> > +\n> > + default:\n> > + LOG(IPARPI, Error) << \"Flicker mode \" << mode << \" is not supported\";\n> > + modeValid = false;\n> > +\n> > + break;\n> > + }\n> > +\n> > + if (modeValid)\n> > + flickerState_.mode = mode;\n> > +\n> > + break;\n> > + }\n>\n> Perhaps remove the blank lines above the break statements above?\n>\n> > +\n> > + case controls::AE_FLICKER_CUSTOM: {\n> > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > + controller_.getAlgorithm(\"agc\"));\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AeFlickerCustom - no AGC algorithm\";\n> > + break;\n> > + }\n> > +\n> > + uint32_t customPeriod = ctrl.second.get();\n> > + flickerState_.customPeriod = customPeriod * 1.0us;\n> > +\n> > + /*\n> > + * We note that it makes no difference if the mode gets set to \"custom\"\n> > + * first, and the period updated after, or vice versa.\n> > + */\n> > + if (flickerState_.mode == controls::FlickerCustom)\n> > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > +\n> > + break;\n> > + }\n>\n> This is not something to do for this change, but for controls like\n> AeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom,\n> perhaps we can mandate that both controls *must* be set in the same control list\n> to be valid? This would avoid having mostly useless state information stored in\n> the IPA class. Ditto for some of the focus, AGC and AWB controls as well. What\n> do folks think? Is that too restrictive on the application?\n>\n\nThis won't work well for AEGC. We have a pending rework of AEGC\ncontrols where we had separated ExposureTime and AnalogueGain. In this\ncontext is valid to set ExposureTimeMode to Manual and not send a new\nvalue so that the exposure time is fixed and the gain is adjusted to\ncompensate it to implement \"Aperture priority\" (or \"Shutter priority\" for the\nother way around) modes.\n\nDoes this make sense ?\n\n> Other than that:\n> Reviewed-by: Naushir Patuck \n>\n> > +\n> > case controls::AWB_ENABLE: {\n> > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > controller_.getAlgorithm(\"awb\"));\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 294F8C0F2A\n\tfor ;\n\tMon, 3 Apr 2023 14:36:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68D5362724;\n\tMon, 3 Apr 2023 16:36:07 +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 2C2F8626E3\n\tfor ;\n\tMon, 3 Apr 2023 16:36:06 +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 A5652496;\n\tMon, 3 Apr 2023 16:36:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680532567;\n\tbh=VxlbJPxoCNfx2C6mzp5lxSnNDfqDjSY7t5yIXyrMfGs=;\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=sHAk46JyxzML86GwBye/uI57brqLLbQpjRiZKyq54xjN02lfxYG/PSSr+aN1ePZ1t\n\t3cioN1rQH16xfVfsASIrwvp1oM6LqicQE0R7l/HhMmqefxDIUc1qFHSBuvmk9MTTUy\n\t0VwADDN7uJ+gM5RrlfXv60M343t9cOdV4gniWoRycXRkaKI0cN/ncUM6rOJ+7Sjb3u\n\ttC8F4kpvNVybiMU2WnDpxnm54sSHEcdlSAQlylTWOXB8NuFoOk9SAwtrRZ6nVEMrzs\n\tNOBqGB35MQC9BGGQVmd8T20jLM904U45+xLRivXTCd4GJDxRe3xKKCu3CLq7cvkCDK\n\t2+UFYTfef6tpQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680532565;\n\tbh=VxlbJPxoCNfx2C6mzp5lxSnNDfqDjSY7t5yIXyrMfGs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mLV34Bxa5Qp53QL/rDSvFMKiILdc8xs2GVUl5W0+Qzx7NGAcr4HTye7WkrxMeW8/x\n\tq+KijSaPI73gKT9sHzSD96vvy6SpVsa46AcJeP0Hm5PTEFds0MhfX6DRge07ljm35a\n\tDvd2k85GAMbpyzBdXdaEAyOabIK8/LJPNc9uDvaw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mLV34Bxa\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 16:36:02 +0200","To":"Naushir Patuck ","Message-ID":"<20230403143602.642m34stf3jiyst5@uno.localdomain>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-3-david.plowman@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: 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":26831,"web_url":"https://patchwork.libcamera.org/comment/26831/","msgid":"<20230404073710.GS16648@pendragon.ideasonboard.com>","date":"2023-04-04T07:37:10","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Handle\n\tAEC/AGC flicker controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Apr 03, 2023 at 04:36:02PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> On Mon, Apr 03, 2023 at 09:06:35AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel wrote:\n> > >\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\nDavid, do you have plans to implement those ?\n\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++\n> > > 1 file changed, 80 insertions(+)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 13757955..d485b851 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -91,6 +91,8 @@ static const ControlInfoMap::Map ipaControls{\n> > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) },\n> > > { &controls::AwbEnable, ControlInfo(false, true) },\n> > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > @@ -230,6 +232,12 @@ private:\n> > > /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > > std::deque frameLengths_;\n> > > Duration lastTimeout_;\n> > > +\n> > > + /* The current state of flicker avoidance. */\n> > > + struct FlickerState {\n> > > + int32_t mode;\n> > > + Duration customPeriod;\n> > > + } flickerState_;\n> > > };\n> > >\n> > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > break;\n> > > }\n> > >\n> > > + case controls::AE_FLICKER_MODE: {\n> > > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > + controller_.getAlgorithm(\"agc\"));\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AeFlickerMode - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > > +\n> > > + int32_t mode = ctrl.second.get();\n> > > + bool modeValid = true;\n> > > +\n> > > + switch (mode) {\n> > > + case controls::FlickerOff: {\n> > > + agc->setFlickerPeriod(0us);\n> > > +\n\nNo blank line needed.\n\nYou may store the period in a local variable and call\nagc->setFlickerPeriod() once after the switch. Up to you.\n\n> > > + break;\n> > > + }\n> > > +\n> > > + case controls::FlickerFreq50Hz: {\n> > > + agc->setFlickerPeriod(10000 * 1.0us);\n> > > +\n> > > + break;\n> > > + }\n> > > +\n> > > + case controls::FlickerFreq60Hz: {\n> > > + agc->setFlickerPeriod(8333.333 * 1.0us);\n> > > +\n> > > + break;\n> > > + }\n> > > +\n> > > + case controls::FlickerCustom: {\n> > > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > > +\n> > > + break;\n> > > + }\n> > > +\n> > > + default:\n> > > + LOG(IPARPI, Error) << \"Flicker mode \" << mode << \" is not supported\";\n> > > + modeValid = false;\n> > > +\n> > > + break;\n> > > + }\n> > > +\n> > > + if (modeValid)\n> > > + flickerState_.mode = mode;\n> > > +\n> > > + break;\n> > > + }\n> >\n> > Perhaps remove the blank lines above the break statements above?\n\nI should read through before reviewing :-)\n\n> > > +\n> > > + case controls::AE_FLICKER_CUSTOM: {\n> > > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > + controller_.getAlgorithm(\"agc\"));\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AeFlickerCustom - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > > +\n> > > + uint32_t customPeriod = ctrl.second.get();\n> > > + flickerState_.customPeriod = customPeriod * 1.0us;\n> > > +\n> > > + /*\n> > > + * We note that it makes no difference if the mode gets set to \"custom\"\n> > > + * first, and the period updated after, or vice versa.\n> > > + */\n> > > + if (flickerState_.mode == controls::FlickerCustom)\n> > > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > > +\n> > > + break;\n> > > + }\n> >\n> > This is not something to do for this change, but for controls like\n> > AeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom,\n> > perhaps we can mandate that both controls *must* be set in the same control list\n> > to be valid? This would avoid having mostly useless state information stored in\n> > the IPA class. Ditto for some of the focus, AGC and AWB controls as well. What\n> > do folks think? Is that too restrictive on the application?\n\nThe best option is to design controls that can't be used inconsistently.\nThat's a goal that can never be fully reached, so we need to decide on a\nconsistent strategy for cases where the user could get it wrong.\n\nThere can be lots of dependencies between controls. If we decide to\nstart enforcing this kind of rules, it will need to be done globally for\nconsistency. I'm worried it could quickly get out of control, which is\nwhy, so far, inconsistent control values are silently ignored. I'm\nwilling to reconsider this though.\n\n> This won't work well for AEGC. We have a pending rework of AEGC\n> controls where we had separated ExposureTime and AnalogueGain. In this\n> context is valid to set ExposureTimeMode to Manual and not send a new\n> value so that the exposure time is fixed and the gain is adjusted to\n> compensate it to implement \"Aperture priority\" (or \"Shutter priority\" for the\n> other way around) modes.\n> \n> Does this make sense ?\n> \n> > Other than that:\n> > Reviewed-by: Naushir Patuck \n> >\n> > > +\n> > > case controls::AWB_ENABLE: {\n> > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > controller_.getAlgorithm(\"awb\"));","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 90AE7C326B\n\tfor ;\n\tTue, 4 Apr 2023 07:37:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DCDF562724;\n\tTue, 4 Apr 2023 09:37:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 51AB2626DA\n\tfor ;\n\tTue, 4 Apr 2023 09:37:04 +0200 (CEST)","from pendragon.ideasonboard.com (fp76f193f3.tkyc206.ap.nuro.jp\n\t[118.241.147.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E052833F;\n\tTue, 4 Apr 2023 09:37:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680593825;\n\tbh=hiCATSn4irBhVv4NS59RBrL0encm/xOoNQZ47rtD2Wk=;\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=PhYQQf8S3sfO0qBleFIYF5m9mpoZGXTBBea7qTViXpHOgyomaZhqlER8RFWDAYjPO\n\tuSQtQZJq4sCCFSLEsTwMnqpe4J7td6vk1LOph8Yc/Qhia6lSkLHIZT4ZQIBl0zR1zE\n\tbutplyG4mVOCjkV28r/8EDcBky81PUaTlmYlniTEDmPOJ2J0BLjQZQrlKeYWliaHBU\n\tBoWTfkHfDhGvtneQyOEvhmaxUtbXzuZYsUT/3/7K0DJQFjBplv/ikeZ2MbItQJl4/q\n\t4qSa71scMFuIm8XKdq2TDzOA9kVy/3c8MVvgFlnp/DFeIxacvoSjbDezRSmUC2ExLm\n\tECOwBBw3QHv0w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680593824;\n\tbh=hiCATSn4irBhVv4NS59RBrL0encm/xOoNQZ47rtD2Wk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sDcMmrMvB/6WBnwh/03ZBSxD7hDFa9co29FaSes9YMulrd7nqlUq+vbksk+NrSDpa\n\txDkCISl0nW9nqkv3/Qy8CEV35DvcFOJsONQEQcWycIhncPHrXAIlQcH8yQjmlw2Cns\n\tSxE6YMvCSvFcxmW7SuCH7hkd1OWd82c4ujUbviiI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"sDcMmrMv\"; dkim-atps=neutral","Date":"Tue, 4 Apr 2023 10:37:10 +0300","To":"Jacopo Mondi ","Message-ID":"<20230404073710.GS16648@pendragon.ideasonboard.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-3-david.plowman@raspberrypi.com>\n\t\n\t<20230403143602.642m34stf3jiyst5@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230403143602.642m34stf3jiyst5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27312,"web_url":"https://patchwork.libcamera.org/comment/27312/","msgid":"<168626428515.2543357.1774189778422236992@Monstersaurus>","date":"2023-06-08T22:44:45","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Handle\n\tAEC/AGC flicker controls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Resending this to reply to the correct mail thread for patchworks sake:\n\nContent added below is from Maarten in \nMessage-ID <022b6380-7d78-8d1c-20ee-0ae211a65653@lankhorst.se>\n\n\nQuoting David Plowman via libcamera-devel (2023-03-28 09:55:21)\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\nThis series removes the power line flickering on my pi v3 camera when\nusing libcamerasrc (with some modifications there).\n\nTested-by: Maarten Lankhorst \n\n\n\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++\n> 1 file changed, 80 insertions(+)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 13757955..d485b851 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -91,6 +91,8 @@ static const ControlInfoMap::Map ipaControls{\n> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) },\n> { &controls::AwbEnable, ControlInfo(false, true) },\n> { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> @@ -230,6 +232,12 @@ private:\n> /* Track the frame length times over FrameLengthsQueueSize frames. */\n> std::deque frameLengths_;\n> Duration lastTimeout_;\n> +\n> + /* The current state of flicker avoidance. */\n> + struct FlickerState {\n> + int32_t mode;\n> + Duration customPeriod;\n> + } flickerState_;\n> };\n> \n> int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> @@ -962,6 +970,78 @@ void IPARPi::queueRequest(const ControlList &controls)\n> break;\n> }\n> \n> + case controls::AE_FLICKER_MODE: {\n> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> + controller_.getAlgorithm(\"agc\"));\n> + if (!agc) {\n> + LOG(IPARPI, Warning)\n> + << \"Could not set AeFlickerMode - no AGC algorithm\";\n> + break;\n> + }\n> +\n> + int32_t mode = ctrl.second.get();\n> + bool modeValid = true;\n> +\n> + switch (mode) {\n> + case controls::FlickerOff: {\n> + agc->setFlickerPeriod(0us);\n> +\n> + break;\n> + }\n> +\n> + case controls::FlickerFreq50Hz: {\n> + agc->setFlickerPeriod(10000 * 1.0us);\n> +\n> + break;\n> + }\n> +\n> + case controls::FlickerFreq60Hz: {\n> + agc->setFlickerPeriod(8333.333 * 1.0us);\n> +\n> + break;\n> + }\n> +\n> + case controls::FlickerCustom: {\n> + agc->setFlickerPeriod(flickerState_.customPeriod);\n> +\n> + break;\n> + }\n> +\n> + default:\n> + LOG(IPARPI, Error) << \"Flicker mode \" << mode << \" is not supported\";\n> + modeValid = false;\n> +\n> + break;\n> + }\n> +\n> + if (modeValid)\n> + flickerState_.mode = mode;\n> +\n> + break;\n> + }\n> +\n> + case controls::AE_FLICKER_CUSTOM: {\n> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> + controller_.getAlgorithm(\"agc\"));\n> + if (!agc) {\n> + LOG(IPARPI, Warning)\n> + << \"Could not set AeFlickerCustom - no AGC algorithm\";\n> + break;\n> + }\n> +\n> + uint32_t customPeriod = ctrl.second.get();\n> + flickerState_.customPeriod = customPeriod * 1.0us;\n> +\n> + /*\n> + * We note that it makes no difference if the mode gets set to \"custom\"\n> + * first, and the period updated after, or vice versa.\n> + */\n> + if (flickerState_.mode == controls::FlickerCustom)\n> + agc->setFlickerPeriod(flickerState_.customPeriod);\n> +\n> + break;\n> + }\n> +\n> case controls::AWB_ENABLE: {\n> RPiController::AwbAlgorithm *awb = dynamic_cast(\n> controller_.getAlgorithm(\"awb\"));\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 B4610C31E9\n\tfor ;\n\tThu, 8 Jun 2023 22:44:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2867461E5F;\n\tFri, 9 Jun 2023 00:44:50 +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 E199E609FF\n\tfor ;\n\tFri, 9 Jun 2023 00:44:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD0C0DE5;\n\tFri, 9 Jun 2023 00:44:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1686264290;\n\tbh=PnMMp2X3aRV/C5gU/vEk3KDmwtYWczh/gAfeqcHnYPM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=lZ24wcjnOph1Am2Hrv37nI2oGsfOK2FBujYMsQlYTzIXBPWD//gsWRR+QGUsnUe/E\n\t47vYqffpgsrozCI6GjlwyRG8DkQWx0d/nDbPGA3EQ/E8bPUV//lCW0GilfQ1Jw+eiV\n\twQaLDTthgIdXCmL9iCIfeKR2ugEtOPCv4C1jpZPc05yIZ90mUZQ/Yb66DuhSEZKpkY\n\tJ5me45XO4ZYbuAJ8K2SSTnvQldwZRB999zyCNX3CyfaL1oIX1o0Gh4sJrLvoWiU0Jt\n\tQuykb4vDmNihyHk+/NB6T9fk+pIhrGX4sN8AZtxLRAcHW62ZKA5Cc5sewKCYrgALnN\n\ta9mnnRguBZ1xw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1686264260;\n\tbh=PnMMp2X3aRV/C5gU/vEk3KDmwtYWczh/gAfeqcHnYPM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=EojD51hKJXHTof1rBmMkUFjB1X0pvCLO1pLxPvO0Dts0XIkR+CMr1U9f643jEEK1y\n\tuZGAXp/N0FvU+/LpfPYuVWxKjbBVZoWX/aSznAAgW/Teb++pmZvZN6lEiM164Oiwgb\n\tkaqC5vZdQ83JowgZ4hrhee8TFkySPvOq/+wdHNEs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EojD51hK\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230328085521.7409-3-david.plowman@raspberrypi.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-3-david.plowman@raspberrypi.com>","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org, Maarten Lankhorst ","Date":"Thu, 08 Jun 2023 23:44:45 +0100","Message-ID":"<168626428515.2543357.1774189778422236992@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27449,"web_url":"https://patchwork.libcamera.org/comment/27449/","msgid":"<20230703095432.GB14933@pendragon.ideasonboard.com>","date":"2023-07-03T09:54:32","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Handle\n\tAEC/AGC flicker controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Apr 04, 2023 at 10:37:10AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Mon, Apr 03, 2023 at 04:36:02PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > On Mon, Apr 03, 2023 at 09:06:35AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel wrote:\n> > > >\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> David, do you have plans to implement those ?\n\nAnd what would it take to implement it ?\n\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++\n> > > > 1 file changed, 80 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 13757955..d485b851 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -91,6 +91,8 @@ static const ControlInfoMap::Map ipaControls{\n> > > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > > > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > > > + { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> > > > + { &controls::AeFlickerCustom, ControlInfo(100, 1000000) },\n> > > > { &controls::AwbEnable, ControlInfo(false, true) },\n> > > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > > @@ -230,6 +232,12 @@ private:\n> > > > /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > > > std::deque frameLengths_;\n> > > > Duration lastTimeout_;\n> > > > +\n> > > > + /* The current state of flicker avoidance. */\n> > > > + struct FlickerState {\n> > > > + int32_t mode;\n> > > > + Duration customPeriod;\n> > > > + } flickerState_;\n> > > > };\n> > > >\n> > > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > > > @@ -962,6 +970,78 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > break;\n> > > > }\n> > > >\n> > > > + case controls::AE_FLICKER_MODE: {\n> > > > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > + controller_.getAlgorithm(\"agc\"));\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AeFlickerMode - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + int32_t mode = ctrl.second.get();\n> > > > + bool modeValid = true;\n> > > > +\n> > > > + switch (mode) {\n> > > > + case controls::FlickerOff: {\n> > > > + agc->setFlickerPeriod(0us);\n> > > > +\n> \n> No blank line needed.\n> \n> You may store the period in a local variable and call\n> agc->setFlickerPeriod() once after the switch. Up to you.\n> \n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + case controls::FlickerFreq50Hz: {\n> > > > + agc->setFlickerPeriod(10000 * 1.0us);\n> > > > +\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + case controls::FlickerFreq60Hz: {\n> > > > + agc->setFlickerPeriod(8333.333 * 1.0us);\n> > > > +\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + case controls::FlickerCustom: {\n> > > > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > > > +\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + default:\n> > > > + LOG(IPARPI, Error) << \"Flicker mode \" << mode << \" is not supported\";\n> > > > + modeValid = false;\n> > > > +\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + if (modeValid)\n> > > > + flickerState_.mode = mode;\n> > > > +\n> > > > + break;\n> > > > + }\n> > >\n> > > Perhaps remove the blank lines above the break statements above?\n> \n> I should read through before reviewing :-)\n> \n> > > > +\n> > > > + case controls::AE_FLICKER_CUSTOM: {\n> > > > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > + controller_.getAlgorithm(\"agc\"));\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AeFlickerCustom - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + uint32_t customPeriod = ctrl.second.get();\n> > > > + flickerState_.customPeriod = customPeriod * 1.0us;\n> > > > +\n> > > > + /*\n> > > > + * We note that it makes no difference if the mode gets set to \"custom\"\n> > > > + * first, and the period updated after, or vice versa.\n> > > > + */\n> > > > + if (flickerState_.mode == controls::FlickerCustom)\n> > > > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > > > +\n> > > > + break;\n> > > > + }\n> > >\n> > > This is not something to do for this change, but for controls like\n> > > AeFlickerMode::FlickerCustom which require a value set in AeFlickerCustom,\n> > > perhaps we can mandate that both controls *must* be set in the same control list\n> > > to be valid? This would avoid having mostly useless state information stored in\n> > > the IPA class. Ditto for some of the focus, AGC and AWB controls as well. What\n> > > do folks think? Is that too restrictive on the application?\n> \n> The best option is to design controls that can't be used inconsistently.\n> That's a goal that can never be fully reached, so we need to decide on a\n> consistent strategy for cases where the user could get it wrong.\n> \n> There can be lots of dependencies between controls. If we decide to\n> start enforcing this kind of rules, it will need to be done globally for\n> consistency. I'm worried it could quickly get out of control, which is\n> why, so far, inconsistent control values are silently ignored. I'm\n> willing to reconsider this though.\n> \n> > This won't work well for AEGC. We have a pending rework of AEGC\n> > controls where we had separated ExposureTime and AnalogueGain. In this\n> > context is valid to set ExposureTimeMode to Manual and not send a new\n> > value so that the exposure time is fixed and the gain is adjusted to\n> > compensate it to implement \"Aperture priority\" (or \"Shutter priority\" for the\n> > other way around) modes.\n> > \n> > Does this make sense ?\n> > \n> > > Other than that:\n> > > Reviewed-by: Naushir Patuck \n> > >\n> > > > +\n> > > > case controls::AWB_ENABLE: {\n> > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > controller_.getAlgorithm(\"awb\"));","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 CBBB4BE175\n\tfor ;\n\tMon, 3 Jul 2023 09:54:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 263C2628BE;\n\tMon, 3 Jul 2023 11:54:33 +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 DB22B61E37\n\tfor ;\n\tMon, 3 Jul 2023 11:54:31 +0200 (CEST)","from pendragon.ideasonboard.com (85-160-45-219.reb.o2.cz\n\t[85.160.45.219])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D686558;\n\tMon, 3 Jul 2023 11:53:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688378073;\n\tbh=ECDg2E+iRaVldOGoIdrMtV3b88PYe+tYxh4bIfjk2xQ=;\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=SgNM8VC9lq8O9NxioMsptleQE7H57zrG86HccShGWOIFpiBpLfdzWHUmSzJMlbimT\n\tPXfbsYI9zGepVNKSVBA6Jzvw0NFr2F6oLXDO0RePhGHuwCRqw611N1cBCk8ltybmbZ\n\tN7fIa9v6xibC41JXoc4/fTItufFStxHN/9OYGIm7/9TiqfFldDnWOkIDormwAfnyjb\n\t67sq4aT1xymAL8s59J8GU8z9NVkEWGjIaTC4kK/G+tLJeD3xigoUT4kJCYeHXNK3eQ\n\tsaoNxjsEjXVJqcSnm8loLB1gW+7yALMkyg6ENNyCTLyEkJ54WCV5EbccuvYVh4lTjP\n\thEhwkkX4z68zw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1688378028;\n\tbh=ECDg2E+iRaVldOGoIdrMtV3b88PYe+tYxh4bIfjk2xQ=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=a+J7C+vt/yTzuRXRbbRo2iI1C1Chbk52YbulZONFBmw1DWgAy41ez/2yJqIFMbbgW\n\tWFmCEGN7iP9sTNmO0rJkjoCrj+FlRcMP4AkiHVimfBCq4GkzL8sGVUPpX6PmUf8YWN\n\tB1GuJO4nKo33DUNEnWZcVwjcOilmjoMClS4j+c38="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"a+J7C+vt\"; dkim-atps=neutral","Date":"Mon, 3 Jul 2023 12:54:32 +0300","To":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20230703095432.GB14933@pendragon.ideasonboard.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-3-david.plowman@raspberrypi.com>\n\t\n\t<20230403143602.642m34stf3jiyst5@uno.localdomain>\n\t<20230404073710.GS16648@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230404073710.GS16648@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]