[PATCH v6 09/12] controls: Redefine AeEnable

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jan 08, 2025 at 06:09:39PM -0600, Paul Elder wrote:
> In the redesign of the AE-related controls, the AeEnable control was
> intended to be removed in favor of more specific sub-controls for
> analogue gain and exposure time. However this will cause problems if an
> aperture sub-controls is introduced, and an application from a

s/controls/control/ (or s/if an/if/ on the previous line)

> pre-aperture era uses a camera that supports aperture.
> 
> If there is no AeEnable control, then a pre-aperture era application
> might set analogue gain and exposure time to manual, while aperture
> silently stays auto since that's the default mode. Thus aperture would
> be uncontrollable by the application.
> 
> With an AeEnable control, then a pre-aperture era application can set
> AeEnable to manual, and under the hood all three of analogue gain and
> exposure time and aperture will be set to manual. The application won't
> be able to set the manual aperture, however.
> 
> Although the above scenario is expected to be rare, as applications are
> expected to recompile on a libcamera version change, the scenario with

It's not a matter of recompiling applications. Even if recompiled, an
application that doesn't know about aperture will not set the related
controls. I think you can just drop the second clause above (from "as
applications" to "version chage, ").

> an AeEnable control seems less detrimental. With an AeEnable control at
> least the aperture would be static at a reasonably usable value, whereas
> without an AeEnable the aperture would be more-or-less uncontrolable and
> could go to extreme values as the AEGC algorithm tries to compensate for
> the manual analogue gain and exposure time values.
> 
> Thus we redefine the AeEnable control, available only as a control and
> not in metadata. It will be preprocessed by the Camera class so that the
> relevant sub-controls are set. No pipline handler nor IPA shall act on
> the AeEnable control. The IPA still has to report the control as
> available, however.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v6
> ---
>  Documentation/design/ae.rst         | 23 +++++++++++++++++------
>  src/libcamera/control_ids_core.yaml | 12 +++++++++---
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/design/ae.rst b/Documentation/design/ae.rst
> index f8b2c887c..bbb6bb1d1 100644
> --- a/Documentation/design/ae.rst
> +++ b/Documentation/design/ae.rst
> @@ -185,6 +185,10 @@ A diagram of our solution:
>      0: Auto
>      1: Manual
>  
> +  AeEnable
> +    - True -> ExposureTimeMode:Auto + AnalogueGainMode:Auto
> +    - False -> ExposureTimeMode:Manual + AnalogueGainMode:Manual
> +
>  
>  The diagram is divided in four sections horizontally:
>  
> @@ -225,10 +229,14 @@ This simulates an auto -> locked -> manual or auto -> manual state transition,
>  and makes it impossible to do the nonsensical manual -> locked state
>  transition.
>  
> -We specifically do not have a "master AE control" like the old AeEnable. This
> -is because we have the individual mode controls, and if we had a master AE
> -control it would be a "control that sets other controls", which could easily
> -get out of control.
> +AeEnable still exists to allow applications to set the mode of all the
> +sub-controls at once. Besides being for convenience, this will also be useful
> +when we eventually implement an aperture control. This is because applications
> +there were made before aperture was available would still be able to set

that are written before aperture is available will still be able to set

> +aperture mode to auto or manual, as opposed to having the aperture stuck at
> +auto while the application really wanted manual. Although the aperture would
> +still be stuck at an uncontrollable value, at least it would be at a static
> +usable value as opposed to varying via the AEGC algorithm.
>  
>  With this solution, the earlier example would become:
>  
> @@ -277,9 +285,12 @@ and gain:
>  
>  - AeState
>  
> +- AeEnable
> +
>  Auto-exposure and auto-gain can be enabled and disabled separately using the
> -ExposureTimeMode and AnalogueGainMode controls respectively. There is no
> -overarching AeEnable control.
> +ExposureTimeMode and AnalogueGainMode controls respectively. The AeEnable
> +control can also be used, as it sets both of the modes simultaneously. The

s/as it/and/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +AeEnable control is not returned in metadata.
>  
>  When the respective mode is set to auto, the respective value that is computed
>  by the AEGC algorithm is applied to the image sensor. Any value that is
> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
> index d88c1852e..aa7448645 100644
> --- a/src/libcamera/control_ids_core.yaml
> +++ b/src/libcamera/control_ids_core.yaml
> @@ -10,11 +10,17 @@ vendor: libcamera
>  controls:
>    - AeEnable:
>        type: bool
> -      direction: inout
> +      direction: in
>        description: |
> -        Enable or disable the AE.
> +        Enable or disable the AEGC algorithm. When this control is set to true,
> +        both ExposureTimeMode and AnalogueGainMode are set to auto, and if this
> +        control is set to false then both are set to manual.
> +
> +        If ExposureTimeMode or AnalogueGainMode are also set in the same
> +        request as AeEnable, then the modes supplied by ExposureTimeMode or
> +        AnalogueGainMode will take precedence.
>  
> -        \sa ExposureTime AnalogueGain
> +        \sa ExposureTimeMode AnalogueGainMode
>  
>    - AeState:
>        type: int32_t

-- 
Regards,

Laurent Pinchart