[{"id":33037,"web_url":"https://patchwork.libcamera.org/comment/33037/","msgid":"","date":"2025-01-13T10:49:33","subject":"Re: [PATCH v7 09/12] controls: Redefine AeEnable","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jan 10, 2025 at 05:57:34PM -0600, Paul Elder wrote:\n> In the redesign of the AE-related controls, the AeEnable control was\n> intended to be removed in favor of more specific sub-controls for\n> analogue gain and exposure time. However this will cause problems if\n> aperture sub-controls are introduced, and an application from a\n> pre-aperture era uses a camera that supports aperture.\n> \n> If there is no AeEnable control, then a pre-aperture era application\n> might set analogue gain and exposure time to manual, while aperture\n> silently stays auto since that's the default mode. Thus aperture would\n> be uncontrollable by the application.\n> \n> With an AeEnable control, then a pre-aperture era application can set\n> AeEnable to manual, and under the hood all three of analogue gain and\n> exposure time and aperture will be set to manual. The application won't\n> be able to set the manual aperture, however.\n> \n> Although the above scenario is expected to be rare, the scenario with an\n> AeEnable control seems less detrimental. With an AeEnable control at\n> least the aperture would be static at a reasonably usable value, whereas\n> without an AeEnable the aperture would be more-or-less uncontrolable and\n> could go to extreme values as the AEGC algorithm tries to compensate for\n> the manual analogue gain and exposure time values.\n> \n> Thus we redefine the AeEnable control, available only as a control and\n> not in metadata. It will be preprocessed by the Camera class so that the\n> relevant sub-controls are set. No pipline handler nor IPA shall act on\n> the AeEnable control. The IPA still has to report the control as\n> available, however.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Laurent Pinchart \n\nExcited to see, how that feels in the implementation...\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> \n> ---\n> Changes in v7:\n> - fix wording\n> \n> New in v6\n> ---\n> Documentation/design/ae.rst | 23 +++++++++++++++++------\n> src/libcamera/control_ids_core.yaml | 12 +++++++++---\n> 2 files changed, 26 insertions(+), 9 deletions(-)\n> \n> diff --git a/Documentation/design/ae.rst b/Documentation/design/ae.rst\n> index 9d5bb5078..df9b1fa76 100644\n> --- a/Documentation/design/ae.rst\n> +++ b/Documentation/design/ae.rst\n> @@ -185,6 +185,10 @@ A diagram of our solution:\n> 0: Auto\n> 1: Manual\n> \n> + AeEnable\n> + - True -> ExposureTimeMode:Auto + AnalogueGainMode:Auto\n> + - False -> ExposureTimeMode:Manual + AnalogueGainMode:Manual\n> +\n> \n> The diagram is divided in four sections horizontally:\n> \n> @@ -225,10 +229,14 @@ This simulates an auto -> locked -> manual or auto -> manual state transition,\n> and makes it impossible to do the nonsensical manual -> locked state\n> transition.\n> \n> -We specifically do not have a \"master AE control\" like the old AeEnable. This\n> -is because we have the individual mode controls, and if we had a master AE\n> -control it would be a \"control that sets other controls\", which could easily\n> -get out of control.\n> +AeEnable still exists to allow applications to set the mode of all the\n> +sub-controls at once. Besides being for convenience, this will also be useful\n> +when we eventually implement an aperture control. This is because applications\n> +that will be made before aperture will have been available would still be able\n> +to set aperture mode to auto or manual, as opposed to having the aperture stuck\n> +at auto while the application really wanted manual. Although the aperture would\n> +still be stuck at an uncontrollable value, at least it would be at a static\n> +usable value as opposed to varying via the AEGC algorithm.\n> \n> With this solution, the earlier example would become:\n> \n> @@ -277,9 +285,12 @@ and gain:\n> \n> - AeState\n> \n> +- AeEnable\n> +\n> Auto-exposure and auto-gain can be enabled and disabled separately using the\n> -ExposureTimeMode and AnalogueGainMode controls respectively. There is no\n> -overarching AeEnable control.\n> +ExposureTimeMode and AnalogueGainMode controls respectively. The AeEnable\n> +control can also be used, as it sets both of the modes simultaneously. The\n> +AeEnable control is not returned in metadata.\n> \n> When the respective mode is set to auto, the respective value that is computed\n> by the AEGC algorithm is applied to the image sensor. Any value that is\n> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> index d88c1852e..aa7448645 100644\n> --- a/src/libcamera/control_ids_core.yaml\n> +++ b/src/libcamera/control_ids_core.yaml\n> @@ -10,11 +10,17 @@ vendor: libcamera\n> controls:\n> - AeEnable:\n> type: bool\n> - direction: inout\n> + direction: in\n> description: |\n> - Enable or disable the AE.\n> + Enable or disable the AEGC algorithm. When this control is set to true,\n> + both ExposureTimeMode and AnalogueGainMode are set to auto, and if this\n> + control is set to false then both are set to manual.\n> +\n> + If ExposureTimeMode or AnalogueGainMode are also set in the same\n> + request as AeEnable, then the modes supplied by ExposureTimeMode or\n> + AnalogueGainMode will take precedence.\n> \n> - \\sa ExposureTime AnalogueGain\n> + \\sa ExposureTimeMode AnalogueGainMode\n> \n> - AeState:\n> type: int32_t\n> -- \n> 2.39.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 7C9B3C326C\n\tfor ;\n\tMon, 13 Jan 2025 10:49:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7DB568516;\n\tMon, 13 Jan 2025 11:49:38 +0100 (CET)","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 4ABCF684E1\n\tfor ;\n\tMon, 13 Jan 2025 11:49:37 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3c15:8eea:8742:2e5a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B7E497E9;\n\tMon, 13 Jan 2025 11:48:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"R4pmfrjc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736765320;\n\tbh=U9TrpuS97+JXkE8Sv1Up8CtuQBuOU3raWmNH/SWvlWI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R4pmfrjcrT5VGzO/fOyiWD/OPnmaUIG0E6p2GgDTh/UNwVLumIEUmG+El7HVKyW/4\n\tZXeXlIuIanoz0FsndxaKws1gIFf3/fOK8D1iaXkIjwvhHU3mu7QqsBWVmN1CqLSJDs\n\tRCY6x7nXNJ04W4jSQajucSQx4gywrXL9hSnicO88=","Date":"Mon, 13 Jan 2025 11:49:33 +0100","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH v7 09/12] controls: Redefine AeEnable","Message-ID":"","References":"<20250110235737.1524733-1-paul.elder@ideasonboard.com>\n\t<20250110235737.1524733-10-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250110235737.1524733-10-paul.elder@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]