[{"id":27570,"web_url":"https://patchwork.libcamera.org/comment/27570/","msgid":"","date":"2023-07-18T07:04:02","subject":"Re: [libcamera-devel] [PATCH v4 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 Thu, Jul 13, 2023 at 04:12:17PM +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 | 79 ++++++++++++++++++++++++++--------\n> 1 file changed, 62 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 056886e6..f1629b89 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -156,6 +156,68 @@ 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\nI would here add that if the AeEnable control is set to false\n(\"manual mode\") this setting has no effect.\n\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> + By default the system will start in FlickerAuto mode if this is\n> + supported, otherwise the flicker mode will be set to FlickerOff.\n> +\n> + enum:\n> + - name: FlickerOff\n> + value: 0\n> + description: No flicker avoidance is performed.\n> + - name: FlickerCustom\n> + value: 1\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: 2\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. Once flicker\n> + is being corrected, it is implementation dependent whether the\n> + system is still able to detect a change in the flicker period.\n\n\"... during a single streaming session\" ?\n\nI presume a camera stop/start sequence re-init the algorithms (so that\nif you travel between countries far apart in the world (or different\nparts of Japan) while using the camera the new flicker period will be\ndetected).\n\n> +\n> + - AeFlickerCustom:\n\nNit: Custom or Manual ? I don't recall if we discussed it or not\n\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> + To cancel 50Hz mains flicker, this should be set to 10000 (corresponding\n> + to 100Hz), or 8333 (120Hz) for 60Hz mains.\n> +\n> + If this control is not available, then the setting of custom flicker\n> + periods is not supported.\n\nHow can you set the control if it is not available ? This seems more\nlike a note for PH implementer not to register this control if\nFlickerCustom is not available ?\n\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> + In the case of 50Hz mains flicker, the value would be 10000\n> + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker.\n> +\n> + It is implementation dependent exactly when the system is able\n> + to detect and report the flicker period.\n\nDoes flicker period detection need a warm-up phase where we can expect\nthe first results to be innacurate or as soon as we receive this value\nin metadata we can assume it is correct in your experience ?\n\nThanks\n j\n\n> +\n> + \\sa AeFlickerMode\n> +\n> - Brightness:\n> type: float\n> description: |\n> @@ -850,23 +912,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 81F51BDB1C\n\tfor ;\n\tTue, 18 Jul 2023 07:04:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B211A628C0;\n\tTue, 18 Jul 2023 09:04:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 454FE61E2A\n\tfor ;\n\tTue, 18 Jul 2023 09:04:07 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6436968;\n\tTue, 18 Jul 2023 09:03:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689663848;\n\tbh=OG81RxDahIAIS4LN0M4XFMRDHstQRXtRo+jnhdHGWvY=;\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=qb0/3ms9hK6LDXxz8/Nr32XqkbuNMnqo8zX0JgCmYHDx+5dlkC37rVOpvw+Tn1ZH+\n\t1WNgKvxMTgTftXH/jx6zhCaqeeZ7eZkWnzdffVqa6ih+BYTjvbnH5sJS3Wi4PxVbWq\n\tcfgubNRM1MJcnkvRmhwM8F1wSWP41SVMFsCaxBso+Bz6SRhXtR333ebvK3om5/LuB9\n\tLx4m0G3Hm5nBAalFJ0408Q4bCf817pNp4tw/hf32nPlGbCebt7alLZQcNzrohtp5YF\n\t/qsiIJ0IJ86/iayPwcbYN+nGblrQYqtUY0K7B9Dpil218jiNBkXhPohrZVMpXDgWSh\n\to1BIQs+cPWrIw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689663793;\n\tbh=OG81RxDahIAIS4LN0M4XFMRDHstQRXtRo+jnhdHGWvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Etj0qdE8ZRUY8xXugLCUnr9HrEtklbDqEuK2IzPR3xJhq/gQAM9yJf/YhntbDJdpp\n\twqbpb11+vNUANqOTzFICQc0lGl/PkVG7Ksngbm2mPy3DkVF7QZ2ah4H0CFQoEMshuf\n\toyoxE6rUfZhxGsOz4xyIaWkZT7fV5WWtrCeSg1UE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Etj0qdE8\"; dkim-atps=neutral","Date":"Tue, 18 Jul 2023 09:04:02 +0200","To":"David Plowman ","Message-ID":"","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230713151218.26045-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":27574,"web_url":"https://patchwork.libcamera.org/comment/27574/","msgid":"","date":"2023-07-18T08:15:00","subject":"Re: [libcamera-devel] [PATCH v4 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 Jacopo\n\nThanks for the comments.\n\nOn Tue, 18 Jul 2023 at 08:04, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Thu, Jul 13, 2023 at 04:12:17PM +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 | 79 ++++++++++++++++++++++++++--------\n> > 1 file changed, 62 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 056886e6..f1629b89 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -156,6 +156,68 @@ 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> I would here add that if the AeEnable control is set to false\n> (\"manual mode\") this setting has no effect.\n\nYes, I think that seems reasonable!\n\n>\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> > + By default the system will start in FlickerAuto mode if this is\n> > + supported, otherwise the flicker mode will be set to FlickerOff.\n\nThis is what we said in Prague, though I'm not sure about it now. I\nthink I'd rather have it be off by default because that's the existing\nbehaviour. If Android wants this behaviour, it should perhaps go in\nthe Android adaptation layer. Thoughts?\n\n> > +\n> > + enum:\n> > + - name: FlickerOff\n> > + value: 0\n> > + description: No flicker avoidance is performed.\n> > + - name: FlickerCustom\n> > + value: 1\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: 2\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. Once flicker\n> > + is being corrected, it is implementation dependent whether the\n> > + system is still able to detect a change in the flicker period.\n>\n> \"... during a single streaming session\" ?\n>\n> I presume a camera stop/start sequence re-init the algorithms (so that\n> if you travel between countries far apart in the world (or different\n> parts of Japan) while using the camera the new flicker period will be\n> detected).\n\nWe don't normally re-initialise control algorithms between\nstop/configure/start calls. This is pretty critical to everything we\ndo - AE, AWB, ALSC and other things are all preserved.\n\nBut re-opening the camera obviously does restart everything from scratch.\n\nWe did briefly talk about places like Japan where it is more plausible\nthat you could go from a 50Hz to a 60Hz environment with the camera\nleft running. If anyone recalls exactly what we said that would be\ninteresting. My own feelings on this include:\n\n* It's a fairly marginal use-case, even in Japan. Let's make the basics work.\n* The detection algorithm could plausibly detect changes to\nfrequencies other than the one being handled, so this could probably\nbe supported (though it would be implementation dependent).\n\nWhat do others think?\n\n>\n> > +\n> > + - AeFlickerCustom:\n>\n> Nit: Custom or Manual ? I don't recall if we discussed it or not\n\nI agree, maybe \"manual\" is better. \"custom\" was the term left over\nfrom when we had explicit 50/60Hz modes.\n\n>\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> > + To cancel 50Hz mains flicker, this should be set to 10000 (corresponding\n> > + to 100Hz), or 8333 (120Hz) for 60Hz mains.\n> > +\n> > + If this control is not available, then the setting of custom flicker\n> > + periods is not supported.\n>\n> How can you set the control if it is not available ? This seems more\n> like a note for PH implementer not to register this control if\n> FlickerCustom is not available ?\n\nI guess it means you wouldn't be able to set the flicker period\n(\"manually\") yourself. I suppose you might still have \"off\" and\nperhaps \"auto\" settings available. Though it feels unlikely that an\nimplementation would support \"auto\" but not \"manual\", though you never\nknow for sure!\n\n>\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> > + In the case of 50Hz mains flicker, the value would be 10000\n> > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker.\n> > +\n> > + It is implementation dependent exactly when the system is able\n> > + to detect and report the flicker period.\n>\n> Does flicker period detection need a warm-up phase where we can expect\n> the first results to be innacurate or as soon as we receive this value\n> in metadata we can assume it is correct in your experience ?\n\nGood point. I think in practice an algorithm would want to see \"a few\nframes\" to confirm the detected flicker period before reporting and\ncancelling it, so it would make sense to set this once it's\n\"confirmed\" and the flicker is now going to be cancelled.\n\nAnyway, I'll put together another version shortly.\n\nThanks!\nDavid\n\n>\n> Thanks\n> j\n>\n> > +\n> > + \\sa AeFlickerMode\n> > +\n> > - Brightness:\n> > type: float\n> > description: |\n> > @@ -850,23 +912,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 C8E6ABDB1C\n\tfor ;\n\tTue, 18 Jul 2023 08:15:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C493628BC;\n\tTue, 18 Jul 2023 10:15:15 +0200 (CEST)","from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com\n\t[IPv6:2607:f8b0:4864:20::f29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF23861E2A\n\tfor ;\n\tTue, 18 Jul 2023 10:15:12 +0200 (CEST)","by mail-qv1-xf29.google.com with SMTP id\n\t6a1803df08f44-635e5b06aaeso28916296d6.0\n\tfor ;\n\tTue, 18 Jul 2023 01:15:12 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689668115;\n\tbh=WCc3fpf4hKP4M8MIOBfNvh3ytU1OyNDZKfjxYDyM5h4=;\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=j+QT4wLi2mfpvhTgg3E8UAA4LdcQeF4FXtf/g2MWKwrp2lIfuKGVa34o8dFGb7CSY\n\t3uzujI3732xRGg1bb/WZk1G126o4045Zw/YC6jhoVFQyWaarzRFwQgm4ufRfEQ5qoD\n\twvGuOgU/hsrgPx0X1ue/nORbyXsAZWlLjwVUlmQceU1LfDJwOTEoeREliiCCQZmSZA\n\tkt191EVrVz1RSgEtomsGSU/ZYyCbHkqhhVmXx4nx4hI7PGsyhxOWNrRBrokSK0WcRA\n\t3KzP48NubgCxWtgKxwnvxQ0q2GvLptxjixqmEngE6LvbaFsKXo+BGmkHTJgbD0ZKxo\n\toPbsA0L0L6teA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689668111; x=1692260111;\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=1HJCHcj23E3VgoLgUym9Q9qbF3N/lOLKVvqSJDZ7QFw=;\n\tb=hGEMTT9dSDOqsLhwzrIa4/B15EHVftHNCrwhjTFX/jVEhI16GlIEf7cBeVLVF/xAcj\n\t36wZgIKjW16yqEzVXO+tAn1fCY1n17h3i4aZ1RUyXy8PU2Mv+V8+8Z4PSkuZl4w+g6Ab\n\tyNiBHf3Xlj8VIdFweumkCe11siD/ebhpjrCey0adti6O4XuH6G96jXoPZ2oneAINRPxj\n\tHYPgUjolzaUxcqyZvJ3L/LiiQ4IVEYns/xEG5ASx3WTrU0RSDNzeHR/lsxD0x8O/JNbG\n\tKSlwwlyy02w+ViARiHTiN/Nk47nfPdlOcr/sdrLFPZFYkEmAJUYkC+L1idGpZM3o+c0h\n\tw9WA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"hGEMTT9d\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689668111; x=1692260111;\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=1HJCHcj23E3VgoLgUym9Q9qbF3N/lOLKVvqSJDZ7QFw=;\n\tb=PF1QHHkZO1VsGipaGGNZjYYJc6bzlLLztilN+xYPh3qCkg48YCZqKLkRS/G3xSFJeW\n\t/kHed3D8Fr74YAau8ahlEvrA4XwWPMao14WJZxnKCRud+SJjtcDB58RUEkL4i+Hs0zUN\n\thjoGBDvBaJ50hC4b0TtlSHx/JC/kavFKWWllixd46xy5R3MPVjoixn1PHDfSFuAa3CLx\n\tBlx+j1D9ufjlQet71bT01pP5nz9cFT0/qAcvsdQtQyxtnb4s4FXJbU67bMizfSuu0ORK\n\tWkAKM3radVp/MMsdxzLAHxdj6k1oxtMBisExcu44e/zR3gX/u79yCaMlgArGpFyt3E2k\n\t2jOQ==","X-Gm-Message-State":"ABy/qLZ2f4WYcAw8mxWyhfohbQ6Yc7cXOg42fOlYya5GyKqPOrkaKG6L\n\ts80CJgSGmKjlqbwkKdOywCz3aXx151IV/h9ybTXSjZBkCaGG3oPe","X-Google-Smtp-Source":"APBJJlEO79ZHE4w3QY3dgUhj83DjxQ9SY7nkCxy0aKw8u70X4IUGXc8MKZrVpmth5U1YLm5n0ZZ4XmUpI//Q4Bt6laA=","X-Received":"by 2002:a0c:be8a:0:b0:636:e4f:6b9a with SMTP id\n\tn10-20020a0cbe8a000000b006360e4f6b9amr1333800qvi.17.1689668111648;\n\tTue, 18 Jul 2023 01:15:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Tue, 18 Jul 2023 09:15:00 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 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":27579,"web_url":"https://patchwork.libcamera.org/comment/27579/","msgid":"","date":"2023-07-18T10:38:49","subject":"Re: [libcamera-devel] [PATCH v4 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, Jul 18, 2023 at 09:15:00AM +0100, David Plowman via libcamera-devel wrote:\n> Hi Jacopo\n>\n> Thanks for the comments.\n>\n> On Tue, 18 Jul 2023 at 08:04, Jacopo Mondi\n> wrote:\n> >\n> > Hi David\n> >\n> > On Thu, Jul 13, 2023 at 04:12:17PM +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 | 79 ++++++++++++++++++++++++++--------\n> > > 1 file changed, 62 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 056886e6..f1629b89 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -156,6 +156,68 @@ 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> > I would here add that if the AeEnable control is set to false\n> > (\"manual mode\") this setting has no effect.\n>\n> Yes, I think that seems reasonable!\n>\n> >\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> > > + By default the system will start in FlickerAuto mode if this is\n> > > + supported, otherwise the flicker mode will be set to FlickerOff.\n>\n> This is what we said in Prague, though I'm not sure about it now. I\n> think I'd rather have it be off by default because that's the existing\n> behaviour. If Android wants this behaviour, it should perhaps go in\n> the Android adaptation layer. Thoughts?\n>\n\nTo be honest, if anything like auto is available, I would expect it to\nbe used, as I don't see why an application wouldn't want to have it\nenabled.\n\nIOW it seems to me all applications will enable it anyway, if\navailable...\n\n> > > +\n> > > + enum:\n> > > + - name: FlickerOff\n> > > + value: 0\n> > > + description: No flicker avoidance is performed.\n> > > + - name: FlickerCustom\n> > > + value: 1\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: 2\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. Once flicker\n> > > + is being corrected, it is implementation dependent whether the\n> > > + system is still able to detect a change in the flicker period.\n> >\n> > \"... during a single streaming session\" ?\n> >\n> > I presume a camera stop/start sequence re-init the algorithms (so that\n> > if you travel between countries far apart in the world (or different\n> > parts of Japan) while using the camera the new flicker period will be\n> > detected).\n>\n> We don't normally re-initialise control algorithms between\n> stop/configure/start calls. This is pretty critical to everything we\n> do - AE, AWB, ALSC and other things are all preserved.\n>\n> But re-opening the camera obviously does restart everything from scratch.\n>\n> We did briefly talk about places like Japan where it is more plausible\n> that you could go from a 50Hz to a 60Hz environment with the camera\n> left running. If anyone recalls exactly what we said that would be\n> interesting. My own feelings on this include:\n>\n> * It's a fairly marginal use-case, even in Japan. Let's make the basics work.\n\nYeah, my comment was half a joke\n\n> * The detection algorithm could plausibly detect changes to\n> frequencies other than the one being handled, so this could probably\n> be supported (though it would be implementation dependent).\n>\n> What do others think?\n\nIt seems a rather marginal use case to me. Ignore my comment.\n\n>\n> >\n> > > +\n> > > + - AeFlickerCustom:\n> >\n> > Nit: Custom or Manual ? I don't recall if we discussed it or not\n>\n> I agree, maybe \"manual\" is better. \"custom\" was the term left over\n> from when we had explicit 50/60Hz modes.\n>\n> >\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> > > + To cancel 50Hz mains flicker, this should be set to 10000 (corresponding\n> > > + to 100Hz), or 8333 (120Hz) for 60Hz mains.\n> > > +\n> > > + If this control is not available, then the setting of custom flicker\n> > > + periods is not supported.\n> >\n> > How can you set the control if it is not available ? This seems more\n> > like a note for PH implementer not to register this control if\n> > FlickerCustom is not available ?\n>\n> I guess it means you wouldn't be able to set the flicker period\n> (\"manually\") yourself. I suppose you might still have \"off\" and\n> perhaps \"auto\" settings available. Though it feels unlikely that an\n> implementation would support \"auto\" but not \"manual\", though you never\n> know for sure!\n\nWhat I meant is that your phrasing sounded to me like like: \"if this\ncontrol is not available you cannot set it\", which seems... expected ?\nDid I get it wrong ?\n\n>\n> >\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> > > + In the case of 50Hz mains flicker, the value would be 10000\n> > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker.\n> > > +\n> > > + It is implementation dependent exactly when the system is able\n> > > + to detect and report the flicker period.\n> >\n> > Does flicker period detection need a warm-up phase where we can expect\n> > the first results to be innacurate or as soon as we receive this value\n> > in metadata we can assume it is correct in your experience ?\n>\n> Good point. I think in practice an algorithm would want to see \"a few\n> frames\" to confirm the detected flicker period before reporting and\n> cancelling it, so it would make sense to set this once it's\n> \"confirmed\" and the flicker is now going to be cancelled.\n>\n> Anyway, I'll put together another version shortly.\n\nThanks\n\n>\n> Thanks!\n> David\n>\n> >\n> > Thanks\n> > j\n> >\n> > > +\n> > > + \\sa AeFlickerMode\n> > > +\n> > > - Brightness:\n> > > type: float\n> > > description: |\n> > > @@ -850,23 +912,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 B25F0BDB1C\n\tfor ;\n\tTue, 18 Jul 2023 10:38:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1AB5B628C0;\n\tTue, 18 Jul 2023 12:38:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B22EC61E2B\n\tfor ;\n\tTue, 18 Jul 2023 12:38:53 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ADAAB838;\n\tTue, 18 Jul 2023 12:37:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689676735;\n\tbh=VQGTdFI13uCjoFk+EVM9RLx9bQvNG7U4yJSfMTt4oNs=;\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=n+X13v1jEkCcPPRVKtNE3BvDkvLu7rnigCH8cU49vbsgtwwT12Zz4mpHTSbB7LkOK\n\t3Zx4AtvCyL0OcgEBXEyIwaIS4Lm6uJ3gDSsvwaBzM6k4w8/fKxv0kXlHV+l2aXwky2\n\tZOE1rZN7B/++ccYmnwuk0p37fNzKlBW0tHVg2ZIQmrUnPLSBUk2TkkduYeY3i4DYLu\n\t7m889uPt8ejyBlHPPvhqV7sSOfyE6ppuGFugPtnnnMByGnMUSE/gU75an8okB1T96G\n\tFTxJBXVoN6pq2I4gTy//TjT31kE0ySLhBk6T7EcL48ZRoJMI3e93yORqEhYgulIScm\n\tqpObYp+t6qyFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689676679;\n\tbh=VQGTdFI13uCjoFk+EVM9RLx9bQvNG7U4yJSfMTt4oNs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UiboDMj80+FJAObXcieonuW74GFeGB+cazGxD/85EoiOcMxkK93MLX3xUCW+VKdfm\n\taD6uS8WdkUcQX61dAtpVO4i2oZotIjV9lM6g39CklqfAIRLFtdXrpa/rXU6Ax9cpOi\n\tKEXLNCGpo46ghJqSeezfm5IcgviVmj8t7h0pGwqs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UiboDMj8\"; dkim-atps=neutral","Date":"Tue, 18 Jul 2023 12:38:49 +0200","To":"David Plowman ","Message-ID":"","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-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 v4 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":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27580,"web_url":"https://patchwork.libcamera.org/comment/27580/","msgid":"","date":"2023-07-18T11:12:15","subject":"Re: [libcamera-devel] [PATCH v4 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 again!\n\nOn Tue, 18 Jul 2023 at 11:38, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Tue, Jul 18, 2023 at 09:15:00AM +0100, David Plowman via libcamera-devel wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the comments.\n> >\n> > On Tue, 18 Jul 2023 at 08:04, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi David\n> > >\n> > > On Thu, Jul 13, 2023 at 04:12:17PM +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 | 79 ++++++++++++++++++++++++++--------\n> > > > 1 file changed, 62 insertions(+), 17 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index 056886e6..f1629b89 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -156,6 +156,68 @@ 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> > > I would here add that if the AeEnable control is set to false\n> > > (\"manual mode\") this setting has no effect.\n> >\n> > Yes, I think that seems reasonable!\n> >\n> > >\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> > > > + By default the system will start in FlickerAuto mode if this is\n> > > > + supported, otherwise the flicker mode will be set to FlickerOff.\n> >\n> > This is what we said in Prague, though I'm not sure about it now. I\n> > think I'd rather have it be off by default because that's the existing\n> > behaviour. If Android wants this behaviour, it should perhaps go in\n> > the Android adaptation layer. Thoughts?\n> >\n>\n> To be honest, if anything like auto is available, I would expect it to\n> be used, as I don't see why an application wouldn't want to have it\n> enabled.\n>\n> IOW it seems to me all applications will enable it anyway, if\n> available...\n\nI'm not sure I agree that \"all applications will enable it anyway\".\nCertainly it will be very common for general purpose image capture\napplications, but we also have many industrial, scientific, and home\nusers with specific purposes for their images.\n\nIn these cases I think I'd rather the new behaviour was a conscious\nchoice, rather than a default behaviour which might seem \"hidden\" to\nthem. But like I say, I can live with this, though probably by adding\ncode in our applications to undo it!\n\n>\n> > > > +\n> > > > + enum:\n> > > > + - name: FlickerOff\n> > > > + value: 0\n> > > > + description: No flicker avoidance is performed.\n> > > > + - name: FlickerCustom\n> > > > + value: 1\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: 2\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. Once flicker\n> > > > + is being corrected, it is implementation dependent whether the\n> > > > + system is still able to detect a change in the flicker period.\n> > >\n> > > \"... during a single streaming session\" ?\n> > >\n> > > I presume a camera stop/start sequence re-init the algorithms (so that\n> > > if you travel between countries far apart in the world (or different\n> > > parts of Japan) while using the camera the new flicker period will be\n> > > detected).\n> >\n> > We don't normally re-initialise control algorithms between\n> > stop/configure/start calls. This is pretty critical to everything we\n> > do - AE, AWB, ALSC and other things are all preserved.\n> >\n> > But re-opening the camera obviously does restart everything from scratch.\n> >\n> > We did briefly talk about places like Japan where it is more plausible\n> > that you could go from a 50Hz to a 60Hz environment with the camera\n> > left running. If anyone recalls exactly what we said that would be\n> > interesting. My own feelings on this include:\n> >\n> > * It's a fairly marginal use-case, even in Japan. Let's make the basics work.\n>\n> Yeah, my comment was half a joke\n>\n> > * The detection algorithm could plausibly detect changes to\n> > frequencies other than the one being handled, so this could probably\n> > be supported (though it would be implementation dependent).\n> >\n> > What do others think?\n>\n> It seems a rather marginal use case to me. Ignore my comment.\n>\n> >\n> > >\n> > > > +\n> > > > + - AeFlickerCustom:\n> > >\n> > > Nit: Custom or Manual ? I don't recall if we discussed it or not\n> >\n> > I agree, maybe \"manual\" is better. \"custom\" was the term left over\n> > from when we had explicit 50/60Hz modes.\n> >\n> > >\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> > > > + To cancel 50Hz mains flicker, this should be set to 10000 (corresponding\n> > > > + to 100Hz), or 8333 (120Hz) for 60Hz mains.\n> > > > +\n> > > > + If this control is not available, then the setting of custom flicker\n> > > > + periods is not supported.\n> > >\n> > > How can you set the control if it is not available ? This seems more\n> > > like a note for PH implementer not to register this control if\n> > > FlickerCustom is not available ?\n> >\n> > I guess it means you wouldn't be able to set the flicker period\n> > (\"manually\") yourself. I suppose you might still have \"off\" and\n> > perhaps \"auto\" settings available. Though it feels unlikely that an\n> > implementation would support \"auto\" but not \"manual\", though you never\n> > know for sure!\n>\n> What I meant is that your phrasing sounded to me like like: \"if this\n> control is not available you cannot set it\", which seems... expected ?\n> Did I get it wrong ?\n\nSorry, I can probably explain this better. The case I have in mind is\nthe case where you can set the mode to manual (because this may be\nhard to prevent if auto is available too), but you can't set the\nflicker period because this control is unavailable. In that case, I\nsuppose we could say that the absence of this control means that\nsetting the mode to manual would have no effect (or be the same as\n\"off\", or something).\n\nDavid\n\n>\n> >\n> > >\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> > > > + In the case of 50Hz mains flicker, the value would be 10000\n> > > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker.\n> > > > +\n> > > > + It is implementation dependent exactly when the system is able\n> > > > + to detect and report the flicker period.\n> > >\n> > > Does flicker period detection need a warm-up phase where we can expect\n> > > the first results to be innacurate or as soon as we receive this value\n> > > in metadata we can assume it is correct in your experience ?\n> >\n> > Good point. I think in practice an algorithm would want to see \"a few\n> > frames\" to confirm the detected flicker period before reporting and\n> > cancelling it, so it would make sense to set this once it's\n> > \"confirmed\" and the flicker is now going to be cancelled.\n> >\n> > Anyway, I'll put together another version shortly.\n>\n> Thanks\n>\n> >\n> > Thanks!\n> > David\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > +\n> > > > + \\sa AeFlickerMode\n> > > > +\n> > > > - Brightness:\n> > > > type: float\n> > > > description: |\n> > > > @@ -850,23 +912,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 1872FBDB1C\n\tfor ;\n\tTue, 18 Jul 2023 11:12:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 88C17628C0;\n\tTue, 18 Jul 2023 13:12:29 +0200 (CEST)","from mail-oi1-x236.google.com (mail-oi1-x236.google.com\n\t[IPv6:2607:f8b0:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 36A0761E2B\n\tfor ;\n\tTue, 18 Jul 2023 13:12:28 +0200 (CEST)","by mail-oi1-x236.google.com with SMTP id\n\t5614622812f47-38c35975545so4325943b6e.1\n\tfor ;\n\tTue, 18 Jul 2023 04:12:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689678749;\n\tbh=igyKddTEUSgnR4thTTFOOSeG9o425mCfwLxfFQXNY7E=;\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=jwoFa097cat6wNgthTs2OPEgCC5DFdr5fhLr8XFB3lehUsELqiNUZDovqgglW2Ef+\n\tsqJY7u5GgdJrmkWB7XprhDHjwftwbBb65laRBhsP3TO/Vwy3hTx0sKrkek1bsMT6D8\n\tdIaVX+5m7YowebAlWwasXR8EQkFeKRBZC/QLMQDbZLvCoifCJZMNaZmTQ4hU1HcYj0\n\toY7QWP9vkXCuOKaSATdjj1XnuwP9XVtQHBAp/kcsSu2ZvzpILxTB/smpOpv1nShCTp\n\tfiYJGYE72/a/ljUSoLxc9AX2N9/3QuzH/+PEHyE7Yd8CymWvltgN+AeQ8iEedZuePP\n\tRAu6MoKzhSm+g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689678747; x=1692270747;\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=rHKDFnB6ZnYBBAF+oCW9FsLuLUyhxbUYMs2r1RCDH+o=;\n\tb=HNeqi/O9bgW/he4K9+smqWg3s7gKe6FX7d2Bl3e4vYumOpSUmjiEXrNuycdtEpL8Qy\n\tKvcxmWrCsuO0QdXrlHb/4qsu9Jg1Oj9QuN6B3Y9Va2cwSRWkJI9nQG2gBYLy5VTsUh7O\n\tYzxoTszFyhpOA55YSHa4TJQqgU0WOvh4OQcMah3eFnCw0Mc7c/4u0XY7eNMmDcO5Fsks\n\tdptUxcliTydo9t5yByMd1TMdEm0YvGDm7hqCuHVen26nKp0rbKSbFapXjximP055ga3v\n\tycUb2GL3ZAhA9vdksVkjOJlrJwXt0+WlsbRdLns+puHvYtkemBTJXreZ1DbePBdl5N6s\n\tXFjw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"HNeqi/O9\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689678747; x=1692270747;\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=rHKDFnB6ZnYBBAF+oCW9FsLuLUyhxbUYMs2r1RCDH+o=;\n\tb=RCfGi2b/kJUzqn13Hb8hb6VHYZODUfpOqhoYF8VMVpFc3YXbU6OxVCs/2xfCLcq9iy\n\tFg4pR1I7cvp3MZ+1EAEfvOe8/4E4v/CvVSBAOsF+L2i/wwcmLghuqZpTABqrfFMQD/ih\n\tL832oCd1lFLbAwZQPi1EMGfASFqDzvVbiRqGnG633jD/7JnpKpkD1mB0RULfocbxAilJ\n\tnb7RP79JzBL6DQQFv2qLI5rkcM/3UEOy8Yzxada6P5Bra8iysnHG7pHXv3wo8taH2ASH\n\tAiznKpq+AeyfD0D5WwN+dDeVaFgP03fmVOU2vGJb7XFn5mKFFo738LIGqWr0gwRodoiQ\n\t4seA==","X-Gm-Message-State":"ABy/qLbjK6Co6sKyKzojc8xHM5mZSAyRzwtcqJwbZMU5CWDkVl5z/jWx\n\t12vIAxpK6d5ntKgmMOeymkT6n4gjw8+w2XBkqXkQw8lgODPTsMIk","X-Google-Smtp-Source":"APBJJlHmdNfnHl+2x9lw/AoBZ/spIhkzsUxJAUx+lCC/5J/f7xZ7LHMaSKUH4vDyvTTuTUwy10QwEEkk9B6lFS+ANaA=","X-Received":"by 2002:a05:6808:1308:b0:3a1:df16:2eed with SMTP id\n\ty8-20020a056808130800b003a1df162eedmr18068732oiv.30.1689678746789;\n\tTue, 18 Jul 2023 04:12:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Tue, 18 Jul 2023 12:12:15 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 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":27585,"web_url":"https://patchwork.libcamera.org/comment/27585/","msgid":"","date":"2023-07-18T12:20:14","subject":"Re: [libcamera-devel] [PATCH v4 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, Jul 18, 2023 at 12:12:15PM +0100, David Plowman wrote:\n> Hi again!\n>\n> On Tue, 18 Jul 2023 at 11:38, Jacopo Mondi\n> wrote:\n> >\n> > Hi David\n> >\n> > On Tue, Jul 18, 2023 at 09:15:00AM +0100, David Plowman via libcamera-devel wrote:\n> > > Hi Jacopo\n> > >\n> > > Thanks for the comments.\n> > >\n> > > On Tue, 18 Jul 2023 at 08:04, Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi David\n> > > >\n> > > > On Thu, Jul 13, 2023 at 04:12:17PM +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 | 79 ++++++++++++++++++++++++++--------\n> > > > > 1 file changed, 62 insertions(+), 17 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > index 056886e6..f1629b89 100644\n> > > > > --- a/src/libcamera/control_ids.yaml\n> > > > > +++ b/src/libcamera/control_ids.yaml\n> > > > > @@ -156,6 +156,68 @@ 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> > > > I would here add that if the AeEnable control is set to false\n> > > > (\"manual mode\") this setting has no effect.\n> > >\n> > > Yes, I think that seems reasonable!\n> > >\n> > > >\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> > > > > + By default the system will start in FlickerAuto mode if this is\n> > > > > + supported, otherwise the flicker mode will be set to FlickerOff.\n> > >\n> > > This is what we said in Prague, though I'm not sure about it now. I\n> > > think I'd rather have it be off by default because that's the existing\n> > > behaviour. If Android wants this behaviour, it should perhaps go in\n> > > the Android adaptation layer. Thoughts?\n> > >\n> >\n> > To be honest, if anything like auto is available, I would expect it to\n> > be used, as I don't see why an application wouldn't want to have it\n> > enabled.\n> >\n> > IOW it seems to me all applications will enable it anyway, if\n> > available...\n>\n> I'm not sure I agree that \"all applications will enable it anyway\".\n> Certainly it will be very common for general purpose image capture\n> applications, but we also have many industrial, scientific, and home\n> users with specific purposes for their images.\n>\n> In these cases I think I'd rather the new behaviour was a conscious\n> choice, rather than a default behaviour which might seem \"hidden\" to\n> them. But like I say, I can live with this, though probably by adding\n> code in our applications to undo it!\n>\n\nYou certainly have more experience dealing with users than me, so I\nguess you can frame the use case better. I'm fine with both options\n\n> >\n> > > > > +\n> > > > > + enum:\n> > > > > + - name: FlickerOff\n> > > > > + value: 0\n> > > > > + description: No flicker avoidance is performed.\n> > > > > + - name: FlickerCustom\n> > > > > + value: 1\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: 2\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. Once flicker\n> > > > > + is being corrected, it is implementation dependent whether the\n> > > > > + system is still able to detect a change in the flicker period.\n> > > >\n> > > > \"... during a single streaming session\" ?\n> > > >\n> > > > I presume a camera stop/start sequence re-init the algorithms (so that\n> > > > if you travel between countries far apart in the world (or different\n> > > > parts of Japan) while using the camera the new flicker period will be\n> > > > detected).\n> > >\n> > > We don't normally re-initialise control algorithms between\n> > > stop/configure/start calls. This is pretty critical to everything we\n> > > do - AE, AWB, ALSC and other things are all preserved.\n> > >\n> > > But re-opening the camera obviously does restart everything from scratch.\n> > >\n> > > We did briefly talk about places like Japan where it is more plausible\n> > > that you could go from a 50Hz to a 60Hz environment with the camera\n> > > left running. If anyone recalls exactly what we said that would be\n> > > interesting. My own feelings on this include:\n> > >\n> > > * It's a fairly marginal use-case, even in Japan. Let's make the basics work.\n> >\n> > Yeah, my comment was half a joke\n> >\n> > > * The detection algorithm could plausibly detect changes to\n> > > frequencies other than the one being handled, so this could probably\n> > > be supported (though it would be implementation dependent).\n> > >\n> > > What do others think?\n> >\n> > It seems a rather marginal use case to me. Ignore my comment.\n> >\n> > >\n> > > >\n> > > > > +\n> > > > > + - AeFlickerCustom:\n> > > >\n> > > > Nit: Custom or Manual ? I don't recall if we discussed it or not\n> > >\n> > > I agree, maybe \"manual\" is better. \"custom\" was the term left over\n> > > from when we had explicit 50/60Hz modes.\n> > >\n> > > >\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> > > > > + To cancel 50Hz mains flicker, this should be set to 10000 (corresponding\n> > > > > + to 100Hz), or 8333 (120Hz) for 60Hz mains.\n> > > > > +\n> > > > > + If this control is not available, then the setting of custom flicker\n> > > > > + periods is not supported.\n> > > >\n> > > > How can you set the control if it is not available ? This seems more\n> > > > like a note for PH implementer not to register this control if\n> > > > FlickerCustom is not available ?\n> > >\n> > > I guess it means you wouldn't be able to set the flicker period\n> > > (\"manually\") yourself. I suppose you might still have \"off\" and\n> > > perhaps \"auto\" settings available. Though it feels unlikely that an\n> > > implementation would support \"auto\" but not \"manual\", though you never\n> > > know for sure!\n> >\n> > What I meant is that your phrasing sounded to me like like: \"if this\n> > control is not available you cannot set it\", which seems... expected ?\n> > Did I get it wrong ?\n>\n> Sorry, I can probably explain this better. The case I have in mind is\n> the case where you can set the mode to manual (because this may be\n> hard to prevent if auto is available too), but you can't set the\n> flicker period because this control is unavailable. In that case, I\n> suppose we could say that the absence of this control means that\n> setting the mode to manual would have no effect (or be the same as\n> \"off\", or something).\n\nI wish lc-compliance would catch situation like these and scold the\npipeline handler developers, as this is clearly an error on their\nside, isn't it ?\n\n>\n> David\n>\n> >\n> > >\n> > > >\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> > > > > + In the case of 50Hz mains flicker, the value would be 10000\n> > > > > + (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains flicker.\n> > > > > +\n> > > > > + It is implementation dependent exactly when the system is able\n> > > > > + to detect and report the flicker period.\n> > > >\n> > > > Does flicker period detection need a warm-up phase where we can expect\n> > > > the first results to be innacurate or as soon as we receive this value\n> > > > in metadata we can assume it is correct in your experience ?\n> > >\n> > > Good point. I think in practice an algorithm would want to see \"a few\n> > > frames\" to confirm the detected flicker period before reporting and\n> > > cancelling it, so it would make sense to set this once it's\n> > > \"confirmed\" and the flicker is now going to be cancelled.\n> > >\n> > > Anyway, I'll put together another version shortly.\n> >\n> > Thanks\n> >\n> > >\n> > > Thanks!\n> > > David\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > +\n> > > > > + \\sa AeFlickerMode\n> > > > > +\n> > > > > - Brightness:\n> > > > > type: float\n> > > > > description: |\n> > > > > @@ -850,23 +912,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 35211BDB1C\n\tfor ;\n\tTue, 18 Jul 2023 12:20:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E642628C0;\n\tTue, 18 Jul 2023 14:20:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C5CB61E2B\n\tfor ;\n\tTue, 18 Jul 2023 14:20:19 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 26E85838;\n\tTue, 18 Jul 2023 14:19:25 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689682820;\n\tbh=9bAqiNIHTLXiI8y8Qf6ScDDkeeJ4XLzoIoUkpmJkyD4=;\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=E1LIRNvtHiwpOAg5gU6n4zl8Ntqa0qRPdnkVUTTewWrFfvSZIAakistroBlw0nm1V\n\tESoeLlzGml51ylJKUzBKDEiQ+TCchqcw/8hau4uUpZTVMPUcVlI/Ga5oMSzJkr4pf5\n\tK8Nlb1iuufwf32BhNVb2mdbKBS1Ai5yl1G8NleThGBLm6qr0/j9T8fdrclLTVRaDRS\n\tE31Em+J7+ZrAiGwrwvzdwwB2613jTz1cc+gxMlsxG6azZtx6+ZUgCmi66jsZQdDuHK\n\t2YuIrmcaqbB6RHBFXbk7kbfH5vuT/aOKJlAlky6P1Wc8PvLcrnVi2e5gzdCu6VbtN4\n\tmuWc+/ePSSxqQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689682765;\n\tbh=9bAqiNIHTLXiI8y8Qf6ScDDkeeJ4XLzoIoUkpmJkyD4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XkT1+r6X9k3tTxUPwpHjE0g0Lmu3bjrD6z3lEVhUNnCaPEJqTDAg+cVZ6f9Rx6ANP\n\tL/gBDUOqD7d63DgM4IRgAN8rDFSUJ2IvAbX2b+MWGoZWumkfgudrXCbMGSyH5i4j8x\n\tXLdlbiHz7+HHOLzWMFf4NjjfBMBKlVB4lwhlIeD0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XkT1+r6X\"; dkim-atps=neutral","Date":"Tue, 18 Jul 2023 14:20:14 +0200","To":"David Plowman ","Message-ID":"","References":"<20230713151218.26045-1-david.plowman@raspberrypi.com>\n\t<20230713151218.26045-2-david.plowman@raspberrypi.com>\n\t\n\t\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 v4 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":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]