[libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE related controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Apr 24 12:13:46 CEST 2020
Hi Naush,
On Fri, Apr 24, 2020 at 10:26:37AM +0100, Naushir Patuck wrote:
> On Thu, 23 Apr 2020 at 20:43, Laurent Pinchart wrote:
> > 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?
Sure, or feel free to squash it in this patch (that was my initial
plan).
> > 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?
Works for me.
> > > + - 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.
Sure.
> > 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
More information about the libcamera-devel
mailing list