[libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE related controls

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Apr 03, 2020 at 03:53:04PM +0100, Naushir Patuck wrote:
> AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type
> controls used to specify operating modes in the AE algorithm. All modes
> may not be supported by all platforms.
> 
> ExposureValue is a new control used to set the log2 exposure adjustment
> for the AE algorithm.
> 
> Lux is a new control that returns the current lux estimate.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 104 +++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 64e81520..50f9e07b 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -4,6 +4,8 @@
>  #
>  %YAML 1.2
>  ---
> +# Unless otherwise stated, all controls are bi-directional, i.e. they can be
> +# set through a Request and returned out through Metadata.

How about adding this to control_ids.cpp.in, to ensure it will end up in
the generated documentation ?

diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
index 99c511d0e712..cdd5d351ad6a 100644
--- a/src/libcamera/control_ids.cpp.in
+++ b/src/libcamera/control_ids.cpp.in
@@ -33,6 +33,10 @@ ${controls_def}

 /**
  * \brief List of all supported libcamera controls
+ *
+ * Unless otherwise stated, all controls are bi-directional, i.e. they can be
+ * set through Request::controls() and returned out through
+ * Request::metadata().
  */
 extern const ControlIdMap controls {
 ${controls_map}

>  controls:
>    - AeEnable:
>        type: bool
> @@ -23,6 +25,103 @@ controls:
>  
>          \sa AeEnable
>  

The controls below look good to me. I'll add a few comments regarding
possible future developments, they're not meant to be addressed today.

> +  # AeMeteringMode needs further attention:
> +  # - Auto-generate max enum value.
> +  # - Better handling of custom types.
> +  - AeMeteringMode:
> +      type: int32_t
> +      description: |
> +        Specify a metering mode for the AE algorithm to use. The metering
> +        modes determine which parts of the image are used to determine the
> +        scene brightness. Metering modes may be platform specific and not
> +        all metering modes may be supported.
> +      enum:
> +        - name: MeteringCentreWeighted
> +          value: 0
> +          description: Centre-weighted metering mode.
> +        - name: MeteringSpot
> +          value: 1
> +          description: Spot metering mode.
> +        - name: MeteringMatrix
> +          value: 2
> +          description: Matrix metering mode.

I think we'll need a way to configure both the spot and matrix modes
with a point and a matrix of weights, respectively. At that point I
think we could possibly merge the two together, as spot mode is
essentially matrix mode with a single region, right ?

> +        - name: MeteringCustom
> +          value: 3
> +          description: Custom metering mode.
> +        - name: MeteringModeMax
> +          value: 3
> +          description: Maximum allowed value (place any new values above here).
> +
> +  # AeConstraintMode needs further attention:
> +  # - Auto-generate max enum value.
> +  # - Better handling of custom types.
> +  - AeConstraintMode:
> +      type: int32_t
> +      description: |
> +        Specify a constraint mode for the AE algorithm to use. These determine
> +        how the measured scene brightness is adjusted to reach the desired
> +        target exposure. Constraint modes may be platform specific, and not
> +        all constraint modes may be supported.
> +      enum:
> +        - name: ConstraintNormal
> +          value: 0
> +          description: Default constraint mode.
> +            This mode aims to balance the exposure of different parts of the
> +            image so as to reach a reasonable average level. However, highlights
> +            in the image may appear over-exposed and lowlights may appear
> +            under-exposed.
> +        - name: ConstraintHighlight
> +          value: 1
> +          description: Highlight constraint mode.
> +            This mode adjusts the exposure levels in order to try and avoid
> +            over-exposing the brightest parts (highlights) of an image.
> +            Other non-highlight parts of the image may appear under-exposed.

Do we need ConstraintLowlight too ?

> +        - name: ConstraintCustom
> +          value: 2
> +          description: Custom constraint mode.
> +        - name: ConstraintModeMax
> +          value: 2
> +          description: Maximum allowed value (place any new values above here).
> +
> +  # AeExposureMode needs further attention:
> +  # - Auto-generate max enum value.
> +  # - Better handling of custom types.
> +  - AeExposureMode:
> +      type: int32_t
> +      description: |
> +        Specify an exposure mode for the AE algorithm to use. These specify
> +        how the desired total exposure is divided between the shutter time
> +        and the sensor's analogue gain. The exposure modes are platform
> +        specific, and not all exposure modes may be supported.
> +      enum:
> +        - name: ExposureNormal
> +          value: 0
> +          description: Default metering mode.

Is this a metering mode, or is that a copy&paste ? :-)

> +        - name: ExposureShort
> +          value: 1
> +          description: Exposure mode allowing only short exposure times.
> +        - name: ExposureLong
> +          value: 2
> +          description: Exposure mode allowing long exposure times.
> +        - name: ExposureCustom
> +          value: 3
> +          description: Custom exposure mode.
> +        - name: ExposureModeMax
> +          value: 3
> +          description: Maximum allowed value (place any new values above here).
> +
> +  - ExposureValue:
> +      type: float
> +      description: |
> +        Specify an Exposure Value (EV) parameter. The EV parameter will only be
> +        applied if the AE algorithm is currently enabled.
> +
> +        By convention EV adjusts the exposure as log2. For example
> +        EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment
> +        of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].
> +
> +        \sa AeEnable
> +
>    - ExposureTime:
>        type: int32_t
>        description: |
> @@ -53,6 +152,11 @@ controls:
>          Specify a fixed contrast parameter. Normal contrast is given by the
>          value 1.0; larger values produce images with more contrast.
>  
> +  # Lux can only be returned in Metadata
> +  - Lux:
> +      type: float
> +      description: Report an estimate of the current lux level.
> +

And here, to replace the comment,

   - Lux:
       type: float
       description: |
         Report an estimate of the current lux level. The Lux control can only
         be returned in metadata.

and maybe nitpicking, should it be "estimate of the current illuminance
in lux" ? The control could also be called Illuminance. I'll leave this
up to you.

With these small issues addressed (except for the comments related to
AeMeteringMode and AeConstraintMode that are really related to future
work),

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

>    - AwbEnable:
>        type: bool
>        description: |

-- 
Regards,

Laurent Pinchart