[{"id":27571,"web_url":"https://patchwork.libcamera.org/comment/27571/","msgid":"","date":"2023-07-18T07:09:26","subject":"Re: [libcamera-devel] [PATCH v4 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 Thu, Jul 13, 2023 at 04:12:18PM +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\nIf so you shouldn't register\n\n\t{ &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n\notherwise AeFlickerMode::FlickerAuto will be exposed to applications ?\n\n\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..81d65ea5 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -61,6 +61,8 @@ 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(controls::AeFlickerModeValues) },\n> +\t{ &controls::AeFlickerCustom, 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 +99,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 +814,66 @@ 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> +\t\t\t}\n\nDo you need braces ?\n\n> +\n> +\t\t\tcase controls::FlickerCustom: {\n> +\t\t\t\tagc->setFlickerPeriod(flickerState_.customPeriod);\n> +\n> +\t\t\t\tbreak;\n> +\t\t\t}\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_CUSTOM: {\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 AeFlickerCustom - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tuint32_t customPeriod = ctrl.second.get();\n> +\t\t\tflickerState_.customPeriod = customPeriod * 1.0us;\n> +\n> +\t\t\t/*\n> +\t\t\t * We note that it makes no difference if the mode gets set to \"custom\"\n> +\t\t\t * first, and the period updated after, or vice versa.\n> +\t\t\t */\n\nTrue, but what if the app never provides a value for\ncontrols::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to\nsome sensible default ?\n\n> +\t\t\tif (flickerState_.mode == controls::FlickerCustom)\n> +\t\t\t\tagc->setFlickerPeriod(flickerState_.customPeriod);\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..22823176 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 customPeriod;\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 44076BDC71\n\tfor ;\n\tTue, 18 Jul 2023 07:09:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD6CD628C0;\n\tTue, 18 Jul 2023 09:09:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71AA461E2A\n\tfor ;\n\tTue, 18 Jul 2023 09:09:31 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD0E1968;\n\tTue, 18 Jul 2023 09:08:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689664172;\n\tbh=6v+Lpg/k/vWRbATE2HdPYLKULVf+phDaexPTeq1/tQY=;\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=wUdvvFmqyZYdOrU/UaZdR6iEJMxB4fU6sDsgKzoMImwRmlo10aXWDhEIBpxIUuhlJ\n\tTwKDfylhafh1QGK8P3hhxTSfi63qqloE0yNszsvqnAy1gNcec9dsk0FmUmMnSBllqB\n\tfiRhH0Qn0WlNjeRhDmVggbEcLSVjW/GYaRuKid6dkPIlwpoaccmTJxpy+7boXMNAd3\n\tHJGU3X+wSiHrbdw8S3m6c7Bx3yqUhUg/GN/kIBY1yp8N/7qz+x/hmijf93XUS7J8Zm\n\tHN90t0yw3uTbhJgMtsTDeTDbJWazVs1FN5cZVFq2ypD0PzyDVsvUFYgiV5yOrRRcuc\n\t3McfPZEiN18ZA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689664118;\n\tbh=6v+Lpg/k/vWRbATE2HdPYLKULVf+phDaexPTeq1/tQY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=etgIY/MRCun+iZZ8aFeQ+Fn4awl8eZunc1tZFwzFY+3SuXA4v1JewOzKuVEvHY5Qh\n\tA+q+V7MkcqlcJw3M9FEyjIeAUm8Vm6Vwft2LZGHS1arnJ4ziY82qDecP07mR5onlBo\n\tqgEpx1az+XK7/ynSJxURfqgIpUgEafKdOQLq3UsM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"etgIY/MR\"; dkim-atps=neutral","Date":"Tue, 18 Jul 2023 09:09:26 +0200","To":"David Plowman ","Message-ID":"","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230713151218.26045-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":27575,"web_url":"https://patchwork.libcamera.org/comment/27575/","msgid":"","date":"2023-07-18T08:24:24","subject":"Re: [libcamera-devel] [PATCH v4 2/2] ipa: rpi: common: Handle\n\tAEC/AGC flicker controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the comments.\n\nOn Tue, 18 Jul 2023 at 08:09, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Thu, Jul 13, 2023 at 04:12:18PM +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>\n> If so you shouldn't register\n>\n> { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n>\n> otherwise AeFlickerMode::FlickerAuto will be exposed to applications ?\n\nIndeed, it might be better not to expose the auto value!\n\n>\n>\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..81d65ea5 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -61,6 +61,8 @@ 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::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > @@ -97,7 +99,7 @@ namespace ipa::RPi {\n> >\n> > IpaBase::IpaBase()\n> > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),\n> > - firstStart_(true)\n> > + firstStart_(true), flickerState_({ 0, 0s })\n> > {\n> > }\n> >\n> > @@ -812,6 +814,66 @@ void IpaBase::applyControls(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> Do you need braces ?\n\nSure, I can remove them!\n\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>\n> True, but what if the app never provides a value for\n> controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to\n> some sensible default ?\n\nIn our implementation we use the value 0 to mean \"no flicker\ncancellection\", so setting the mode to custom/manual without a flicker\nperiod will cause it to do nothing (until you set one). I'm OK with\nthis behaviour unless anyone objects, though I don't feel strongly!\nI'd rather document that setting custom/manual mode without a period\nleads to undefined behaviour rather than mandating a particular value,\nI think, as not every PH may wish to do the same. What do you think?\n\nThanks!\nDavid\n\n>\n> > + if (flickerState_.mode == controls::FlickerCustom)\n> > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > +\n> > + break;\n> > + }\n> > +\n> > case controls::AWB_ENABLE: {\n> > /* Silently ignore this control for a mono sensor. */\n> > if (monoSensor_)\n> > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > index 39d00760..22823176 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> > /* Frame duration (1/fps) limits. */\n> > utils::Duration minFrameDuration_;\n> > utils::Duration maxFrameDuration_;\n> > +\n> > + /* The current state of flicker avoidance. */\n> > + struct FlickerState {\n> > + int32_t mode;\n> > + utils::Duration customPeriod;\n> > + } 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 16229BDC71\n\tfor ;\n\tTue, 18 Jul 2023 08:24:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 563E6628C0;\n\tTue, 18 Jul 2023 10:24:37 +0200 (CEST)","from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com\n\t[IPv6:2607:f8b0:4864:20::f29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0411861E2A\n\tfor ;\n\tTue, 18 Jul 2023 10:24:36 +0200 (CEST)","by mail-qv1-xf29.google.com with SMTP id\n\t6a1803df08f44-635e3ceb152so21105996d6.2\n\tfor ;\n\tTue, 18 Jul 2023 01:24:35 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689668677;\n\tbh=7obsakkICuIJTiwaNWHZk6VXrM203PbS2pDI4QIJjc8=;\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=uNVci2GRgiczUuo9/r8KrN0og0F4Ih45+x5cU5XU6cBd5BAhP2X6eH0I8pTBjpcxL\n\tXZ+Xm5McLyfm1zJJu659ic7UcdzAXWlMcJG5Y5nIoPkrspcd70E38tPabvbIwsUHTG\n\tFzIA7mmABIl1fLMCf2e19RLFc+wwcVXsc62ZMAxY8Majoo6A90uWPg+jPkT23LHpzZ\n\tIVDIC/g5DudExCS++8M8jPx/gC1Qg3amfDJEi82r6bFl03mOJXUyS65ek6PbJSLQgz\n\t62cMZi9zK/arvYWKqmvgxfbKAfT17fOBxgWrLhouh+DeRYITtqm0hxLwPc085HXUVs\n\t1kH+72cdlFA0Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689668675; x=1692260675;\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=SQ5EsmlJHnT3BSnoCUReTnOxS0FqwZH+N+kOMWRJEik=;\n\tb=dwoBhwIL2oBUV2+COboMEcnZgDPMDmPirDrreKNONx0KCdUIo518jqUC1DImqea+Y9\n\tcDhOj0DBpH7cwcjVaIU62b2sPZ5krjw7rGfg6kX4TF74TmlYPv+qj4+EYS52gER6wDeQ\n\tjqMbIo9ICao6BfCOyET/z6hsCozHLNTqmrxMh1fydVPqtw7LeNGJ6utvqI/2AqpXGJSQ\n\tva1UtuKYR6ZzuDWgmnoGtZz7TpWtr+vvq6yY6PPg7AfgM/O3KX5KZv8vH02eKvdFtEIR\n\tJVccE8gSSOEvTQE6Tx2pS3o0Qoz+dbBQaM9XMXw/hCq+2bvMhmR+3/pr6SMHPSfoTbi1\n\tQhHw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"dwoBhwIL\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689668675; x=1692260675;\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=SQ5EsmlJHnT3BSnoCUReTnOxS0FqwZH+N+kOMWRJEik=;\n\tb=iUg9sj0tX2FY83wNnrAMBB2n2HEcMx61NpvmbB7bN3ZI/+GLhVrNrlfRUbHKSxitXs\n\tbrjLJsyUifx9VfUA8iRei6GY+2CbsoupQBNmL1sEhOQZy/ElHNZKQ91qkFoKqXhFaTpX\n\tCJW5VhzJiovIXjqQO+6rQzgo9TIXV+uHzeml1k2sSLt4IUHDhTfeSCqC5j6u2JHnk82I\n\tufNHD04Udn9TskzvJ0pcmyoHbJHEvAci8bC1xVcFHVVnrY5Q8nv13uK76nl3rrIHEQpj\n\t5uTTssWDUTNjqBipcKhYDPf8Eou8W31Jgga/8IX2xdu5MfZBG2TZLS4mY73AQloujm5w\n\tiZHQ==","X-Gm-Message-State":"ABy/qLYWn+oRk9b2amqEC8mTlfePkJlF5USvMeT4hot+r7P8+ISsPU9c\n\tJU+QaDDuhvNk/mI2bvjawejo1MY5fgBppvZWloSyCgEKk3ka5rQD","X-Google-Smtp-Source":"APBJJlEV+ab2G9AsttuTwIsldwvbeaYPCktAZ70rXAdXoUl7HR7wEbmaUUptUvVcu+cs6uR+krbf6hBNduU5hqPELDs=","X-Received":"by 2002:a0c:f344:0:b0:63c:6bcc:5a2b with SMTP id\n\te4-20020a0cf344000000b0063c6bcc5a2bmr8880886qvm.46.1689668674733;\n\tTue, 18 Jul 2023 01:24:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-3-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Tue, 18 Jul 2023 09:24:24 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 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":"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":27576,"web_url":"https://patchwork.libcamera.org/comment/27576/","msgid":"<168967011948.265056.4933661704595756930@Monstersaurus>","date":"2023-07-18T08:48:39","subject":"Re: [libcamera-devel] [PATCH v4 2/2] ipa: rpi: common: 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":"Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24)\n> Hi Jacopo\n> \n> Thanks for the comments.\n> \n> On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi\n> wrote:\n> >\n> > Hi David\n> >\n> > On Thu, Jul 13, 2023 at 04:12:18PM +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> >\n> > If so you shouldn't register\n> >\n> > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> >\n> > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ?\n> \n> Indeed, it might be better not to expose the auto value!\n> \n> >\n> >\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..81d65ea5 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -61,6 +61,8 @@ 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::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > @@ -97,7 +99,7 @@ namespace ipa::RPi {\n> > >\n> > > IpaBase::IpaBase()\n> > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),\n> > > - firstStart_(true)\n> > > + firstStart_(true), flickerState_({ 0, 0s })\n> > > {\n> > > }\n> > >\n> > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(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> > Do you need braces ?\n> \n> Sure, I can remove them!\n> \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> >\n> > True, but what if the app never provides a value for\n> > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to\n> > some sensible default ?\n> \n> In our implementation we use the value 0 to mean \"no flicker\n> cancellection\", so setting the mode to custom/manual without a flicker\n> period will cause it to do nothing (until you set one). I'm OK with\n> this behaviour unless anyone objects, though I don't feel strongly!\n> I'd rather document that setting custom/manual mode without a period\n> leads to undefined behaviour rather than mandating a particular value,\n> I think, as not every PH may wish to do the same. What do you think?\n\n\nI had wondered if we could get this down to a single control by having\n'0' as no flicker cancellation - but we'd have no way to express 'auto'\nso I think we do still need two, and it lets us report in the metadata\nif the value was determined automatically or explicitly I guess? (Though\nthe app should know that anyway as it would know if it set the manual\nvalue?)\n\nThrowing paint on the bike shed I feel like 'AeFlickerPeriod' would be\nbetter suited for the control name as then it would be usable to report\nthe period back in the metadata when it's detected with 'Auto' mode.\n\nWhat limits will be imposed for the period ? I think setting\nAeFlickerMode to 'Custom' (or 'Manual'?) and not setting an\n{AeFlickerCustom,AeFlickerPeriod} control should be the same as not\nchanging anything, but I expect that maybe the ControlInfo might have\nlimits that would preclude setting a period of '0' ?\n\nIt might be reasonable to say setting AeFlickerMode to Manual/Custom\nwill only take effect when the corresponding AeFlickerPeriod control is\napplied ?\n\n\n--\nKieran\n\n\n> \n> Thanks!\n> David\n> \n> >\n> > > + if (flickerState_.mode == controls::FlickerCustom)\n> > > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > > +\n> > > + break;\n> > > + }\n> > > +\n> > > case controls::AWB_ENABLE: {\n> > > /* Silently ignore this control for a mono sensor. */\n> > > if (monoSensor_)\n> > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > > index 39d00760..22823176 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> > > /* Frame duration (1/fps) limits. */\n> > > utils::Duration minFrameDuration_;\n> > > utils::Duration maxFrameDuration_;\n> > > +\n> > > + /* The current state of flicker avoidance. */\n> > > + struct FlickerState {\n> > > + int32_t mode;\n> > > + utils::Duration customPeriod;\n> > > + } 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 66D45BDC71\n\tfor ;\n\tTue, 18 Jul 2023 08:48:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4AE6628C0;\n\tTue, 18 Jul 2023 10:48:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7822061E2A\n\tfor ;\n\tTue, 18 Jul 2023 10:48:42 +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 D56FE838;\n\tTue, 18 Jul 2023 10:47:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689670123;\n\tbh=WppbtjzV172BLuVa5KiRo1jU0j8orIaNsBy1L6bbX/U=;\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:Cc:\n\tFrom;\n\tb=179honcWtFK0bneL/Z5C6ijf3u03m1J+O+zm9ba24wWt+A9FPSeXVOg9Rhgyt31Vl\n\tGVKMjch1EJkwBiN6hGZ/kkWS5e6rAhEBcB8ee5AR6J7PNf1CO2v+O3P7vLKAIeCxqZ\n\triRva984LykUI6QxknyVv0lPPL+ZQulkbEWQzkAQlkm1OpplZSjG8SHZ7nE0C5x0WS\n\tJvMlp2YGG40UcdSKG+utZBhxUV3tSNwd99eHOxyn8MqR1HrHu0gc4n0mI10GC+avRu\n\ti1Rdndejo6/gSzCLXv8s/7l6NUJoSpO2c3LXLcpj+WgWsFc4E3Q8Gim9FuFdD6d1XG\n\tIlYTb3CGBG7aw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689670069;\n\tbh=WppbtjzV172BLuVa5KiRo1jU0j8orIaNsBy1L6bbX/U=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JW4TFFm/XTf0erzj23gV/GMjYW77O6P9RTYx72H9CHQ/JMU7iNAOBBtg/dskLkx48\n\t/gO+qdoR9rzqwyvB/5YUJ3AIGW6vgcuPWQZ132RO9EpBnU0mRavJMclQh1etCyQwDt\n\tMia6aB8Ekw2IbBBzCaPWcAFJr6rrsdRqrq1tyKtA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JW4TFFm/\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-3-david.plowman@raspberrypi.com>\n\t\n\t","To":"David Plowman ,\n\tDavid Plowman via libcamera-devel , \n\tJacopo Mondi ","Date":"Tue, 18 Jul 2023 09:48:39 +0100","Message-ID":"<168967011948.265056.4933661704595756930@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27578,"web_url":"https://patchwork.libcamera.org/comment/27578/","msgid":"","date":"2023-07-18T09:13:42","subject":"Re: [libcamera-devel] [PATCH v4 2/2] ipa: rpi: common: Handle\n\tAEC/AGC flicker controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nOn Tue, 18 Jul 2023 at 09:48, Kieran Bingham\n wrote:\n>\n> Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24)\n> > Hi Jacopo\n> >\n> > Thanks for the comments.\n> >\n> > On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi David\n> > >\n> > > On Thu, Jul 13, 2023 at 04:12:18PM +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> > >\n> > > If so you shouldn't register\n> > >\n> > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> > >\n> > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ?\n> >\n> > Indeed, it might be better not to expose the auto value!\n> >\n> > >\n> > >\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..81d65ea5 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -61,6 +61,8 @@ 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::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > > @@ -97,7 +99,7 @@ namespace ipa::RPi {\n> > > >\n> > > > IpaBase::IpaBase()\n> > > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),\n> > > > - firstStart_(true)\n> > > > + firstStart_(true), flickerState_({ 0, 0s })\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(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> > > Do you need braces ?\n> >\n> > Sure, I can remove them!\n> >\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> > >\n> > > True, but what if the app never provides a value for\n> > > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to\n> > > some sensible default ?\n> >\n> > In our implementation we use the value 0 to mean \"no flicker\n> > cancellection\", so setting the mode to custom/manual without a flicker\n> > period will cause it to do nothing (until you set one). I'm OK with\n> > this behaviour unless anyone objects, though I don't feel strongly!\n> > I'd rather document that setting custom/manual mode without a period\n> > leads to undefined behaviour rather than mandating a particular value,\n> > I think, as not every PH may wish to do the same. What do you think?\n>\n>\n> I had wondered if we could get this down to a single control by having\n> '0' as no flicker cancellation - but we'd have no way to express 'auto'\n> so I think we do still need two, and it lets us report in the metadata\n> if the value was determined automatically or explicitly I guess? (Though\n> the app should know that anyway as it would know if it set the manual\n> value?)\n\nYes, I think we do still want a separate control.\n\n>\n> Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be\n> better suited for the control name as then it would be usable to report\n> the period back in the metadata when it's detected with 'Auto' mode.\n\nI agree that 'AeFlickerPeriod' is a better name, particularly if the\n'custom' mode becomes the 'manual' mode.\n\n>\n> What limits will be imposed for the period ? I think setting\n> AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an\n> {AeFlickerCustom,AeFlickerPeriod} control should be the same as not\n> changing anything, but I expect that maybe the ControlInfo might have\n> limits that would preclude setting a period of '0' ?\n\nI guess implementations get to choose their own limits though, for the\nmost part, I see no reason not to allow them to be very wide.\nExtremely short or extremely long periods are going to be increasingly\nunlikely IRL, of course.\n\n>\n> It might be reasonable to say setting AeFlickerMode to Manual/Custom\n> will only take effect when the corresponding AeFlickerPeriod control is\n> applied ?\n\nYes, we could say that setting the mode to manual has no effect until\na period is also set. Though of course I assume the period is\n\"sticky\", so once you've set it, you can go to and fro to manual mode\nwithout re-sending the said period.\n\nI guess we have to ask what happens if you go from auto to manual\nmode, having never set a period. What happens then? TBH, I think I'd\nbe quite happy to say that selecting manual mode, without ever setting\na period, is the same as being \"off\". Thoughts?\n\nDavid\n\n>\n>\n> --\n> Kieran\n>\n>\n> >\n> > Thanks!\n> > David\n> >\n> > >\n> > > > + if (flickerState_.mode == controls::FlickerCustom)\n> > > > + agc->setFlickerPeriod(flickerState_.customPeriod);\n> > > > +\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > case controls::AWB_ENABLE: {\n> > > > /* Silently ignore this control for a mono sensor. */\n> > > > if (monoSensor_)\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > > > index 39d00760..22823176 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> > > > /* Frame duration (1/fps) limits. */\n> > > > utils::Duration minFrameDuration_;\n> > > > utils::Duration maxFrameDuration_;\n> > > > +\n> > > > + /* The current state of flicker avoidance. */\n> > > > + struct FlickerState {\n> > > > + int32_t mode;\n> > > > + utils::Duration customPeriod;\n> > > > + } 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 7686ABDC71\n\tfor ;\n\tTue, 18 Jul 2023 09:13:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9383C628BC;\n\tTue, 18 Jul 2023 11:13:56 +0200 (CEST)","from mail-qv1-xf2e.google.com (mail-qv1-xf2e.google.com\n\t[IPv6:2607:f8b0:4864:20::f2e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CFF061E2A\n\tfor ;\n\tTue, 18 Jul 2023 11:13:54 +0200 (CEST)","by mail-qv1-xf2e.google.com with SMTP id\n\t6a1803df08f44-635e0e6b829so37541406d6.0\n\tfor ;\n\tTue, 18 Jul 2023 02:13:54 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689671636;\n\tbh=gjtHYjNviEeWm8Hk9oeBQShtNYq/a7beS8HlSXhGm6s=;\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=ZeCjiYT10CS5AynKOHPJohWt1u8xjrRqtJw+6IuneSEb9qMcbW2R0RrHFv7Sd6fOP\n\tSWTVsDcFemTp+nGyWcEyicU1oZ6dgBXl0rEEEYevaHl89IpCjy4Afve0eXVVWg7+r4\n\t0S1eYQQV1sQzma5sPn1C2DWEg6dikb9FvTECUeNWFj5J6zZIk6axJiRTlxD+XjqMug\n\tKckyQfz/Rt30szsE6CedddBP/Dla2MDdUGXfKJdun86HY7lh/h2LaWDa5Ubblk1mAE\n\tUOxvtyVD2A6BavAf1vMHZfmIF8R3Tu6+bMHK6iJZ77EsDU6mOada4TDdrdlGGnsS7R\n\t2eemMUOoEtvTA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689671633; x=1692263633;\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=gLLS2ktmW5nio7fF9r4yBmd4oEYevFhJBHLF1lNET3Y=;\n\tb=Np435Zh2pLOIYAdz8bsdeZgMRtUzUCUBxIC5q++NZGzrGmW29zWf5XI00uhtggBXtC\n\tCPxomJBcbiWix7pLZCs5JsHZR0ipRd++zkPjVxVHJlSlMdhW7Q6nkVYrKuG7wsD0Tapk\n\tY72YM/XMU947QMx3GhH9LdY6izVAqmIPlOpjEKEwVxA2vgKEQz5b5grVOIbPi6a1sxbs\n\t1uU3d588vDk1R1aYDn2SWNe6i47aqzsGQLLhV4XUt3fhXEYYmqhgLvp3bDejL1Q3l1v8\n\t6xzInFw191yPmCRykn8135T026xZtzI1CqSXII7czKl89tObderNfBhFBpeyL02y5ehe\n\tKyJA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Np435Zh2\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689671633; x=1692263633;\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=gLLS2ktmW5nio7fF9r4yBmd4oEYevFhJBHLF1lNET3Y=;\n\tb=W57f/lCKuhV+/kqggz9BXNSHSqS7rxYxK9fUMKlryRkb37foyD+7nIT0WSrHHW0iw6\n\tyVmcVTuHs0MjZNu1sSRQpN1HV4uGdOA7Sq6Ki1DqnAQaozA9I8Ftd81070397mzUK0Qv\n\tRIsUCTIdEnXOjTrruc8PR32kOoPMPSYj1v0TLjZZTYG8gEFf9CJsdlfcWKtppMmqB4IP\n\tWnKNWUclHmje58ATLLxJMV8GiR0o9uOwZ0oujG0smlslH006SHtw+qpTZr/vnZv+hn12\n\tlv4Eriwxyblmy9CdkMgsSoiMLtWtro9pTQFX66r5APUGk8eJNUFoVUc4ia64+LlUb+dp\n\tvi4w==","X-Gm-Message-State":"ABy/qLbKQqZZV/UHNWuwL0GbacIXxkKkLckEtMYmmHrSN6vFlJcKee3G\n\tuw+kC+VONcRqDPsZ8t6DX/VFi6ILYLqX1s8ndEMUWht/Iw1a1ofM","X-Google-Smtp-Source":"APBJJlF2LGDgmWGMUTBRZj+4v6fQI4bk7MbEOUeuivHhmwRhOqxZnu1Gm7ljjEbLXCrBIAxpmmLnBKl74ZkFfKX9LgE=","X-Received":"by 2002:a0c:b55a:0:b0:635:e2ca:7102 with SMTP id\n\tw26-20020a0cb55a000000b00635e2ca7102mr13143827qvd.61.1689671633290;\n\tTue, 18 Jul 2023 02:13:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-3-david.plowman@raspberrypi.com>\n\t\n\t\n\t<168967011948.265056.4933661704595756930@Monstersaurus>","In-Reply-To":"<168967011948.265056.4933661704595756930@Monstersaurus>","Date":"Tue, 18 Jul 2023 10:13:42 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"Jacopo Mondi ,\n\tDavid Plowman via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27581,"web_url":"https://patchwork.libcamera.org/comment/27581/","msgid":"<168967896220.265056.12325288666562701853@Monstersaurus>","date":"2023-07-18T11:16:02","subject":"Re: [libcamera-devel] [PATCH v4 2/2] ipa: rpi: common: 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":"Quoting David Plowman (2023-07-18 10:13:42)\n> Hi Kieran\n> \n> On Tue, 18 Jul 2023 at 09:48, Kieran Bingham\n> wrote:\n> >\n> > Quoting David Plowman via libcamera-devel (2023-07-18 09:24:24)\n> > > Hi Jacopo\n> > >\n> > > Thanks for the comments.\n> > >\n> > > On Tue, 18 Jul 2023 at 08:09, Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi David\n> > > >\n> > > > On Thu, Jul 13, 2023 at 04:12:18PM +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> > > >\n> > > > If so you shouldn't register\n> > > >\n> > > > { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },\n> > > >\n> > > > otherwise AeFlickerMode::FlickerAuto will be exposed to applications ?\n> > >\n> > > Indeed, it might be better not to expose the auto value!\n> > >\n> > > >\n> > > >\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..81d65ea5 100644\n> > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > @@ -61,6 +61,8 @@ 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::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > > > > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > > > @@ -97,7 +99,7 @@ namespace ipa::RPi {\n> > > > >\n> > > > > IpaBase::IpaBase()\n> > > > > : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),\n> > > > > - firstStart_(true)\n> > > > > + firstStart_(true), flickerState_({ 0, 0s })\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > @@ -812,6 +814,66 @@ void IpaBase::applyControls(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> > > > Do you need braces ?\n> > >\n> > > Sure, I can remove them!\n> > >\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> > > >\n> > > > True, but what if the app never provides a value for\n> > > > controls::AE_FLICKER_CUSTOM ? Should you initialize customPeriod to\n> > > > some sensible default ?\n> > >\n> > > In our implementation we use the value 0 to mean \"no flicker\n> > > cancellection\", so setting the mode to custom/manual without a flicker\n> > > period will cause it to do nothing (until you set one). I'm OK with\n> > > this behaviour unless anyone objects, though I don't feel strongly!\n> > > I'd rather document that setting custom/manual mode without a period\n> > > leads to undefined behaviour rather than mandating a particular value,\n> > > I think, as not every PH may wish to do the same. What do you think?\n> >\n> >\n> > I had wondered if we could get this down to a single control by having\n> > '0' as no flicker cancellation - but we'd have no way to express 'auto'\n> > so I think we do still need two, and it lets us report in the metadata\n> > if the value was determined automatically or explicitly I guess? (Though\n> > the app should know that anyway as it would know if it set the manual\n> > value?)\n> \n> Yes, I think we do still want a separate control.\n> \n> >\n> > Throwing paint on the bike shed I feel like 'AeFlickerPeriod' would be\n> > better suited for the control name as then it would be usable to report\n> > the period back in the metadata when it's detected with 'Auto' mode.\n> \n> I agree that 'AeFlickerPeriod' is a better name, particularly if the\n> 'custom' mode becomes the 'manual' mode.\n> \n> >\n> > What limits will be imposed for the period ? I think setting\n> > AeFlickerMode to 'Custom' (or 'Manual'?) and not setting an\n> > {AeFlickerCustom,AeFlickerPeriod} control should be the same as not\n> > changing anything, but I expect that maybe the ControlInfo might have\n> > limits that would preclude setting a period of '0' ?\n> \n> I guess implementations get to choose their own limits though, for the\n> most part, I see no reason not to allow them to be very wide.\n> Extremely short or extremely long periods are going to be increasingly\n> unlikely IRL, of course.\n\nThe part I was wondering about is if we 'defined' 0 as being equivalent\nto controls::FlickerOff, but that could make it harder to validate the\ncontrol if it means any value from 0...MAX is 'acceptable'.\n\nBut yes, I think this can all be handled by the pipeline handler/IPA.\n\n\n> > It might be reasonable to say setting AeFlickerMode to Manual/Custom\n> > will only take effect when the corresponding AeFlickerPeriod control is\n> > applied ?\n> \n> Yes, we could say that setting the mode to manual has no effect until\n> a period is also set. Though of course I assume the period is\n> \"sticky\", so once you've set it, you can go to and fro to manual mode\n> without re-sending the said period.\n\n