[libcamera-devel] [PATCH v2 3/6] libcamera: controls: Add AE related controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 26 17:26:00 CET 2020
Hi Naush,
Thank you for the patch.
On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote:
> On 23/03/2020 15:29, Naushir Patuck wrote:
> > On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote:
> >> 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?
> >
> > The intention here was to allow users to add new modes to the
> > algorithm, perhaps something very specific to their application. If
> > the tuning parameters for the mode could be added/tweaked via a
> > text/config file, then the user could directly use the new mode
> > without ever having to download/re-compile any code. Initially (as in
> > patch v1), these modes were defined as std::string types, but we
> > concluded that added too much vagueness to the list of options.
> > Perhaps 3 custom modes is too much, and we can make do with just one?
>
> Maybe one is enough I don't know. That's why I wondered about one custom
> control which has a secondary option to give more flexibility.
>
> Otherwise '3' seems like a very arbitrary definition ...
This is indeed an issue, I'll try to sleep over it. Having a single
value wouldn't be too bad when it comes to extending the enum, but that
may not be enough.
That being said, for this specific control, I wonder if we should make
it a bit more finer-grained by specifying a list of weighted regions.
Just brainstorming, the ControlInfo (previously known as ControlRange)
class could then possibly offer a set of defaults based on an enum.
> >>> + - 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.
> >
> > This is primarily needed for setting up ControlRange for the control.
> > It should be entirely possible for the gen_controls script to add the
> > Max item to the end of the enum?
>
> Aha, then indeed perhaps we should automate the creation somehow
Yes, we should do that.
> >>> + 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.
Would it be possible to give a more detailed description of those modes
?
> >>> + - 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).
Reading the description, I wonder if this should be turned into a scene
mode control, as it could also have an effect on other algorithms
(auto-focus comes to mind for instance).
> >>> + - 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?
> >
> > I only moved them around - so all the controls in the file are ordered
> > in appropriate groups (AGC, AWB, General...).
>
> Aha - in that case, it would generally be better to commit any
> non-functional-change code move as a separate patch, so that it's clear
> and distinct on it's own.
>
> >>> + - 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list