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

[Naushir Patuck]

Hi Laurent,

On Thu, 23 Apr 2020 at 20:43, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
>

Yes, that's a good idea.  Would it make sense if I make this change to
control_ids.cpp.in as a separate commit and make it the first commit
in the patchset?

> 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 ?

Yes, if we have (somewhat) arbitrary freedom to assign weights and
regions this should be possible.

>
> > +        - 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 ?

Yes, that makes sense, I'll add a new mode.  We don't currently have
tuning that will handle it, but that can come later.
One suggestion, can I call it ConstraintShadows instead of LowLight as
it's a constraint on the amount of under-exposed area?

>
> > +        - 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 ? :-)

Oops, will fix.

>
> > +        - 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.

Done.  I think we should keep the control name as Lux which is a
standard imaging term.

> 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,
Naush

> --
> Regards,
>
> Laurent Pinchart