[{"id":26806,"web_url":"https://patchwork.libcamera.org/comment/26806/","msgid":"","date":"2023-04-03T07:57:59","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThanks for this, it looks good to me. Just one question below:\n\nOn Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel\n wrote:\n>\n> Flicker is the term used to describe brightness banding or oscillation\n> of images caused typically by artificial lighting driven by a 50 or\n> 60Hz mains supply. We add three controls intended to be used by\n> AEC/AGC algorithms:\n>\n> AeFlickerMode to enable flicker avoidance.\n>\n> AeFlickerCustom to set custom flicker periods.\n>\n> AeFlickerDetected to report any flicker that is currently detected.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> 1 file changed, 67 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index adea5f90..b472050c 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -156,6 +156,73 @@ controls:\n> control of which features should be automatically adjusted shouldn't\n> better be handled through a separate AE mode control.\n>\n> + - AeFlickerMode:\n> + type: int32_t\n> + description: |\n> + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> + algorithm attempts to hide flicker effects caused by the duty cycle of\n> + artificial lighting.\n> +\n> + Although implementation dependent, many algorithms for \"flicker\n> + avoidance\" work by restricting this exposure time to integer multiples\n> + of the cycle period, wherever possible.\n> +\n> + Implementations may not support all of the flicker modes listed below.\n> +\n> + enum:\n> + - name: FlickerOff\n> + value: 0\n> + description: No flicker avoidance is performed.\n> + - name: FlickerFreq50Hz\n> + value: 1\n> + description: 50Hz flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a 100Hz\n> + period (such as that produced by 50Hz mains electricity).\n> + - name: FlickerFreq60Hz\n> + value: 2\n> + description: 60Hz flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a 120Hz\n> + period (such as that produced by 60Hz mains electricity).\n> + - name: FlickerCustom\n> + value: 3\n> + description: Custom flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a period\n> + specified by the AeFlickerCustom control.\n> + \\sa AeFlickerCustom\n> + - name: FlickerAuto\n> + value: 4\n> + description: Automatic flicker period detection and avoidance.\n> + The system will automatically determine the most likely value of\n> + flicker period, and avoid flicker of this frequency.\n> +\n> + - AeFlickerCustom:\n> + type: int32_t\n> + description: Custom flicker period in microseconds.\n> + This value sets the current flicker period to avoid. It is used when\n> + AeFlickerMode is set to FlickerCustom.\n> +\n> + If this control is not available, then the setting of custom flicker\n> + periods is not supported.\n> +\n> + \\sa AeFlickerMode\n> +\n> + - AeFlickerDetected:\n> + type: int32_t\n> + description: Flicker period detected in microseconds.\n> + The value reported here indicates the currently detected flicker\n> + period, or zero if no flicker at all is detected.\n> +\n> + So in the case of 50Hz mains flicker, the value would be 10000\n> + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> + note that it is not required that any form of scene flicker is detected\n> + at all, so depending on the implementation, this metadata may just\n> + report zero or be entirely absent.\n> +\n> + AeFlickerDetected may also report other values for non-standard flicker\n> + periods.\n> +\n> + \\sa AeFlickerMode\n\nTo clarify, AeFlickerDetected ought to be returned unconditionally on every\nframe, even if AeFlickerMode is set to something other than FlickerAuto, is that\ncorrect?\n\nOther than that:\n\nReviewed-by: Naushir Patuck \n\n> +\n> - Brightness:\n> type: float\n> description: |\n> @@ -843,23 +910,6 @@ controls:\n> value: 1\n> description: The lens shading map mode is available.\n>\n> - - SceneFlicker:\n> - type: int32_t\n> - draft: true\n> - description: |\n> - Control to report the detected scene light frequency. Currently\n> - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> - enum:\n> - - name: SceneFickerOff\n> - value: 0\n> - description: No flickering detected.\n> - - name: SceneFicker50Hz\n> - value: 1\n> - description: 50Hz flickering detected.\n> - - name: SceneFicker60Hz\n> - value: 2\n> - description: 60Hz flickering detected.\n> -\n> - PipelineDepth:\n> type: int32_t\n> draft: true\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 B2853C326B\n\tfor ;\n\tMon, 3 Apr 2023 07:58:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E46CB61EC6;\n\tMon, 3 Apr 2023 09:58:04 +0200 (CEST)","from mail-yb1-xb33.google.com (mail-yb1-xb33.google.com\n\t[IPv6:2607:f8b0:4864:20::b33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 317C261EC6\n\tfor ;\n\tMon, 3 Apr 2023 09:58:02 +0200 (CEST)","by mail-yb1-xb33.google.com with SMTP id e65so33643302ybh.10\n\tfor ;\n\tMon, 03 Apr 2023 00:58:01 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680508684;\n\tbh=dVwf019wMh/GCrs1uH5YLlaVaa0iRkOwXB7lNxRput0=;\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=DgycndOvKg11u6KEab9EBAy0KS2q0jzHZg3VMnui0yjnrqTSyyv9xYNx/Aw0SRW2s\n\tSPJgirZQf6j+XmA2x8SoSsvNWUunHp9Wgo5jymvoh3J22n6IAYBQTC/y1hkCxy+uZs\n\tLD6eioH02F1KBKHu66ahSg0MJuK94lPPZUUFImHG8MCJOwpVHWYFM3fm/zWioH8zHi\n\tCDWxkjNC8il3Ugb4UvfEH9XF602ZQMuOtfXkPV1U3ajKPuDumGjMuM2iMI8EKxLpDE\n\trPq202o5fbuJ//9PeBeaPu+W9ckhFzUhg6gJiIC9hgiQAj0WqRw03tEYBUi0NN3AJC\n\t4FHh2g/peSXUA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1680508681;\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=dJNZ7rvvif0potMerUqfIIuMCkDOO68Xd8JQGMKcc+k=;\n\tb=T3SFqRXn9aWDmEqiBhxOueZy1f7T0/I7s/3j+vqACVr2aTiAieQaP0Br5Eg2Qzkdbq\n\tGtXoC11nFt/+Dhg871++JiiOjvpQs3IohJQpcbqW+6CHf6SJWyAH/BrU+oRm8WCNYkgc\n\tf+dHMzYnH9PENnjolxk2Uup4Wz/fQf6X87phGcDOklEYwlkwE+sAf4mOxiEIivuDq07K\n\tgafPQjZUFN1UsoR8jys6Mzy9wGPHNNPtoEpp4T24IRpITCEdYs/gLE2NwoSKnt2korRx\n\t78OfCKm1bnx+j3tgN/vpPaiHptU0CWss0HehOemoRzZv0QxCjrUCLIKdYklob2OewUgy\n\t+xlg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"T3SFqRXn\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680508681;\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=dJNZ7rvvif0potMerUqfIIuMCkDOO68Xd8JQGMKcc+k=;\n\tb=VMC/jDCQiTwK4q4Rc1ZxeDhUt3QC6td3zD9u2w4M9vOIfdex4smr23WHam/XuJKMzV\n\tQNk/lkuApB/vDkypq2Jr+0EtiDqlYXNTera/ONp16Cm+lFe1OHvFHZTRB/eCkiyqFcW5\n\tA/jTw+guyHqtcozb8IdrZZl941aTnZXt7jXhobqp+b9ff2lyDTUFwHywR9knE6XF83nS\n\t7idqGVwg+fHF6mfjJTadc8P6iAlEgmRD0MU8na5M6EH9IlYEKEdzyY0XP8zaKuusqRRa\n\tsLKBiWQm055zsfKzIFwWiFKipoNfpiYIB16mAcnDuKsm4/eT1c8dqEdiL61Sft/dlYwZ\n\tCXWQ==","X-Gm-Message-State":"AAQBX9fHkdNxN5lv5dTRXpmi1oXyMhus2Jlp+IFeW4KE1uPU8U3SWs8K\n\tP3IFieyTcHCHj74smNAy88hqPk9YGObjQD/2xsFtTQ==","X-Google-Smtp-Source":"AKy350a86/E9v2sbKsfwzET0cSKT1sP4/6XB1s0vl1AbjIS6s5tSphrq1RO8q9L8UjB/s/H8SXKjlOSONQoAARoeyY4=","X-Received":"by 2002:a25:3104:0:b0:b86:92c0:6433 with SMTP id\n\tx4-20020a253104000000b00b8692c06433mr2682395ybx.9.1680508680847;\n\tMon, 03 Apr 2023 00:58:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20230328085521.7409-2-david.plowman@raspberrypi.com>","Date":"Mon, 3 Apr 2023 08:57:59 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":26809,"web_url":"https://patchwork.libcamera.org/comment/26809/","msgid":"","date":"2023-04-03T08:20:39","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nOn Mon, 3 Apr 2023 at 08:58, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thanks for this, it looks good to me. Just one question below:\n>\n> On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel\n> wrote:\n> >\n> > Flicker is the term used to describe brightness banding or oscillation\n> > of images caused typically by artificial lighting driven by a 50 or\n> > 60Hz mains supply. We add three controls intended to be used by\n> > AEC/AGC algorithms:\n> >\n> > AeFlickerMode to enable flicker avoidance.\n> >\n> > AeFlickerCustom to set custom flicker periods.\n> >\n> > AeFlickerDetected to report any flicker that is currently detected.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> > 1 file changed, 67 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index adea5f90..b472050c 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -156,6 +156,73 @@ controls:\n> > control of which features should be automatically adjusted shouldn't\n> > better be handled through a separate AE mode control.\n> >\n> > + - AeFlickerMode:\n> > + type: int32_t\n> > + description: |\n> > + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> > + algorithm attempts to hide flicker effects caused by the duty cycle of\n> > + artificial lighting.\n> > +\n> > + Although implementation dependent, many algorithms for \"flicker\n> > + avoidance\" work by restricting this exposure time to integer multiples\n> > + of the cycle period, wherever possible.\n> > +\n> > + Implementations may not support all of the flicker modes listed below.\n> > +\n> > + enum:\n> > + - name: FlickerOff\n> > + value: 0\n> > + description: No flicker avoidance is performed.\n> > + - name: FlickerFreq50Hz\n> > + value: 1\n> > + description: 50Hz flicker avoidance.\n> > + Suppress flicker effects caused by lighting running with a 100Hz\n> > + period (such as that produced by 50Hz mains electricity).\n> > + - name: FlickerFreq60Hz\n> > + value: 2\n> > + description: 60Hz flicker avoidance.\n> > + Suppress flicker effects caused by lighting running with a 120Hz\n> > + period (such as that produced by 60Hz mains electricity).\n> > + - name: FlickerCustom\n> > + value: 3\n> > + description: Custom flicker avoidance.\n> > + Suppress flicker effects caused by lighting running with a period\n> > + specified by the AeFlickerCustom control.\n> > + \\sa AeFlickerCustom\n> > + - name: FlickerAuto\n> > + value: 4\n> > + description: Automatic flicker period detection and avoidance.\n> > + The system will automatically determine the most likely value of\n> > + flicker period, and avoid flicker of this frequency.\n> > +\n> > + - AeFlickerCustom:\n> > + type: int32_t\n> > + description: Custom flicker period in microseconds.\n> > + This value sets the current flicker period to avoid. It is used when\n> > + AeFlickerMode is set to FlickerCustom.\n> > +\n> > + If this control is not available, then the setting of custom flicker\n> > + periods is not supported.\n> > +\n> > + \\sa AeFlickerMode\n> > +\n> > + - AeFlickerDetected:\n> > + type: int32_t\n> > + description: Flicker period detected in microseconds.\n> > + The value reported here indicates the currently detected flicker\n> > + period, or zero if no flicker at all is detected.\n> > +\n> > + So in the case of 50Hz mains flicker, the value would be 10000\n> > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> > + note that it is not required that any form of scene flicker is detected\n> > + at all, so depending on the implementation, this metadata may just\n> > + report zero or be entirely absent.\n> > +\n> > + AeFlickerDetected may also report other values for non-standard flicker\n> > + periods.\n> > +\n> > + \\sa AeFlickerMode\n>\n> To clarify, AeFlickerDetected ought to be returned unconditionally on every\n> frame, even if AeFlickerMode is set to something other than FlickerAuto, is that\n> correct?\n\nActually I think that's a good question. A lot of this stuff _could_\nbe quite implementation dependent, I think, and we can't really\nmandate that implementations will do auto-detection in all\ncircumstances (some may well not do it al all).\n\nFor example, if we're actually doing flicker avoidance at 50Hz, you\nmight expect that it gets tricky to detect 50Hz flicker. So what do we\nreport? Maybe we should require that the metadata is absent (or invent\na \"can't tell\" value) in these circumstances. We should probably be\nclearer on this, so maybe another version is required\n\nDavid.\n\n>\n> Other than that:\n>\n> Reviewed-by: Naushir Patuck \n>\n> > +\n> > - Brightness:\n> > type: float\n> > description: |\n> > @@ -843,23 +910,6 @@ controls:\n> > value: 1\n> > description: The lens shading map mode is available.\n> >\n> > - - SceneFlicker:\n> > - type: int32_t\n> > - draft: true\n> > - description: |\n> > - Control to report the detected scene light frequency. Currently\n> > - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> > - enum:\n> > - - name: SceneFickerOff\n> > - value: 0\n> > - description: No flickering detected.\n> > - - name: SceneFicker50Hz\n> > - value: 1\n> > - description: 50Hz flickering detected.\n> > - - name: SceneFicker60Hz\n> > - value: 2\n> > - description: 60Hz flickering detected.\n> > -\n> > - PipelineDepth:\n> > type: int32_t\n> > draft: true\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 0827AC0F2A\n\tfor ;\n\tMon, 3 Apr 2023 08:20:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6414762722;\n\tMon, 3 Apr 2023 10:20:53 +0200 (CEST)","from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com\n\t[IPv6:2607:f8b0:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D984D61EC6\n\tfor ;\n\tMon, 3 Apr 2023 10:20:51 +0200 (CEST)","by mail-oi1-x22f.google.com with SMTP id bm2so21126695oib.4\n\tfor ;\n\tMon, 03 Apr 2023 01:20:51 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680510053;\n\tbh=gK6Mo50Jlj5OREYv0BNrNHm9KGauHgD3114/33LhO+w=;\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=fOmNNn1LnNKae89jQjWXs1nDeJgAcC9JBPLXwUpk8dVr07lvO0oDtHRqnnVXdOoNb\n\tFcNOI7n9yYlxfTEmHf2QNi1EK5SOQcqIFSnQ330XEOL+FjlfK4Gq5gEzRNC4HFp4Mt\n\t0cZOug191Hn3x2OyQZE1EaxNhLo3x7kqBqUeZA88/2toOv7yS8SSq0F/ASeM79TPoz\n\t2bBLJ8/by0nET0QyoVfBDHX/VCYJKDePOWwVrlX/iph0q/gqqHNnlPHt0ohKk+INf6\n\tJ281Yp5jgXull3V6sI94E2C6KXneWJRmvX/cWh2enjFSCd+pJ0+H+Ge6RzC9EDJ356\n\t7AW8w5wR2M33g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1680510050;\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=31MlXvDYBbk2gFJycwZULHp8I44YB8z8et/Rd+p5NuU=;\n\tb=KJD4ynRbRP4hW3l0cvyOafg3tSynFB+ErBIKk+6f0oGE3x4RElrxcLUP3AXNv5oZ0x\n\taomNf+zu/sgMg45PuFrdiwcuIT3YDzj7Zvqgs+FjxNZShoxHt1VnzzosRNxcz1Z8vq0S\n\tCKd0PTill66WXucESqBrycf6ghK6wkKDu0ZwigkYW9idrzanw/X7wuSfidr9EYk5nbTV\n\tUqqeFu20885LWlI+IMEsUe3p7nt0OOlgH7xpPqzOfFEM+7+9GdSBI2Qkn53FXTpuGyce\n\tScroEIhcXV5LfaKBR244q7qP5M1H1MGHANu2Tkiyj3rcOdhWoIXzhmhDN927pMXm3+oi\n\tGcXg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"KJD4ynRb\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680510050;\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=31MlXvDYBbk2gFJycwZULHp8I44YB8z8et/Rd+p5NuU=;\n\tb=ARozLRcVZx8RcXJaHZD5T73UoNeMCG2RK7FNKN1dO1DaUcsNDJ68fAUxtRdrur+FBC\n\tO/UqGFOijzsHPNiCLq8TdtCM528n5ip8ZPtpmV9lUt47ivc92g1AWrNM6OzIyN98sJpe\n\tYyFyLyIeuIK+Kwlw58PnBovdUvwdYCNFOiCBUu/JDZvKfc45vknuY1cocU3mMVa00LAk\n\tGjpdjyuBT16b4dQ4Rp6VpRbH278HSVm5OAuVgcA1agcI2VFPo93TIJor3xQVhVqojjCT\n\tpAB1pxtro/Q7bgGp2xResxzEhsdlrO6wYKs9yCBxy14MW3C6fnEwXwXxJI9nqGUH4+iN\n\taVVQ==","X-Gm-Message-State":"AO0yUKUn2e7NWWDvfVJNKmPebWPg7pItsIUhmmcvg3/786WjX/qETOsl\n\t3waj43/mxVGAqHvhkYvWz71aeCP6TgwWPKHfTp7z/ZkgI7aAs5qD","X-Google-Smtp-Source":"AK7set8oLDUuYO5RU6Xi4OvJxFMbwc0wBqOjlujhqnPOyEjuxo36692hOW6E+VW6hbjNzEVoLI/QGkYWlQ+Ax+LmXTA=","X-Received":"by 2002:a05:6808:638b:b0:386:d70b:d67c with SMTP id\n\tec11-20020a056808638b00b00386d70bd67cmr8854284oib.11.1680510050517;\n\tMon, 03 Apr 2023 01:20:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 3 Apr 2023 09:20:39 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":26820,"web_url":"https://patchwork.libcamera.org/comment/26820/","msgid":"<20230403143025.dt3fzpwrpbta2gki@uno.localdomain>","date":"2023-04-03T14:30:25","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Tue, Mar 28, 2023 at 09:55:20AM +0100, David Plowman via libcamera-devel wrote:\n> Flicker is the term used to describe brightness banding or oscillation\n> of images caused typically by artificial lighting driven by a 50 or\n> 60Hz mains supply. We add three controls intended to be used by\n> AEC/AGC algorithms:\n>\n> AeFlickerMode to enable flicker avoidance.\n>\n> AeFlickerCustom to set custom flicker periods.\n>\n> AeFlickerDetected to report any flicker that is currently detected.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> 1 file changed, 67 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index adea5f90..b472050c 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -156,6 +156,73 @@ controls:\n> control of which features should be automatically adjusted shouldn't\n> better be handled through a separate AE mode control.\n>\n> + - AeFlickerMode:\n> + type: int32_t\n> + description: |\n> + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> + algorithm attempts to hide flicker effects caused by the duty cycle of\n> + artificial lighting.\n> +\n> + Although implementation dependent, many algorithms for \"flicker\n> + avoidance\" work by restricting this exposure time to integer multiples\n> + of the cycle period, wherever possible.\n> +\n> + Implementations may not support all of the flicker modes listed below.\n\n If the automatic control of exposure time is disabled, this\n control has no effect.\n> +\n> + enum:\n> + - name: FlickerOff\n> + value: 0\n> + description: No flicker avoidance is performed.\n> + - name: FlickerFreq50Hz\n> + value: 1\n> + description: 50Hz flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a 100Hz\n> + period (such as that produced by 50Hz mains electricity).\n> + - name: FlickerFreq60Hz\n> + value: 2\n> + description: 60Hz flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a 120Hz\n> + period (such as that produced by 60Hz mains electricity).\n> + - name: FlickerCustom\n> + value: 3\n> + description: Custom flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a period\n> + specified by the AeFlickerCustom control.\n> + \\sa AeFlickerCustom\n> + - name: FlickerAuto\n> + value: 4\n> + description: Automatic flicker period detection and avoidance.\n> + The system will automatically determine the most likely value of\n> + flicker period, and avoid flicker of this frequency.\n> +\n> + - AeFlickerCustom:\n> + type: int32_t\n> + description: Custom flicker period in microseconds.\n\nmicroseconds or Hz ?\n\n> + This value sets the current flicker period to avoid. It is used when\n> + AeFlickerMode is set to FlickerCustom.\n> +\n> + If this control is not available, then the setting of custom flicker\n> + periods is not supported.\n\nI presume pipeline handlers/IPA should not register FlickerCustom\namong the supported AeFlickerMode instead.\n\n> +\n> + \\sa AeFlickerMode\n> +\n> + - AeFlickerDetected:\n\nIs this \"Ae\" ?\nOr should it be just (Scene)FlickerDetected ?\n\n> + type: int32_t\n> + description: Flicker period detected in microseconds.\n> + The value reported here indicates the currently detected flicker\n> + period, or zero if no flicker at all is detected.\n> +\n> + So in the case of 50Hz mains flicker, the value would be 10000\n> + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> + note that it is not required that any form of scene flicker is detected\n> + at all, so depending on the implementation, this metadata may just\n> + report zero or be entirely absent.\n\nI would be more strict and require platforms that do not support\nflicker detection not to report this control at all, instead of\nreporting 0\n\n> +\n> + AeFlickerDetected may also report other values for non-standard flicker\n> + periods.\n> +\n> + \\sa AeFlickerMode\n> +\n> - Brightness:\n> type: float\n> description: |\n> @@ -843,23 +910,6 @@ controls:\n> value: 1\n> description: The lens shading map mode is available.\n>\n> - - SceneFlicker:\n> - type: int32_t\n> - draft: true\n> - description: |\n> - Control to report the detected scene light frequency. Currently\n> - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> - enum:\n> - - name: SceneFickerOff\n> - value: 0\n> - description: No flickering detected.\n> - - name: SceneFicker50Hz\n> - value: 1\n> - description: 50Hz flickering detected.\n> - - name: SceneFicker60Hz\n> - value: 2\n> - description: 60Hz flickering detected.\n> -\n> - PipelineDepth:\n> type: int32_t\n> draft: true\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 48285C326B\n\tfor ;\n\tMon, 3 Apr 2023 14:30:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E3E36271C;\n\tMon, 3 Apr 2023 16:30:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D09C626E3\n\tfor ;\n\tMon, 3 Apr 2023 16:30:28 +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 E54924A7;\n\tMon, 3 Apr 2023 16:30:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680532230;\n\tbh=94WRxfDzTZ/P1Twlm8dcWamns5bjq1TD8FfmhSEI6JI=;\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=a6KqHHSmfaRI+MPLl5o+BC+1U28cGcnWcmV4gqBz1zQAM1vcoOdGBD1juh4dM/onH\n\tpYPSuCyzzHl9ueqCoxjmCgmaBD495rF0sL/8gG62w2SkvnZNqjzqeffiZeWznKVDgG\n\t216/tDw2yTEWPhgCHfNOBEgjkCtbhixKXTn7D6oTJn+2af3ztTDIFe2q8f+XV5mgnj\n\t2MLzEiytGm/rjIn/HbDP0Yec+NzTH7/XpSNHyo5kWKyXSdrDjJsNYyx1TGBNLUDMST\n\tNSPWIZVmTKY/AiV4+wCqS1pySaLQNLkEaqPhsp1Q1c8pbHzVP+ySstv9WUHvxzpfSB\n\tbG+FDEWjrSaAQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680532228;\n\tbh=94WRxfDzTZ/P1Twlm8dcWamns5bjq1TD8FfmhSEI6JI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fkN8Yoog6UteWKldnBWYdpnNZU74LhlR/ZvRUhOeHV84Iu5DP29t+YDU1c2CLIWB0\n\tw08jA73mpLsRxq4MEKzWlFslQ5axvziUaz4fmsXliGnjRT2xevopDqeEoPycWfH2xT\n\tb9ltRG6nYZfhz2nHlgJeS9es8j8++YihydS9vWO4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fkN8Yoog\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 16:30:25 +0200","To":"David Plowman ","Message-ID":"<20230403143025.dt3fzpwrpbta2gki@uno.localdomain>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230328085521.7409-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":26822,"web_url":"https://patchwork.libcamera.org/comment/26822/","msgid":"<65cc89e7d7f61e4a1625d0db73f000a7ca181279.camel@ndufresne.ca>","date":"2023-04-03T19:34:26","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mardi 28 mars 2023 à 09:55 +0100, David Plowman via libcamera-devel a écrit :\n> Flicker is the term used to describe brightness banding or oscillation\n> of images caused typically by artificial lighting driven by a 50 or\n> 60Hz mains supply. We add three controls intended to be used by\n> AEC/AGC algorithms:\n> \n> AeFlickerMode to enable flicker avoidance.\n> \n> AeFlickerCustom to set custom flicker periods.\n> \n> AeFlickerDetected to report any flicker that is currently detected.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> 1 file changed, 67 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index adea5f90..b472050c 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -156,6 +156,73 @@ controls:\n> control of which features should be automatically adjusted shouldn't\n> better be handled through a separate AE mode control.\n> \n> + - AeFlickerMode:\n\nMaybe loaded question, since it goes beyound the scope of the patch, but how are\nwe suppose to know which controls is an actual control, and which one are\nmetadata ? (read-only, possibly on sync with request ?)\n\n> + type: int32_t\n> + description: |\n> + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> + algorithm attempts to hide flicker effects caused by the duty cycle of\n> + artificial lighting.\n> +\n> + Although implementation dependent, many algorithms for \"flicker\n> + avoidance\" work by restricting this exposure time to integer multiples\n> + of the cycle period, wherever possible.\n> +\n> + Implementations may not support all of the flicker modes listed below.\n> +\n> + enum:\n> + - name: FlickerOff\n> + value: 0\n> + description: No flicker avoidance is performed.\n> + - name: FlickerFreq50Hz\n> + value: 1\n> + description: 50Hz flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a 100Hz\n> + period (such as that produced by 50Hz mains electricity).\n> + - name: FlickerFreq60Hz\n> + value: 2\n> + description: 60Hz flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a 120Hz\n> + period (such as that produced by 60Hz mains electricity).\n> + - name: FlickerCustom\n> + value: 3\n> + description: Custom flicker avoidance.\n> + Suppress flicker effects caused by lighting running with a period\n> + specified by the AeFlickerCustom control.\n> + \\sa AeFlickerCustom\n> + - name: FlickerAuto\n> + value: 4\n> + description: Automatic flicker period detection and avoidance.\n> + The system will automatically determine the most likely value of\n> + flicker period, and avoid flicker of this frequency.\n> +\n> + - AeFlickerCustom:\n> + type: int32_t\n> + description: Custom flicker period in microseconds.\n> + This value sets the current flicker period to avoid. It is used when\n> + AeFlickerMode is set to FlickerCustom.\n> +\n> + If this control is not available, then the setting of custom flicker\n> + periods is not supported.\n> +\n> + \\sa AeFlickerMode\n> +\n> + - AeFlickerDetected:\n> + type: int32_t\n> + description: Flicker period detected in microseconds.\n> + The value reported here indicates the currently detected flicker\n> + period, or zero if no flicker at all is detected.\n> +\n> + So in the case of 50Hz mains flicker, the value would be 10000\n> + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> + note that it is not required that any form of scene flicker is detected\n> + at all, so depending on the implementation, this metadata may just\n> + report zero or be entirely absent.\n> +\n> + AeFlickerDetected may also report other values for non-standard flicker\n> + periods.\n> +\n> + \\sa AeFlickerMode\n> +\n> - Brightness:\n> type: float\n> description: |\n> @@ -843,23 +910,6 @@ controls:\n> value: 1\n> description: The lens shading map mode is available.\n> \n> - - SceneFlicker:\n> - type: int32_t\n> - draft: true\n> - description: |\n> - Control to report the detected scene light frequency. Currently\n> - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> - enum:\n> - - name: SceneFickerOff\n> - value: 0\n> - description: No flickering detected.\n> - - name: SceneFicker50Hz\n> - value: 1\n> - description: 50Hz flickering detected.\n> - - name: SceneFicker60Hz\n> - value: 2\n> - description: 60Hz flickering detected.\n> -\n> - PipelineDepth:\n> type: int32_t\n> draft: true","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 3F05CC0F2A\n\tfor ;\n\tMon, 3 Apr 2023 19:34:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77B5B626E3;\n\tMon, 3 Apr 2023 21:34:30 +0200 (CEST)","from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com\n\t[IPv6:2607:f8b0:4864:20::82c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C240A626E3\n\tfor ;\n\tMon, 3 Apr 2023 21:34:28 +0200 (CEST)","by mail-qt1-x82c.google.com with SMTP id r5so29490910qtp.4\n\tfor ;\n\tMon, 03 Apr 2023 12:34:28 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:15:a07e::580])\n\tby smtp.gmail.com with ESMTPSA id\n\th17-20020ac85151000000b003e4e1dbdcc3sm2750120qtn.47.2023.04.03.12.34.26\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 03 Apr 2023 12:34:27 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680550470;\n\tbh=MMek9Au3AIEq/rP3osB+3DhooDLD9+oDK0g0B75tvxQ=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=JS6FQPKujZhw4QdJ7blcHROYdgmnJVsVAuCNc0zv0kqSGF32jBSAY+Oxch5sHmi/Y\n\tq3937uIpb6LH4ZciJVjlJlVsMyorfZH0peDaeBFOv9IFx9iKYNvbXAW+oSybtpHCn/\n\tSehpTGQxugVNCpLCsG6GVAgM65y+wqlhy9T2QdEN7RQKDPfwkaSkMLbKqeRl8jDl38\n\tMLXZHMC3hrdOQqwPXbZ0HaNcmowlW4CFm193EJMzZeznvjT14HUjChAVi5dzP/LHNm\n\tZ9Dh56zise7K+Xx0eEl8HqtunQlLyrcZlLECU8Kptrg9v5bYwvOFnyXGMcdKSMUzNu\n\tFefRlAM3SktzQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20210112.gappssmtp.com; s=20210112; t=1680550467;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Q3bR1e+xQ41EJoJSdB/dSowguRFcKtEsitZ5Of6womE=;\n\tb=j+iBLekYf9OS8xNtLYykiP8QaYMCpgbpGbMQocsY6EX7D86MZlq7kOiSMM91cViCN3\n\tjy5Vo/cE76ppUJsbPcVzgkZZ4dbnCXiNbWv7AhrcqPVpDtWIlK2wkpPq1ZUnW/zFxPKl\n\tJNyrWn15+S9NZLGdjdFYy3T1m7GRYWc/NXV56CHjcTG7RlvmPWszx04kCdFZPgpt1J+S\n\tE26u+muJeVv1nKdiZ+C6XwRRle6tvmkaPffWtftNIdzH4zrOc9PrCuJB6k0RGPTyF6pV\n\ttM/C1NAli9TnjAcZJEVIt6OF3DE9WsldZe/F3zxBNdHEXddfjYe40xVe1q07304ea/wm\n\tt9fQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20210112.gappssmtp.com\n\theader.i=@ndufresne-ca.20210112.gappssmtp.com header.b=\"j+iBLekY\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680550467;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=Q3bR1e+xQ41EJoJSdB/dSowguRFcKtEsitZ5Of6womE=;\n\tb=wvZzoPIWK1yUEZe6Mb4gOAoQXFVmDsY364zptWh2YaHVnxFb/ACCC19f6nm4iylL7V\n\toVC1w953cRspc3voztNOkxI/3D8CfNZNqy5U/tIMWrzgQifDwSHm2Cztq+eHk+7Gw9wj\n\t2+zdQMurcpukNDKcR7+dZyj/bJncEYjH8D2555Wx4gOB+Y80poIn87nqBrRu8EK7QuYT\n\tK/UAl+Oi/rQlpZcToFbJ2T6lZbiJoN5fKYJQq5zbz19zqqH9mniyEf4i3nXhW4rZWuL8\n\tyOTMtpuLYFiuzmzIcq1nlCncxlRWrz+FVaDIT0OCvOJEwhIHA/UMLKilPbV7YHZgAHeB\n\tHh7g==","X-Gm-Message-State":"AAQBX9esdU5DWZ/o+jGCroWsDkXy3/YpSXnz0pVIDXJAwz60QQ1WtEuY\n\ta7DS6urMds7sM93vTiLoQ4AgMA==","X-Google-Smtp-Source":"AKy350b6vhXlvMGL2RKPqKJijkGyc1YU09cCPP21oA2GfeF4J0++RMsC61Zfn7UsyTT6V1Lb3Zv0dw==","X-Received":"by 2002:a05:622a:1c1:b0:3e1:78b5:7198 with SMTP id\n\tt1-20020a05622a01c100b003e178b57198mr424766qtw.8.1680550467328; \n\tMon, 03 Apr 2023 12:34:27 -0700 (PDT)","Message-ID":"<65cc89e7d7f61e4a1625d0db73f000a7ca181279.camel@ndufresne.ca>","To":"David Plowman , \n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 03 Apr 2023 15:34:26 -0400","In-Reply-To":"<20230328085521.7409-2-david.plowman@raspberrypi.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.46.4 (3.46.4-1.fc37) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26823,"web_url":"https://patchwork.libcamera.org/comment/26823/","msgid":"<168055406861.810867.12685882690166719572@Monstersaurus>","date":"2023-04-03T20:34:28","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Nicolas Dufresne via libcamera-devel (2023-04-03 20:34:26)\n> Le mardi 28 mars 2023 à 09:55 +0100, David Plowman via libcamera-devel a écrit :\n> > Flicker is the term used to describe brightness banding or oscillation\n> > of images caused typically by artificial lighting driven by a 50 or\n> > 60Hz mains supply. We add three controls intended to be used by\n> > AEC/AGC algorithms:\n> > \n> > AeFlickerMode to enable flicker avoidance.\n> > \n> > AeFlickerCustom to set custom flicker periods.\n> > \n> > AeFlickerDetected to report any flicker that is currently detected.\n> > \n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> > 1 file changed, 67 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index adea5f90..b472050c 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -156,6 +156,73 @@ controls:\n> > control of which features should be automatically adjusted shouldn't\n> > better be handled through a separate AE mode control.\n> > \n> > + - AeFlickerMode:\n> \n> Maybe loaded question, since it goes beyound the scope of the patch, but how are\n> we suppose to know which controls is an actual control, and which one are\n> metadata ? (read-only, possibly on sync with request ?)\n\na camera exposes a ControlList as camera->controls()\nThose (and only those) are the valid controls for that camera.\n\nWhen a request completes, it can share data in request->metadata() which\nis a ControlList of ControlValues that can be interpreted by the\napplication to see what was, or any control value that the pipeline\nhandler chooses to put in there really. I think we did look at having\nseparate 'Metadata' control lists - but it seemed like that would be a\nlot of duplication of keys...\n\nUnless you're asking how can we know 'from the documentation'. .. in\nwhich case - currently it's not specifically defined, but we could add a\nproperty to the yaml file?\n\n--\nKieran\n\n\n> \n> > + type: int32_t\n> > + description: |\n> > + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> > + algorithm attempts to hide flicker effects caused by the duty cycle of\n> > + artificial lighting.\n> > +\n> > + Although implementation dependent, many algorithms for \"flicker\n> > + avoidance\" work by restricting this exposure time to integer multiples\n> > + of the cycle period, wherever possible.\n> > +\n> > + Implementations may not support all of the flicker modes listed below.\n> > +\n> > + enum:\n> > + - name: FlickerOff\n> > + value: 0\n> > + description: No flicker avoidance is performed.\n> > + - name: FlickerFreq50Hz\n> > + value: 1\n> > + description: 50Hz flicker avoidance.\n> > + Suppress flicker effects caused by lighting running with a 100Hz\n> > + period (such as that produced by 50Hz mains electricity).\n> > + - name: FlickerFreq60Hz\n> > + value: 2\n> > + description: 60Hz flicker avoidance.\n> > + Suppress flicker effects caused by lighting running with a 120Hz\n> > + period (such as that produced by 60Hz mains electricity).\n> > + - name: FlickerCustom\n> > + value: 3\n> > + description: Custom flicker avoidance.\n> > + Suppress flicker effects caused by lighting running with a period\n> > + specified by the AeFlickerCustom control.\n> > + \\sa AeFlickerCustom\n> > + - name: FlickerAuto\n> > + value: 4\n> > + description: Automatic flicker period detection and avoidance.\n> > + The system will automatically determine the most likely value of\n> > + flicker period, and avoid flicker of this frequency.\n> > +\n> > + - AeFlickerCustom:\n> > + type: int32_t\n> > + description: Custom flicker period in microseconds.\n> > + This value sets the current flicker period to avoid. It is used when\n> > + AeFlickerMode is set to FlickerCustom.\n> > +\n> > + If this control is not available, then the setting of custom flicker\n> > + periods is not supported.\n> > +\n> > + \\sa AeFlickerMode\n> > +\n> > + - AeFlickerDetected:\n> > + type: int32_t\n> > + description: Flicker period detected in microseconds.\n> > + The value reported here indicates the currently detected flicker\n> > + period, or zero if no flicker at all is detected.\n> > +\n> > + So in the case of 50Hz mains flicker, the value would be 10000\n> > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> > + note that it is not required that any form of scene flicker is detected\n> > + at all, so depending on the implementation, this metadata may just\n> > + report zero or be entirely absent.\n> > +\n> > + AeFlickerDetected may also report other values for non-standard flicker\n> > + periods.\n> > +\n> > + \\sa AeFlickerMode\n> > +\n> > - Brightness:\n> > type: float\n> > description: |\n> > @@ -843,23 +910,6 @@ controls:\n> > value: 1\n> > description: The lens shading map mode is available.\n> > \n> > - - SceneFlicker:\n> > - type: int32_t\n> > - draft: true\n> > - description: |\n> > - Control to report the detected scene light frequency. Currently\n> > - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> > - enum:\n> > - - name: SceneFickerOff\n> > - value: 0\n> > - description: No flickering detected.\n> > - - name: SceneFicker50Hz\n> > - value: 1\n> > - description: 50Hz flickering detected.\n> > - - name: SceneFicker60Hz\n> > - value: 2\n> > - description: 60Hz flickering detected.\n> > -\n> > - PipelineDepth:\n> > type: int32_t\n> > draft: true\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 D6047C0F2A\n\tfor ;\n\tMon, 3 Apr 2023 20:34:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E132626E3;\n\tMon, 3 Apr 2023 22:34:32 +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 4F9C6626E3\n\tfor ;\n\tMon, 3 Apr 2023 22:34:31 +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 D5CC2BC4;\n\tMon, 3 Apr 2023 22:34:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680554072;\n\tbh=d60ec4WGenjd1XXofzURO3Rr/0xHeT2flerVqmUEW9E=;\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=KltrNMtw8T0o6JvGiJcQT0qfqxCNRu3eqiaUhB0+jUzGatBN85EkA8qnucT6Tb27H\n\tZX5HX0oxOr6fJzHUbGYnfpSv4ahAuMaX0yg8sMNwj4r+TEJcJ48vij4skoQ5gml41Z\n\tjWJGHUIDWRpUmmjI/TrD01sAxtxAuF5m7R/UCLIqK5VKwigbrrvjQutbfKohhgMEo4\n\tnfHzlQ7A670hy9QJPN76NHXBi1kGK5ntEYE3cMkhJnMZLjWDftUorZ/DW1Esxt9yjT\n\tIHWZJmMeHRRKDjAahLjnvVygG0k88jWvN78Ug9T1R41V170lfW/JfvVLfVwNG8iJJM\n\t/roKTrEwyK7Rw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680554070;\n\tbh=d60ec4WGenjd1XXofzURO3Rr/0xHeT2flerVqmUEW9E=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=NadKjUfFExBrsx68IUgC162cCR5Ahc3P4siNn6pXKCXB88YBMQeg8sr73XfAqkZVR\n\tadg0yP2Ns/m6v/OM1xwFRpUm7nxbcHFYW/S/4XCrE6wqiY9JhGza8/H83Q821eFxHV\n\t1SNv7aMMABq1tf0wo42bi8gj4IAjYlm/s7mjPCec="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NadKjUfF\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<65cc89e7d7f61e4a1625d0db73f000a7ca181279.camel@ndufresne.ca>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>\n\t<65cc89e7d7f61e4a1625d0db73f000a7ca181279.camel@ndufresne.ca>","To":"David Plowman ,\n\tNicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 03 Apr 2023 21:34:28 +0100","Message-ID":"<168055406861.810867.12685882690166719572@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":26825,"web_url":"https://patchwork.libcamera.org/comment/26825/","msgid":"<20230404054943.GQ16648@pendragon.ideasonboard.com>","date":"2023-04-04T05:49:43","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Apr 03, 2023 at 09:34:28PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Nicolas Dufresne via libcamera-devel (2023-04-03 20:34:26)\n> > Le mardi 28 mars 2023 à 09:55 +0100, David Plowman via libcamera-devel a écrit :\n> > > Flicker is the term used to describe brightness banding or oscillation\n> > > of images caused typically by artificial lighting driven by a 50 or\n> > > 60Hz mains supply. We add three controls intended to be used by\n> > > AEC/AGC algorithms:\n> > > \n> > > AeFlickerMode to enable flicker avoidance.\n> > > \n> > > AeFlickerCustom to set custom flicker periods.\n> > > \n> > > AeFlickerDetected to report any flicker that is currently detected.\n> > > \n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> > > 1 file changed, 67 insertions(+), 17 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index adea5f90..b472050c 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -156,6 +156,73 @@ controls:\n> > > control of which features should be automatically adjusted shouldn't\n> > > better be handled through a separate AE mode control.\n> > > \n> > > + - AeFlickerMode:\n> > \n> > Maybe loaded question, since it goes beyound the scope of the patch, but how are\n> > we suppose to know which controls is an actual control, and which one are\n> > metadata ? (read-only, possibly on sync with request ?)\n> \n> a camera exposes a ControlList as camera->controls()\n> Those (and only those) are the valid controls for that camera.\n> \n> When a request completes, it can share data in request->metadata() which\n> is a ControlList of ControlValues that can be interpreted by the\n> application to see what was, or any control value that the pipeline\n> handler chooses to put in there really. I think we did look at having\n> separate 'Metadata' control lists - but it seemed like that would be a\n> lot of duplication of keys...\n> \n> Unless you're asking how can we know 'from the documentation'. .. in\n> which case - currently it's not specifically defined, but we could add a\n> property to the yaml file?\n\nI think we should.\n\n> > > + type: int32_t\n> > > + description: |\n> > > + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> > > + algorithm attempts to hide flicker effects caused by the duty cycle of\n> > > + artificial lighting.\n> > > +\n> > > + Although implementation dependent, many algorithms for \"flicker\n> > > + avoidance\" work by restricting this exposure time to integer multiples\n> > > + of the cycle period, wherever possible.\n> > > +\n> > > + Implementations may not support all of the flicker modes listed below.\n> > > +\n> > > + enum:\n> > > + - name: FlickerOff\n> > > + value: 0\n> > > + description: No flicker avoidance is performed.\n> > > + - name: FlickerFreq50Hz\n> > > + value: 1\n> > > + description: 50Hz flicker avoidance.\n> > > + Suppress flicker effects caused by lighting running with a 100Hz\n> > > + period (such as that produced by 50Hz mains electricity).\n> > > + - name: FlickerFreq60Hz\n> > > + value: 2\n> > > + description: 60Hz flicker avoidance.\n> > > + Suppress flicker effects caused by lighting running with a 120Hz\n> > > + period (such as that produced by 60Hz mains electricity).\n> > > + - name: FlickerCustom\n> > > + value: 3\n> > > + description: Custom flicker avoidance.\n> > > + Suppress flicker effects caused by lighting running with a period\n> > > + specified by the AeFlickerCustom control.\n> > > + \\sa AeFlickerCustom\n> > > + - name: FlickerAuto\n> > > + value: 4\n> > > + description: Automatic flicker period detection and avoidance.\n> > > + The system will automatically determine the most likely value of\n> > > + flicker period, and avoid flicker of this frequency.\n> > > +\n> > > + - AeFlickerCustom:\n> > > + type: int32_t\n> > > + description: Custom flicker period in microseconds.\n> > > + This value sets the current flicker period to avoid. It is used when\n> > > + AeFlickerMode is set to FlickerCustom.\n> > > +\n> > > + If this control is not available, then the setting of custom flicker\n> > > + periods is not supported.\n> > > +\n> > > + \\sa AeFlickerMode\n> > > +\n> > > + - AeFlickerDetected:\n> > > + type: int32_t\n> > > + description: Flicker period detected in microseconds.\n> > > + The value reported here indicates the currently detected flicker\n> > > + period, or zero if no flicker at all is detected.\n> > > +\n> > > + So in the case of 50Hz mains flicker, the value would be 10000\n> > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> > > + note that it is not required that any form of scene flicker is detected\n> > > + at all, so depending on the implementation, this metadata may just\n> > > + report zero or be entirely absent.\n> > > +\n> > > + AeFlickerDetected may also report other values for non-standard flicker\n> > > + periods.\n> > > +\n> > > + \\sa AeFlickerMode\n> > > +\n> > > - Brightness:\n> > > type: float\n> > > description: |\n> > > @@ -843,23 +910,6 @@ controls:\n> > > value: 1\n> > > description: The lens shading map mode is available.\n> > > \n> > > - - SceneFlicker:\n> > > - type: int32_t\n> > > - draft: true\n> > > - description: |\n> > > - Control to report the detected scene light frequency. Currently\n> > > - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> > > - enum:\n> > > - - name: SceneFickerOff\n> > > - value: 0\n> > > - description: No flickering detected.\n> > > - - name: SceneFicker50Hz\n> > > - value: 1\n> > > - description: 50Hz flickering detected.\n> > > - - name: SceneFicker60Hz\n> > > - value: 2\n> > > - description: 60Hz flickering detected.\n> > > -\n> > > - PipelineDepth:\n> > > type: int32_t\n> > > draft: true","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 894EDC0F2A\n\tfor ;\n\tTue, 4 Apr 2023 05:49:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D011D603A4;\n\tTue, 4 Apr 2023 07:49:39 +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 37EEB603A4\n\tfor ;\n\tTue, 4 Apr 2023 07:49:38 +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 922AC4A7;\n\tTue, 4 Apr 2023 07:49:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680587379;\n\tbh=rwmiBFVRoNW7bRimnLoeUUxdSkrmacQGKtGcp+i2bzU=;\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=cZcSVuxqT56SiTgHpu3xUQeOcKudad6Y3f33QUapw0hkPaWHUXVqia1wAKYnU6x3u\n\tTty3KmHWqxH/NNDozfg6uDjMEppDEr4UAVtRAGwD9yk6I3bB7wvsfYS2Gc3bS9zFnb\n\t8DB0A4ez1dq5mt8LOjytvt3uIj+ESAAN7Maceuba9SE5nXh9T9AwHJT68wyOfaimqy\n\tTgXvFzDXDFp4mX9kPyrxjAgiouC3GSdUhBlfNHiFA3BxCYX8XyNu8km3a9cqu7Y1x+\n\t44CqJqQpvDy0pkL2/4f80FIXlTBlfgmltuRCZ9OYex4v+ZlfKF1sqFlTyYEjYup++O\n\tU5wSUZ/aJJTtw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680587377;\n\tbh=rwmiBFVRoNW7bRimnLoeUUxdSkrmacQGKtGcp+i2bzU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Vc6m0nlq7KTw7o8ONQg4127EjSkoP1px7visGvT2gsrH/3RwZQ10M7BJefAoD72z5\n\tt382jCSCaTnsQZYAgPGY+HfogcMxyCmta4MaGxyDf1A6oT4sMvHbR3QIUU8ekLcQMJ\n\tfP/dwwInA9skS3aoZ2Kx0tXQJcG6Pd/aE8uS+aNU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Vc6m0nlq\"; dkim-atps=neutral","Date":"Tue, 4 Apr 2023 08:49:43 +0300","To":"Kieran Bingham ","Message-ID":"<20230404054943.GQ16648@pendragon.ideasonboard.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>\n\t<65cc89e7d7f61e4a1625d0db73f000a7ca181279.camel@ndufresne.ca>\n\t<168055406861.810867.12685882690166719572@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<168055406861.810867.12685882690166719572@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":26826,"web_url":"https://patchwork.libcamera.org/comment/26826/","msgid":"<20230404060636.GR16648@pendragon.ideasonboard.com>","date":"2023-04-04T06:06:36","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Mon, Apr 03, 2023 at 09:20:39AM +0100, David Plowman via libcamera-devel wrote:\n> On Mon, 3 Apr 2023 at 08:58, Naushir Patuck wrote:\n> > On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel wrote:\n> > >\n> > > Flicker is the term used to describe brightness banding or oscillation\n> > > of images caused typically by artificial lighting driven by a 50 or\n> > > 60Hz mains supply. We add three controls intended to be used by\n> > > AEC/AGC algorithms:\n> > >\n> > > AeFlickerMode to enable flicker avoidance.\n> > >\n> > > AeFlickerCustom to set custom flicker periods.\n> > >\n> > > AeFlickerDetected to report any flicker that is currently detected.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> > > 1 file changed, 67 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index adea5f90..b472050c 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -156,6 +156,73 @@ controls:\n> > > control of which features should be automatically adjusted shouldn't\n> > > better be handled through a separate AE mode control.\n> > >\n> > > + - AeFlickerMode:\n> > > + type: int32_t\n> > > + description: |\n> > > + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> > > + algorithm attempts to hide flicker effects caused by the duty cycle of\n> > > + artificial lighting.\n> > > +\n> > > + Although implementation dependent, many algorithms for \"flicker\n> > > + avoidance\" work by restricting this exposure time to integer multiples\n> > > + of the cycle period, wherever possible.\n> > > +\n> > > + Implementations may not support all of the flicker modes listed below.\n> > > +\n> > > + enum:\n> > > + - name: FlickerOff\n> > > + value: 0\n> > > + description: No flicker avoidance is performed.\n> > > + - name: FlickerFreq50Hz\n> > > + value: 1\n> > > + description: 50Hz flicker avoidance.\n> > > + Suppress flicker effects caused by lighting running with a 100Hz\n> > > + period (such as that produced by 50Hz mains electricity).\n> > > + - name: FlickerFreq60Hz\n> > > + value: 2\n> > > + description: 60Hz flicker avoidance.\n> > > + Suppress flicker effects caused by lighting running with a 120Hz\n> > > + period (such as that produced by 60Hz mains electricity).\n\nHave you considered having 3 values here (off, manual and auto), and a\nAeFlickerPeriod control to set the manual value ? 50Hz and 60Hz are\ncommon values, but do they require special cases ?\n\n> > > + - name: FlickerCustom\n> > > + value: 3\n> > > + description: Custom flicker avoidance.\n> > > + Suppress flicker effects caused by lighting running with a period\n> > > + specified by the AeFlickerCustom control.\n> > > + \\sa AeFlickerCustom\n> > > + - name: FlickerAuto\n> > > + value: 4\n> > > + description: Automatic flicker period detection and avoidance.\n> > > + The system will automatically determine the most likely value of\n> > > + flicker period, and avoid flicker of this frequency.\n> > > +\n> > > + - AeFlickerCustom:\n> > > + type: int32_t\n> > > + description: Custom flicker period in microseconds.\n> > > + This value sets the current flicker period to avoid. It is used when\n> > > + AeFlickerMode is set to FlickerCustom.\n> > > +\n> > > + If this control is not available, then the setting of custom flicker\n> > > + periods is not supported.\n> > > +\n> > > + \\sa AeFlickerMode\n> > > +\n> > > + - AeFlickerDetected:\n> > > + type: int32_t\n> > > + description: Flicker period detected in microseconds.\n> > > + The value reported here indicates the currently detected flicker\n> > > + period, or zero if no flicker at all is detected.\n> > > +\n> > > + So in the case of 50Hz mains flicker, the value would be 10000\n> > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> > > + note that it is not required that any form of scene flicker is detected\n> > > + at all, so depending on the implementation, this metadata may just\n> > > + report zero or be entirely absent.\n> > > +\n> > > + AeFlickerDetected may also report other values for non-standard flicker\n> > > + periods.\n> > > +\n> > > + \\sa AeFlickerMode\n> >\n> > To clarify, AeFlickerDetected ought to be returned unconditionally on every\n> > frame, even if AeFlickerMode is set to something other than FlickerAuto, is that\n> > correct?\n> \n> Actually I think that's a good question. A lot of this stuff _could_\n> be quite implementation dependent, I think, and we can't really\n> mandate that implementations will do auto-detection in all\n> circumstances (some may well not do it al all).\n> \n> For example, if we're actually doing flicker avoidance at 50Hz, you\n> might expect that it gets tricky to detect 50Hz flicker. So what do we\n> report? Maybe we should require that the metadata is absent (or invent\n> a \"can't tell\" value) in these circumstances. We should probably be\n> clearer on this, so maybe another version is required\n\nTo answer this question I'd like to look at it from an application's\npoint of view. How do you envision this control being used in\napplications ?\n\n> > Other than that:\n> >\n> > Reviewed-by: Naushir Patuck \n> >\n> > > +\n> > > - Brightness:\n> > > type: float\n> > > description: |\n> > > @@ -843,23 +910,6 @@ controls:\n> > > value: 1\n> > > description: The lens shading map mode is available.\n> > >\n> > > - - SceneFlicker:\n> > > - type: int32_t\n> > > - draft: true\n> > > - description: |\n> > > - Control to report the detected scene light frequency. Currently\n> > > - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> > > - enum:\n> > > - - name: SceneFickerOff\n> > > - value: 0\n> > > - description: No flickering detected.\n> > > - - name: SceneFicker50Hz\n> > > - value: 1\n> > > - description: 50Hz flickering detected.\n> > > - - name: SceneFicker60Hz\n> > > - value: 2\n> > > - description: 60Hz flickering detected.\n> > > -\n> > > - PipelineDepth:\n> > > type: int32_t\n> > > draft: true","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 375A1C326B\n\tfor ;\n\tTue, 4 Apr 2023 06:06:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FFF16271C;\n\tTue, 4 Apr 2023 08:06: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 3CF8C603A4\n\tfor ;\n\tTue, 4 Apr 2023 08:06:30 +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 B93F47F8;\n\tTue, 4 Apr 2023 08:06:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680588392;\n\tbh=YHJdmQEIR3a9P5fdPHcohZP2vRKLEarG2jdplKqMiBY=;\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=UGkpsBugNWO9Y1swIMhlXBPQG0AeDKKujmdldkaznkFJqVnZs4S8qNXd05/w9+JTS\n\tB/IYAIKd+JsaFr0hd/zJh+uPhMm038US16AKn35dSQrYGPIRGu3ldJqF6zTMUBtjEh\n\twpgk6zSjy+goYQNQVJR1OAuCzsDDjvyfLBv+2Vh4Jomv/fc3RJZErrfvtAr+svcUYl\n\tN13ZbXe85trmL9yyzzV9+gFaH0Jtsq+EI9dnGSZL38H8qbCQI5z73KMVOnBN8i+Oxl\n\tVpQOcnyBVlrIuWPsFDHUTABVhtRLFZX0PZoY+kCx/wf9Q5yt7dHEIaHZlU2mBLuJXR\n\th2Dqkk2YZTzPw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680588389;\n\tbh=YHJdmQEIR3a9P5fdPHcohZP2vRKLEarG2jdplKqMiBY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ii8u3z9PQuwuO8iqM8z1bHofNMZhan+GtCSg3g7usdykQQBcN7AnfZSCw83v2A6n4\n\tDKvSQfwZrxk4w2xMP4TNInflqK0fDeiq4ueAXI4T9qsgLWllwC6Vdc7tQC1M9jKsmy\n\t4I8JA0xONwOr5cqpwpoRi7C6OFH7xww0mHefTYLs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ii8u3z9P\"; dkim-atps=neutral","Date":"Tue, 4 Apr 2023 09:06:36 +0300","To":"David Plowman ","Message-ID":"<20230404060636.GR16648@pendragon.ideasonboard.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>\n\t\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 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":26839,"web_url":"https://patchwork.libcamera.org/comment/26839/","msgid":"","date":"2023-04-04T10:40:53","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the review.\n\nOn Tue, 4 Apr 2023 at 07:06, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 03, 2023 at 09:20:39AM +0100, David Plowman via libcamera-devel wrote:\n> > On Mon, 3 Apr 2023 at 08:58, Naushir Patuck wrote:\n> > > On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel wrote:\n> > > >\n> > > > Flicker is the term used to describe brightness banding or oscillation\n> > > > of images caused typically by artificial lighting driven by a 50 or\n> > > > 60Hz mains supply. We add three controls intended to be used by\n> > > > AEC/AGC algorithms:\n> > > >\n> > > > AeFlickerMode to enable flicker avoidance.\n> > > >\n> > > > AeFlickerCustom to set custom flicker periods.\n> > > >\n> > > > AeFlickerDetected to report any flicker that is currently detected.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> > > > 1 file changed, 67 insertions(+), 17 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index adea5f90..b472050c 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -156,6 +156,73 @@ controls:\n> > > > control of which features should be automatically adjusted shouldn't\n> > > > better be handled through a separate AE mode control.\n> > > >\n> > > > + - AeFlickerMode:\n> > > > + type: int32_t\n> > > > + description: |\n> > > > + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> > > > + algorithm attempts to hide flicker effects caused by the duty cycle of\n> > > > + artificial lighting.\n> > > > +\n> > > > + Although implementation dependent, many algorithms for \"flicker\n> > > > + avoidance\" work by restricting this exposure time to integer multiples\n> > > > + of the cycle period, wherever possible.\n> > > > +\n> > > > + Implementations may not support all of the flicker modes listed below.\n> > > > +\n> > > > + enum:\n> > > > + - name: FlickerOff\n> > > > + value: 0\n> > > > + description: No flicker avoidance is performed.\n> > > > + - name: FlickerFreq50Hz\n> > > > + value: 1\n> > > > + description: 50Hz flicker avoidance.\n> > > > + Suppress flicker effects caused by lighting running with a 100Hz\n> > > > + period (such as that produced by 50Hz mains electricity).\n> > > > + - name: FlickerFreq60Hz\n> > > > + value: 2\n> > > > + description: 60Hz flicker avoidance.\n> > > > + Suppress flicker effects caused by lighting running with a 120Hz\n> > > > + period (such as that produced by 60Hz mains electricity).\n>\n> Have you considered having 3 values here (off, manual and auto), and a\n> AeFlickerPeriod control to set the manual value ? 50Hz and 60Hz are\n> common values, but do they require special cases ?\n\nInteresting thought, maybe I was following Android too slavishly. I\nbelieve it to be the case that almost no one will care about anything\nother than 50/60Hz so making those convenient seems helpful, but a\nmanual mode like this would be fine too. Too many choices... need to\nthink!\n\n>\n> > > > + - name: FlickerCustom\n> > > > + value: 3\n> > > > + description: Custom flicker avoidance.\n> > > > + Suppress flicker effects caused by lighting running with a period\n> > > > + specified by the AeFlickerCustom control.\n> > > > + \\sa AeFlickerCustom\n> > > > + - name: FlickerAuto\n> > > > + value: 4\n> > > > + description: Automatic flicker period detection and avoidance.\n> > > > + The system will automatically determine the most likely value of\n> > > > + flicker period, and avoid flicker of this frequency.\n> > > > +\n> > > > + - AeFlickerCustom:\n> > > > + type: int32_t\n> > > > + description: Custom flicker period in microseconds.\n> > > > + This value sets the current flicker period to avoid. It is used when\n> > > > + AeFlickerMode is set to FlickerCustom.\n> > > > +\n> > > > + If this control is not available, then the setting of custom flicker\n> > > > + periods is not supported.\n> > > > +\n> > > > + \\sa AeFlickerMode\n> > > > +\n> > > > + - AeFlickerDetected:\n> > > > + type: int32_t\n> > > > + description: Flicker period detected in microseconds.\n> > > > + The value reported here indicates the currently detected flicker\n> > > > + period, or zero if no flicker at all is detected.\n> > > > +\n> > > > + So in the case of 50Hz mains flicker, the value would be 10000\n> > > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> > > > + note that it is not required that any form of scene flicker is detected\n> > > > + at all, so depending on the implementation, this metadata may just\n> > > > + report zero or be entirely absent.\n> > > > +\n> > > > + AeFlickerDetected may also report other values for non-standard flicker\n> > > > + periods.\n> > > > +\n> > > > + \\sa AeFlickerMode\n> > >\n> > > To clarify, AeFlickerDetected ought to be returned unconditionally on every\n> > > frame, even if AeFlickerMode is set to something other than FlickerAuto, is that\n> > > correct?\n> >\n> > Actually I think that's a good question. A lot of this stuff _could_\n> > be quite implementation dependent, I think, and we can't really\n> > mandate that implementations will do auto-detection in all\n> > circumstances (some may well not do it al all).\n> >\n> > For example, if we're actually doing flicker avoidance at 50Hz, you\n> > might expect that it gets tricky to detect 50Hz flicker. So what do we\n> > report? Maybe we should require that the metadata is absent (or invent\n> > a \"can't tell\" value) in these circumstances. We should probably be\n> > clearer on this, so maybe another version is required\n>\n> To answer this question I'd like to look at it from an application's\n> point of view. How do you envision this control being used in\n> applications ?\n\nYes, always a good idea to do this. Let me try and list a number of scenarios:\n\n1. Flicker avoidance permanently off. For users who don't care!\n\n2. A user or application might know what country they're in and set\nFlicker avoidance to 50/60Hz immediately, and leave it forever. These\nfolks would just use the \"manual\" mode.\n\n3. A user might want to rely on auto-detection to discover 50/60Hz\nflicker, then they set flicker avoidance to that frequency and leave\nit forever. Here we'd watch the \"flicker detected\" value, and as soon\nas we see 50 or 60Hz, we'd set the flicker avoidance manually and\nnever touch it again.\n\nAfter this, we might consider users with non-50/60Hz lights, or where\nthe flicker period might change. I think these use cases are way more\nniche.\n\n4. A user has some special lighting with a known period, and they want\nto set it manually. After that they leave it alone. Though somewhat\nniche, this sounds plausible to me, and is also something that is\neasily implemented so we should probably cover it.\n\n5. User has unknown lighting (that may not be 50/60Hz) and wants to be\ntold the flicker period so that they can cancel it. Obviously depends\non whether the implementation can support this. Niche certainly, but\nperhaps it could happen.\n\nAfter this I think there are cases where someone might want to detect\nflicker even when one particular frequency is being cancelled, perhaps\nso that they can change the flicker period. This sounds like the least\nrealistic end of the spectrum, and will definitely depend a lot on\nwhat the capabilities of the platform are.\n\nI also haven't talked about what \"auto\" means. There will be a class\nof users who just want to set \"auto\" and forget about it. It may\ndepend on the implementation what \"auto\" means, but I would expect\nsomething like case 3 (above) might be an obvious candidate. If a\nplatform can detect other flicker frequencies than the one it is\ncancelling, then \"auto\" could potentially be cleverer, though most of\nthe time there's no benefit to this.\n\nSorry if that's got rather long! I guess the thing that perhaps leaves\nme the most questions is exactly what flicker detection means when a\nparticular period is already being cancelled. Do we need to\ndistinguish between \"I've detected another flicker frequency\" from\n\"I've detected that there is no flicker at all\" from \"I can't tell\" or\neven \"I can't detect a different frequency but I can't tell if the one\nI'm cancelling is still there\"?\n\nThanks\nDavid\n\n>\n> > > Other than that:\n> > >\n> > > Reviewed-by: Naushir Patuck \n> > >\n> > > > +\n> > > > - Brightness:\n> > > > type: float\n> > > > description: |\n> > > > @@ -843,23 +910,6 @@ controls:\n> > > > value: 1\n> > > > description: The lens shading map mode is available.\n> > > >\n> > > > - - SceneFlicker:\n> > > > - type: int32_t\n> > > > - draft: true\n> > > > - description: |\n> > > > - Control to report the detected scene light frequency. Currently\n> > > > - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> > > > - enum:\n> > > > - - name: SceneFickerOff\n> > > > - value: 0\n> > > > - description: No flickering detected.\n> > > > - - name: SceneFicker50Hz\n> > > > - value: 1\n> > > > - description: 50Hz flickering detected.\n> > > > - - name: SceneFicker60Hz\n> > > > - value: 2\n> > > > - description: 60Hz flickering detected.\n> > > > -\n> > > > - PipelineDepth:\n> > > > type: int32_t\n> > > > draft: true\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 EA634C0F2A\n\tfor ;\n\tTue, 4 Apr 2023 10:41:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13E0262724;\n\tTue, 4 Apr 2023 12:41:07 +0200 (CEST)","from mail-oa1-x31.google.com (mail-oa1-x31.google.com\n\t[IPv6:2001:4860:4864:20::31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 825FD626DB\n\tfor ;\n\tTue, 4 Apr 2023 12:41:05 +0200 (CEST)","by mail-oa1-x31.google.com with SMTP id\n\t586e51a60fabf-17aeb49429eso33995060fac.6\n\tfor ;\n\tTue, 04 Apr 2023 03:41:05 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680604867;\n\tbh=QdngyD8Lsd/sHLQUO0qGk25JUQ/EpIGA9WZWxZ9GFiM=;\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=ZLeBQT8vwradaok0e2TV740/M38gR+2Sv48/hKfjDS8M9pDsDI4qGOn+60shIeE0a\n\tFOjcdDPNJ3erdIjtupz4EW1Id5htG0D5PSVtSmZjA3I+BLwYaMzAM1KyvJzL9LWZ1P\n\tAnBjpzNBo5vlODA9S3j0bNy2hJUCvj9T0LZYQ+X/HHYEJR4/LRDhPkOXhxzee/HkqO\n\t4Q31f+0U7okABnj/fGSNzbgo3SIB+7OA9MQ2WJsP+WKvs9QbwXAYPDO8ZrBQ8/sCXn\n\tATNADROs5qpxWBMnPDXPwOgrNCprwuoKyatXhkqFGgJ63jh/vnWXBwVpqr4jNhPJB9\n\t43TsKo5h897Pg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1680604864;\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=9ELsF8Z0qEsVPPLT4BsKK9cobCvzOi8a7eWMYln2LYk=;\n\tb=IcK6vSlifk4JmFbgcrzvL5y4TXTU1hq6u+jmYuuuGT/xQXxkFjZe8808kL5cPi717V\n\t+LH5DFHRnPJzZAbgkuCAlTjTAuC131n5xI7jobZc0g6tMBEwv+sU+HrB7Vj9fVGZxLaz\n\tFa187S0G4rVIVjHRC64aejLizYKb6+5fVcHCRBMQRi4EU3mFt0a01hufgObfi2PVMv1O\n\tbb4Y9/ouW7Do1hWWZhjB3RtZ/l3pslf4+9EwhdS5fBGZBkZ0ERQVWzhZEnAGgxr2Yrsi\n\tlsmgoNt+VzfX84/6KmDduq6fy4oUmeMSUDDEJi3bQSJgDEzRKzPJvPlAlcLyniLgbmNp\n\tWA7g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"IcK6vSli\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680604864;\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=9ELsF8Z0qEsVPPLT4BsKK9cobCvzOi8a7eWMYln2LYk=;\n\tb=PffE1A/fzFHRePC/AIzyuwiTSIdxAJhXm/hR94vAzgCYKC/YUAz2g2s7I/L7eMNw9l\n\tSBtXVLo0WOt2lonArZ7pFbZ+T0VCMGZ4A3a2KDQUFNNc2ZezvAB2ILPnug2P7aW7AeK+\n\t7AAwyVqm57iZq4cxrIOpdjPoAboaBpiXsFNTUcCsEhSHKSTxCL7fWjx/japAUkIZN5Vz\n\tkoRukZqRwmD4C0TbITTn3CVbrFv95hZAYn9unoRkqBjZi7EFAxsswg4vP9h0TymmOWJd\n\t53u/G64ws4hGVgiIXeqn2fRuW8eleROg826B/dQCgCwMv60G3cs5t+XF090Od7uQvf/A\n\tHSpw==","X-Gm-Message-State":"AAQBX9f2nXxATNWWHD31e15bf8LIvLAdZ3/3gDvAHeYZuOTUZE9hO2Oo\n\tBgc1WBprAyEXHkmRA4FdvaVY85NF/MUszPFubIqKxQ==","X-Google-Smtp-Source":"AKy350Y7rRggSjRQ2PjzotSWHbMif038NBjDHKUw2exQ6cUyFTQAXiu3PRCfkFUe3LvP2vS+31c3xg2dSXTHfJ+cb+8=","X-Received":"by 2002:a05:6870:b605:b0:17e:2e88:40dc with SMTP id\n\tcm5-20020a056870b60500b0017e2e8840dcmr954112oab.11.1680604864191;\n\tTue, 04 Apr 2023 03:41:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t<20230404060636.GR16648@pendragon.ideasonboard.com>","In-Reply-To":"<20230404060636.GR16648@pendragon.ideasonboard.com>","Date":"Tue, 4 Apr 2023 11:40:53 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":26846,"web_url":"https://patchwork.libcamera.org/comment/26846/","msgid":"<20230405020656.GC9915@pendragon.ideasonboard.com>","date":"2023-04-05T02:06:56","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Tue, Apr 04, 2023 at 11:40:53AM +0100, David Plowman wrote:\n> On Tue, 4 Apr 2023 at 07:06, Laurent Pinchart wrote:\n> > On Mon, Apr 03, 2023 at 09:20:39AM +0100, David Plowman via libcamera-devel wrote:\n> > > On Mon, 3 Apr 2023 at 08:58, Naushir Patuck wrote:\n> > > > On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel wrote:\n> > > > >\n> > > > > Flicker is the term used to describe brightness banding or oscillation\n> > > > > of images caused typically by artificial lighting driven by a 50 or\n> > > > > 60Hz mains supply. We add three controls intended to be used by\n> > > > > AEC/AGC algorithms:\n> > > > >\n> > > > > AeFlickerMode to enable flicker avoidance.\n> > > > >\n> > > > > AeFlickerCustom to set custom flicker periods.\n> > > > >\n> > > > > AeFlickerDetected to report any flicker that is currently detected.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> > > > > 1 file changed, 67 insertions(+), 17 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > index adea5f90..b472050c 100644\n> > > > > --- a/src/libcamera/control_ids.yaml\n> > > > > +++ b/src/libcamera/control_ids.yaml\n> > > > > @@ -156,6 +156,73 @@ controls:\n> > > > > control of which features should be automatically adjusted shouldn't\n> > > > > better be handled through a separate AE mode control.\n> > > > >\n> > > > > + - AeFlickerMode:\n> > > > > + type: int32_t\n> > > > > + description: |\n> > > > > + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> > > > > + algorithm attempts to hide flicker effects caused by the duty cycle of\n> > > > > + artificial lighting.\n> > > > > +\n> > > > > + Although implementation dependent, many algorithms for \"flicker\n> > > > > + avoidance\" work by restricting this exposure time to integer multiples\n> > > > > + of the cycle period, wherever possible.\n> > > > > +\n> > > > > + Implementations may not support all of the flicker modes listed below.\n> > > > > +\n> > > > > + enum:\n> > > > > + - name: FlickerOff\n> > > > > + value: 0\n> > > > > + description: No flicker avoidance is performed.\n> > > > > + - name: FlickerFreq50Hz\n> > > > > + value: 1\n> > > > > + description: 50Hz flicker avoidance.\n> > > > > + Suppress flicker effects caused by lighting running with a 100Hz\n> > > > > + period (such as that produced by 50Hz mains electricity).\n> > > > > + - name: FlickerFreq60Hz\n> > > > > + value: 2\n> > > > > + description: 60Hz flicker avoidance.\n> > > > > + Suppress flicker effects caused by lighting running with a 120Hz\n> > > > > + period (such as that produced by 60Hz mains electricity).\n> >\n> > Have you considered having 3 values here (off, manual and auto), and a\n> > AeFlickerPeriod control to set the manual value ? 50Hz and 60Hz are\n> > common values, but do they require special cases ?\n> \n> Interesting thought, maybe I was following Android too slavishly. I\n> believe it to be the case that almost no one will care about anything\n> other than 50/60Hz so making those convenient seems helpful, but a\n> manual mode like this would be fine too. Too many choices... need to\n> think!\n\nWith the world (or at least the parts I'm used to) switching to LED\nlighting, won't automatic frequency detection become more relevant than\n50/60Hz manual selection ?\n\n> > > > > + - name: FlickerCustom\n> > > > > + value: 3\n> > > > > + description: Custom flicker avoidance.\n> > > > > + Suppress flicker effects caused by lighting running with a period\n> > > > > + specified by the AeFlickerCustom control.\n> > > > > + \\sa AeFlickerCustom\n> > > > > + - name: FlickerAuto\n> > > > > + value: 4\n> > > > > + description: Automatic flicker period detection and avoidance.\n> > > > > + The system will automatically determine the most likely value of\n> > > > > + flicker period, and avoid flicker of this frequency.\n> > > > > +\n> > > > > + - AeFlickerCustom:\n> > > > > + type: int32_t\n> > > > > + description: Custom flicker period in microseconds.\n> > > > > + This value sets the current flicker period to avoid. It is used when\n> > > > > + AeFlickerMode is set to FlickerCustom.\n> > > > > +\n> > > > > + If this control is not available, then the setting of custom flicker\n> > > > > + periods is not supported.\n> > > > > +\n> > > > > + \\sa AeFlickerMode\n> > > > > +\n> > > > > + - AeFlickerDetected:\n> > > > > + type: int32_t\n> > > > > + description: Flicker period detected in microseconds.\n> > > > > + The value reported here indicates the currently detected flicker\n> > > > > + period, or zero if no flicker at all is detected.\n> > > > > +\n> > > > > + So in the case of 50Hz mains flicker, the value would be 10000\n> > > > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> > > > > + note that it is not required that any form of scene flicker is detected\n> > > > > + at all, so depending on the implementation, this metadata may just\n> > > > > + report zero or be entirely absent.\n> > > > > +\n> > > > > + AeFlickerDetected may also report other values for non-standard flicker\n> > > > > + periods.\n> > > > > +\n> > > > > + \\sa AeFlickerMode\n> > > >\n> > > > To clarify, AeFlickerDetected ought to be returned unconditionally on every\n> > > > frame, even if AeFlickerMode is set to something other than FlickerAuto, is that\n> > > > correct?\n> > >\n> > > Actually I think that's a good question. A lot of this stuff _could_\n> > > be quite implementation dependent, I think, and we can't really\n> > > mandate that implementations will do auto-detection in all\n> > > circumstances (some may well not do it al all).\n> > >\n> > > For example, if we're actually doing flicker avoidance at 50Hz, you\n> > > might expect that it gets tricky to detect 50Hz flicker. So what do we\n> > > report? Maybe we should require that the metadata is absent (or invent\n> > > a \"can't tell\" value) in these circumstances. We should probably be\n> > > clearer on this, so maybe another version is required\n> >\n> > To answer this question I'd like to look at it from an application's\n> > point of view. How do you envision this control being used in\n> > applications ?\n> \n> Yes, always a good idea to do this. Let me try and list a number of scenarios:\n> \n> 1. Flicker avoidance permanently off. For users who don't care!\n\nAnd for controlled environment where the user knows there's no lighting\nflicker.\n\n> 2. A user or application might know what country they're in and set\n> Flicker avoidance to 50/60Hz immediately, and leave it forever. These\n> folks would just use the \"manual\" mode.\n> \n> 3. A user might want to rely on auto-detection to discover 50/60Hz\n> flicker, then they set flicker avoidance to that frequency and leave\n> it forever. Here we'd watch the \"flicker detected\" value, and as soon\n> as we see 50 or 60Hz, we'd set the flicker avoidance manually and\n> never touch it again.\n\nWouldn't using auto mode be simpler for applications in this case ?\n\n> After this, we might consider users with non-50/60Hz lights, or where\n> the flicker period might change. I think these use cases are way more\n> niche.\n\nAs mentioned in a reply to the cover letter, won't non-50/60Hz\nfrequencies become increasingly important with LED lighting ?\n\n> 4. A user has some special lighting with a known period, and they want\n> to set it manually. After that they leave it alone. Though somewhat\n> niche, this sounds plausible to me, and is also something that is\n> easily implemented so we should probably cover it.\n\nI agree with both points, it's a bit niche, but I see no reason not to\nsupport it.\n\n> 5. User has unknown lighting (that may not be 50/60Hz) and wants to be\n> told the flicker period so that they can cancel it. Obviously depends\n> on whether the implementation can support this. Niche certainly, but\n> perhaps it could happen.\n> \n> After this I think there are cases where someone might want to detect\n> flicker even when one particular frequency is being cancelled, perhaps\n> so that they can change the flicker period. This sounds like the least\n> realistic end of the spectrum, and will definitely depend a lot on\n> what the capabilities of the platform are.\n> \n> I also haven't talked about what \"auto\" means. There will be a class\n> of users who just want to set \"auto\" and forget about it. It may\n> depend on the implementation what \"auto\" means, but I would expect\n> something like case 3 (above) might be an obvious candidate. If a\n> platform can detect other flicker frequencies than the one it is\n> cancelling, then \"auto\" could potentially be cleverer, though most of\n> the time there's no benefit to this.\n\nThe way you've specified the behaviour of \"auto\" in your patch seems\nfine to me. Implementations can be cleverer than that if they want to.\n\n> Sorry if that's got rather long! I guess the thing that perhaps leaves\n> me the most questions is exactly what flicker detection means when a\n> particular period is already being cancelled. Do we need to\n> distinguish between \"I've detected another flicker frequency\" from\n> \"I've detected that there is no flicker at all\" from \"I can't tell\" or\n> even \"I can't detect a different frequency but I can't tell if the one\n> I'm cancelling is still there\"?\n\nThat's also the part that bothers me a bit, I'm not sure what would be\nbest for applications. Detecting and handling multiple frequencies seems\nto me like an advanced use case that may be best left to applications to\nmanage.\n\nRegarding reporting no flicker vs. unknown flicker frequency, is that\neven something we can do ? If you set the exposure time to a multiple of\nthe flicker half period, my understanding is that libcamera wouldn't be\nable to distinguish the two cases. We should then document that a zero\nvalue for the detected flicker frequency could mean either when in\nnon-off mode.\n\n> > > > Other than that:\n> > > >\n> > > > Reviewed-by: Naushir Patuck \n> > > >\n> > > > > +\n> > > > > - Brightness:\n> > > > > type: float\n> > > > > description: |\n> > > > > @@ -843,23 +910,6 @@ controls:\n> > > > > value: 1\n> > > > > description: The lens shading map mode is available.\n> > > > >\n> > > > > - - SceneFlicker:\n> > > > > - type: int32_t\n> > > > > - draft: true\n> > > > > - description: |\n> > > > > - Control to report the detected scene light frequency. Currently\n> > > > > - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> > > > > - enum:\n> > > > > - - name: SceneFickerOff\n> > > > > - value: 0\n> > > > > - description: No flickering detected.\n> > > > > - - name: SceneFicker50Hz\n> > > > > - value: 1\n> > > > > - description: 50Hz flickering detected.\n> > > > > - - name: SceneFicker60Hz\n> > > > > - value: 2\n> > > > > - description: 60Hz flickering detected.\n> > > > > -\n> > > > > - PipelineDepth:\n> > > > > type: int32_t\n> > > > > draft: true","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 156B0C326B\n\tfor ;\n\tWed, 5 Apr 2023 02:06:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 699EA6271F;\n\tWed, 5 Apr 2023 04:06:51 +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 E4F15626E3\n\tfor ;\n\tWed, 5 Apr 2023 04:06:49 +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 6EF06905;\n\tWed, 5 Apr 2023 04:06:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680660411;\n\tbh=3Lqi53XzgxeIqa+4owL/Nwwku3hU8noOUiW8wy7NPW8=;\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=kRNl+OHdzeOBT1v6vQph9EjlQPSIezF5SioWWcojjvHvqrjZioMBd6j4JRhtOjpo0\n\t1a/MOYgGXDarUkWUGVcMPkAlUvNG1jtkbw18GmVOFHvhlNyfw99MR6LB8WAJ+0EQOn\n\tnLKJnZUt5uRbI18h0E2sLvnoGVeE09b+Xu1ZRm1ZPPcjsOPl+YdDX7XDocxohOPUuV\n\tPX7R8tPY7MkXLlK5u+kPjwKGZ8hdHHDf6KrzepDuufK1L9QTBeEDZ/nMxiN3LdprVd\n\t6oK3nRwRYOk8KDJIDpL7JtOWUg+u+SpuADPfD90xSJi7DmtnNbFUMtnOE4CVdLx8wx\n\t39PNEa+4rb0yg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680660409;\n\tbh=3Lqi53XzgxeIqa+4owL/Nwwku3hU8noOUiW8wy7NPW8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YELTn5kmlzgNqnZ3DGlO2YSXMl5yXc08G7j2nlbw2qMpxgiiNt8IslQrg2TJ5mNX4\n\tFJPIOKTSTFplRfOqO4vRg/JZOVCpUYSuwIYehFqCgh0PTcC/Z/3JZtIFryrektb9Un\n\tDzpMSpN0BCW5OXYjbCPY3U8PogiH5BD2etMFzUu4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YELTn5km\"; dkim-atps=neutral","Date":"Wed, 5 Apr 2023 05:06:56 +0300","To":"David Plowman ","Message-ID":"<20230405020656.GC9915@pendragon.ideasonboard.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t<20230404060636.GR16648@pendragon.ideasonboard.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 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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":27448,"web_url":"https://patchwork.libcamera.org/comment/27448/","msgid":"<20230703091849.GG9285@pendragon.ideasonboard.com>","date":"2023-07-03T09:18:49","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nResurecting this old thread.\n\nOn Wed, Apr 05, 2023 at 05:06:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Tue, Apr 04, 2023 at 11:40:53AM +0100, David Plowman wrote:\n> > On Tue, 4 Apr 2023 at 07:06, Laurent Pinchart wrote:\n> > > On Mon, Apr 03, 2023 at 09:20:39AM +0100, David Plowman via libcamera-devel wrote:\n> > > > On Mon, 3 Apr 2023 at 08:58, Naushir Patuck wrote:\n> > > > > On Tue, 28 Mar 2023 at 09:55, David Plowman via libcamera-devel wrote:\n> > > > > >\n> > > > > > Flicker is the term used to describe brightness banding or oscillation\n> > > > > > of images caused typically by artificial lighting driven by a 50 or\n> > > > > > 60Hz mains supply. We add three controls intended to be used by\n> > > > > > AEC/AGC algorithms:\n> > > > > >\n> > > > > > AeFlickerMode to enable flicker avoidance.\n> > > > > >\n> > > > > > AeFlickerCustom to set custom flicker periods.\n> > > > > >\n> > > > > > AeFlickerDetected to report any flicker that is currently detected.\n> > > > > >\n> > > > > > Signed-off-by: David Plowman \n> > > > > > ---\n> > > > > > src/libcamera/control_ids.yaml | 84 +++++++++++++++++++++++++++-------\n> > > > > > 1 file changed, 67 insertions(+), 17 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > index adea5f90..b472050c 100644\n> > > > > > --- a/src/libcamera/control_ids.yaml\n> > > > > > +++ b/src/libcamera/control_ids.yaml\n> > > > > > @@ -156,6 +156,73 @@ controls:\n> > > > > > control of which features should be automatically adjusted shouldn't\n> > > > > > better be handled through a separate AE mode control.\n> > > > > >\n> > > > > > + - AeFlickerMode:\n> > > > > > + type: int32_t\n> > > > > > + description: |\n> > > > > > + Set the flicker mode, which determines whether, and how, the AGC/AEC\n> > > > > > + algorithm attempts to hide flicker effects caused by the duty cycle of\n> > > > > > + artificial lighting.\n> > > > > > +\n> > > > > > + Although implementation dependent, many algorithms for \"flicker\n> > > > > > + avoidance\" work by restricting this exposure time to integer multiples\n> > > > > > + of the cycle period, wherever possible.\n> > > > > > +\n> > > > > > + Implementations may not support all of the flicker modes listed below.\n> > > > > > +\n> > > > > > + enum:\n> > > > > > + - name: FlickerOff\n> > > > > > + value: 0\n> > > > > > + description: No flicker avoidance is performed.\n> > > > > > + - name: FlickerFreq50Hz\n> > > > > > + value: 1\n> > > > > > + description: 50Hz flicker avoidance.\n> > > > > > + Suppress flicker effects caused by lighting running with a 100Hz\n> > > > > > + period (such as that produced by 50Hz mains electricity).\n> > > > > > + - name: FlickerFreq60Hz\n> > > > > > + value: 2\n> > > > > > + description: 60Hz flicker avoidance.\n> > > > > > + Suppress flicker effects caused by lighting running with a 120Hz\n> > > > > > + period (such as that produced by 60Hz mains electricity).\n> > >\n> > > Have you considered having 3 values here (off, manual and auto), and a\n> > > AeFlickerPeriod control to set the manual value ? 50Hz and 60Hz are\n> > > common values, but do they require special cases ?\n> > \n> > Interesting thought, maybe I was following Android too slavishly. I\n> > believe it to be the case that almost no one will care about anything\n> > other than 50/60Hz so making those convenient seems helpful, but a\n> > manual mode like this would be fine too. Too many choices... need to\n> > think!\n> \n> With the world (or at least the parts I'm used to) switching to LED\n> lighting, won't automatic frequency detection become more relevant than\n> 50/60Hz manual selection ?\n\nWhat are everyone's thoughts on this ?\n\n> > > > > > + - name: FlickerCustom\n> > > > > > + value: 3\n> > > > > > + description: Custom flicker avoidance.\n> > > > > > + Suppress flicker effects caused by lighting running with a period\n> > > > > > + specified by the AeFlickerCustom control.\n> > > > > > + \\sa AeFlickerCustom\n> > > > > > + - name: FlickerAuto\n> > > > > > + value: 4\n> > > > > > + description: Automatic flicker period detection and avoidance.\n> > > > > > + The system will automatically determine the most likely value of\n> > > > > > + flicker period, and avoid flicker of this frequency.\n> > > > > > +\n> > > > > > + - AeFlickerCustom:\n> > > > > > + type: int32_t\n> > > > > > + description: Custom flicker period in microseconds.\n> > > > > > + This value sets the current flicker period to avoid. It is used when\n> > > > > > + AeFlickerMode is set to FlickerCustom.\n> > > > > > +\n> > > > > > + If this control is not available, then the setting of custom flicker\n> > > > > > + periods is not supported.\n> > > > > > +\n> > > > > > + \\sa AeFlickerMode\n> > > > > > +\n> > > > > > + - AeFlickerDetected:\n> > > > > > + type: int32_t\n> > > > > > + description: Flicker period detected in microseconds.\n> > > > > > + The value reported here indicates the currently detected flicker\n> > > > > > + period, or zero if no flicker at all is detected.\n> > > > > > +\n> > > > > > + So in the case of 50Hz mains flicker, the value would be 10000\n> > > > > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker. But\n> > > > > > + note that it is not required that any form of scene flicker is detected\n> > > > > > + at all, so depending on the implementation, this metadata may just\n> > > > > > + report zero or be entirely absent.\n> > > > > > +\n> > > > > > + AeFlickerDetected may also report other values for non-standard flicker\n> > > > > > + periods.\n> > > > > > +\n> > > > > > + \\sa AeFlickerMode\n> > > > >\n> > > > > To clarify, AeFlickerDetected ought to be returned unconditionally on every\n> > > > > frame, even if AeFlickerMode is set to something other than FlickerAuto, is that\n> > > > > correct?\n> > > >\n> > > > Actually I think that's a good question. A lot of this stuff _could_\n> > > > be quite implementation dependent, I think, and we can't really\n> > > > mandate that implementations will do auto-detection in all\n> > > > circumstances (some may well not do it al all).\n> > > >\n> > > > For example, if we're actually doing flicker avoidance at 50Hz, you\n> > > > might expect that it gets tricky to detect 50Hz flicker. So what do we\n> > > > report? Maybe we should require that the metadata is absent (or invent\n> > > > a \"can't tell\" value) in these circumstances. We should probably be\n> > > > clearer on this, so maybe another version is required\n> > >\n> > > To answer this question I'd like to look at it from an application's\n> > > point of view. How do you envision this control being used in\n> > > applications ?\n> > \n> > Yes, always a good idea to do this. Let me try and list a number of scenarios:\n> > \n> > 1. Flicker avoidance permanently off. For users who don't care!\n> \n> And for controlled environment where the user knows there's no lighting\n> flicker.\n\nAnd for users who don't care, shouldn't the camera default to auto if it\nsupports that mode ? That's Android's default, and I think it makes\nsense for applications that target consumer users. I'd expect\napplications that target use cases where it's important to disable\nflicker avoidance to be able to do so manually.\n\n> > 2. A user or application might know what country they're in and set\n> > Flicker avoidance to 50/60Hz immediately, and leave it forever. These\n> > folks would just use the \"manual\" mode.\n> > \n> > 3. A user might want to rely on auto-detection to discover 50/60Hz\n> > flicker, then they set flicker avoidance to that frequency and leave\n> > it forever. Here we'd watch the \"flicker detected\" value, and as soon\n> > as we see 50 or 60Hz, we'd set the flicker avoidance manually and\n> > never touch it again.\n> \n> Wouldn't using auto mode be simpler for applications in this case ?\n\nIf the IPA has the ability to detect the frequency, I believe we should\nrequire the auto mode to be supported, in which case applications should\nshould use it.\n\nA posibly more common use case for 50/60Hz detection reported to\napplications is users operating in a \"standard\" 50/60Hz environment who\nwant to use manual exposure.\n\n> > After this, we might consider users with non-50/60Hz lights, or where\n> > the flicker period might change. I think these use cases are way more\n> > niche.\n> \n> As mentioned in a reply to the cover letter, won't non-50/60Hz\n> frequencies become increasingly important with LED lighting ?\n> \n> > 4. A user has some special lighting with a known period, and they want\n> > to set it manually. After that they leave it alone. Though somewhat\n> > niche, this sounds plausible to me, and is also something that is\n> > easily implemented so we should probably cover it.\n> \n> I agree with both points, it's a bit niche, but I see no reason not to\n> support it.\n> \n> > 5. User has unknown lighting (that may not be 50/60Hz) and wants to be\n> > told the flicker period so that they can cancel it. Obviously depends\n> > on whether the implementation can support this. Niche certainly, but\n> > perhaps it could happen.\n> > \n> > After this I think there are cases where someone might want to detect\n> > flicker even when one particular frequency is being cancelled, perhaps\n> > so that they can change the flicker period. This sounds like the least\n> > realistic end of the spectrum, and will definitely depend a lot on\n> > what the capabilities of the platform are.\n\nAgreed.\n\n> > I also haven't talked about what \"auto\" means. There will be a class\n> > of users who just want to set \"auto\" and forget about it. It may\n> > depend on the implementation what \"auto\" means, but I would expect\n> > something like case 3 (above) might be an obvious candidate. If a\n> > platform can detect other flicker frequencies than the one it is\n> > cancelling, then \"auto\" could potentially be cleverer, though most of\n> > the time there's no benefit to this.\n> \n> The way you've specified the behaviour of \"auto\" in your patch seems\n> fine to me. Implementations can be cleverer than that if they want to.\n> \n> > Sorry if that's got rather long! I guess the thing that perhaps leaves\n> > me the most questions is exactly what flicker detection means when a\n> > particular period is already being cancelled. Do we need to\n> > distinguish between \"I've detected another flicker frequency\" from\n> > \"I've detected that there is no flicker at all\" from \"I can't tell\" or\n> > even \"I can't detect a different frequency but I can't tell if the one\n> > I'm cancelling is still there\"?\n> \n> That's also the part that bothers me a bit, I'm not sure what would be\n> best for applications. Detecting and handling multiple frequencies seems\n> to me like an advanced use case that may be best left to applications to\n> manage.\n> \n> Regarding reporting no flicker vs. unknown flicker frequency, is that\n> even something we can do ? If you set the exposure time to a multiple of\n> the flicker half period, my understanding is that libcamera wouldn't be\n> able to distinguish the two cases. We should then document that a zero\n> value for the detected flicker frequency could mean either when in\n> non-off mode.\n> \n> > > > > Other than that:\n> > > > >\n> > > > > Reviewed-by: Naushir Patuck \n> > > > >\n> > > > > > +\n> > > > > > - Brightness:\n> > > > > > type: float\n> > > > > > description: |\n> > > > > > @@ -843,23 +910,6 @@ controls:\n> > > > > > value: 1\n> > > > > > description: The lens shading map mode is available.\n> > > > > >\n> > > > > > - - SceneFlicker:\n> > > > > > - type: int32_t\n> > > > > > - draft: true\n> > > > > > - description: |\n> > > > > > - Control to report the detected scene light frequency. Currently\n> > > > > > - identical to ANDROID_STATISTICS_SCENE_FLICKER.\n> > > > > > - enum:\n> > > > > > - - name: SceneFickerOff\n> > > > > > - value: 0\n> > > > > > - description: No flickering detected.\n> > > > > > - - name: SceneFicker50Hz\n> > > > > > - value: 1\n> > > > > > - description: 50Hz flickering detected.\n> > > > > > - - name: SceneFicker60Hz\n> > > > > > - value: 2\n> > > > > > - description: 60Hz flickering detected.\n> > > > > > -\n> > > > > > - PipelineDepth:\n> > > > > > type: int32_t\n> > > > > > draft: true","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 C7514BDC71\n\tfor ;\n\tMon, 3 Jul 2023 09:18:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFB94628C0;\n\tMon, 3 Jul 2023 11:18: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 F228F61E37\n\tfor ;\n\tMon, 3 Jul 2023 11:18:48 +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 4CDFC558;\n\tMon, 3 Jul 2023 11:18:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688375931;\n\tbh=n8LD4jvbjtsLDvxGGDOSui+s7xScT6da8krtAvfY1QA=;\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=EBJqFNKwsldRto1rXIeyTkBFNDlcm9KgAAJyR144sKiDKuhTmMLwjIOumVcB5mT4z\n\tw+EaMcf68FxVZ303qxuAl1AFkGwrl/nV/KedUQAqTfVethzb2QhmWP6g2kAvEvm4xX\n\to7C0XVdZvcKfNV/4GLImz2M3sRarlAx00a38NG/OtjhsB8EVSGyShn//K2P2URbRNT\n\tiLgNZ5Us2OKqqDPdPLkkhPIRF3A2NxB9Kh5BipkByX4sZ8Bj1Swx/LWkhSwYRLXfJq\n\tgLlOOYyQC9SMVfWqRlp0vl3z60QVjKS3RULtPs/lTy/bZT33V1y260s3gaS/7JHbqu\n\tsgL7ULq4P25mQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1688375885;\n\tbh=n8LD4jvbjtsLDvxGGDOSui+s7xScT6da8krtAvfY1QA=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=LHbNhKM1sEs3y5CatY4QAKcqi1XYgwF0vCBKXRmSy2gpL1xEqO+qrKZnA5FtxtLlS\n\tp2V0xrotXttN8nuo/+9NUcaLuZfcdu4OwDQG61yn3KdFYsxNHXXwaSmtN9wL8LrnUI\n\tdyyRIop2DLWkfHWiMzhQ8qyTP+7f2qJhJml8SAAU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LHbNhKM1\"; dkim-atps=neutral","Date":"Mon, 3 Jul 2023 12:18:49 +0300","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20230703091849.GG9285@pendragon.ideasonboard.com>","References":"<20230328085521.7409-1-david.plowman@raspberrypi.com>\n\t<20230328085521.7409-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t<20230404060636.GR16648@pendragon.ideasonboard.com>\n\t\n\t<20230405020656.GC9915@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230405020656.GC9915@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: controls: Add\n\tcontrols for AEC/AGC flicker avoidance","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\" "}}]