[libcamera-devel] [PATCH v2 3/6] libcamera: controls: Add AE related controls

[Kieran Bingham]

Hi Naush,

On 09/03/2020 12:33, 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 double type control used to set the log2 exposure
> adjustment for the AE algorithm.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----
>  1 file changed, 109 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 5bbe65ae..da1a7b43 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -23,24 +23,104 @@ controls:
>  
>          \sa AeEnable
>  
> -  - AwbEnable:
> -      type: bool
> +  - AeMeteringMode:
> +      type: int32_t
>        description: |
> -        Enable or disable the AWB.
> -
> -        \sa ManualGain
> +        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

Only caught because of the spelling error below, but in the other
instances you do not hyphenate platform-specific. Maybe the simple
option is to not hyphenate here either :-)

> +        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.
> +        - name: MeteringCustom1
> +          value: 3

This feels a bit painful if we want to add more (non-custom, or custom)
modes.

We would only be able to append to prevent ABI breakage - Then the enums
could seem a bit ugly:

 enum {
	MeteringCentreWeighted,
	MeteringSpot,
	MeteringMatrix,
	MeteringCustom1,
	MeteringCustom2,
	MeteringCustom3,
	MeteringMadeUpGeneralControl,
	MeteringCustom4,
	MeteringMadeupButGeneralControl2,
 }

But I fear that may not be something we can easily avoid unless we know
*all* of the metering mode names in advance...

Of course we're intentionally not yet ABI stable anyway, so this could
still be an intermediate step...


One option could be to have MeteringCustom, with the actual 'custom'
choice provided by another means ('yet another' control?), but maybe
that doesn't scale well either. I'm not sure yet...

> +          description: Custom metering mode 1.
> +        - name: MeteringCustom2
> +          value: 4
> +          description: Custom metering mode 2.
> +        - name: MeteringCustom3
> +          value: 5
> +          description: Custom metering mode 3.

Are you able to express what you currently envisage the 3 custom modes
would be used for?

> +        - name: MeteringModeMax
> +          value: 5
It's a shame we have to express the enum max value manually here :-(
I haven't looked at if it could be identified through code though.

> +          description: Maximum allowed value (place any new values above here).
>  
> -  - Brightness:
> +  - AeConstraintMode:
>        type: int32_t
> -      description: Specify a fixed brightness parameter
> +      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 constaint modes may be supported.

/constaint/constraint/

> +      enum:
> +        - name: ConstraintNormal
> +          value: 0
> +          description: Default constraint mode.
> +        - name: ConstraintHighlight
> +          value: 1
> +          description: Highlight constraint mode.
> +        - name: ConstraintCustom1
> +          value: 2
> +          description: Custom constraint mode 1.
> +        - name: ConstraintCustom2
> +          value: 3
> +          description: Custom constraint mode 2.
> +        - name: ConstraintCustom3
> +          value: 4
> +          description: Custom constraint mode 3.
> +        - name: ConstraintModeMax
> +          value: 4
> +          description: Maximum allowed value (place any new values above here).

Same comments apply here of course.

Perhaps we should expand the initial set from the modes/constraints that
we will need for supporting the Android HAL...


>  
> -  - Contrast:
> +  - AeExposureMode:
>        type: int32_t
> -      description: Specify a fixed contrast parameter
> +      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
> +        spcific, and not all exposure modes may be supported.

s/spcific/specific/

> +      enum:
> +        - name: ExposureNormal
> +          value: 0
> +          description: Default metering mode.
> +        - name: ExposureSport
> +          value: 1
> +          description: Sport metering mode (favouring short shutter times).
> +        - name: ExposureLong
> +          value: 2
> +          description: Exposure mode allowing long exposure times.
> +        - name: ExposureCustom1
> +          value: 3
> +          description: Custom exposure mode 1.
> +        - name: ExposureCustom2
> +          value: 4
> +          description: Custom exposure mode 2.
> +        - name: ExposureCustom3
> +          value: 5
> +          description: Custom exposure mode 3.
> +        - name: ExposureModeMax
> +          value: 5
> +          description: Maximum allowed value (place any new values above here).
>  
> -  - Saturation:
> -      type: int32_t
> -      description: Specify a fixed saturation parameter
> +  - ExposureValue:
> +      type: float
> +      description: |
> +        Specify an Exposure Value (EV) parameter. The EV parameter will only be
> +        applied if the AE alogrithm 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
>  
>    - ManualExposure:
>        type: int32_t
> @@ -57,4 +137,21 @@ controls:
>          applied to all colour channels.
>  
>          \sa ManualExposure
> +

What changes were made to the properties below ? (I don't see anything
in particular) or is that just git being a pain and adding churn?



> +  - AwbEnable:
> +      type: bool
> +      description: |
> +        Enable or disable the AWB.
> +
> +  - Brightness:
> +      type: int32_t
> +      description: Specify a fixed brightness parameter
> +
> +  - Contrast:
> +      type: int32_t
> +      description: Specify a fixed contrast parameter
> +
> +  - Saturation:
> +      type: int32_t
> +      description: Specify a fixed saturation parameter
>  ...
> 

-- 
Regards
--
Kieran