[libcamera-devel] [PATCH v2 3/6] libcamera: controls: Add AE related controls
Naushir Patuck
naush at raspberrypi.com
Fri Mar 27 12:29:03 CET 2020
HI Laurent,
On Fri, 27 Mar 2020 at 11:03, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Fri, Mar 27, 2020 at 10:56:14AM +0000, Naushir Patuck wrote:
> > On Thu, 26 Mar 2020 at 16:26, Laurent Pinchart wrote:
> > > 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.
> >
> > This is possible, but it might get tricky trying to interoperate with
> > different vendor IPAs where the HW/SW capabilities may not be the
> > same.
>
> I agree, that's why I was wondering what would be a good way to express
> metering (which will apply to AF too, and potentially AWB ?) in a
> flexible way that could give finer-grained control when the hardware and
> IPA support it, but wouldn't be awkward to use for devices that have
> more limited capabilities. We don't have to fix all of this now, it
> could come as a later rework of the controls, but I wanted to raise the
> issue already in case we could figure out a solution in the short term.
>
> > >>>>> + - 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.
> >
> > As mentioned previously, the max value is only used for ControlInfo
> > (previously known as ControlRange). I do wonder if ControlInfo is a
> > sensible thing to have for an enum though. Suppose an IPA allows only
> > metering modes 0, 1, 2, 4, 5. In this case, the ControlInfo must be
> > specified as (min, max) == (0, 5), but a value of 3 is invalid.
>
> I agree with you. We need ControlInfo to report the valid values for
> enumerated controls. Another item on our todo list :-)
>
> > >>>>> + 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
> > > ?
> >
> > Will do.
> >
> > >>>>> + - 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).
> >
> > I always looked as scene mode as a higher level construct within the
> > camera application. The application bunches a group of settings for a
> > particular scene mode (e.g. for Sport scene mode, AE turned to sport,
> > focus turned to fast converge, flash turned to fast sync) and sets
> > them individually. This way it could tailor them exactly to what it
> > needs.
>
> That makes sense to me, but should the lower-level settings then use
> scene mode names ? We could for instance name this ExposureShort (very
> easy to mistype ExposureSport :-)) for instance (or any other name that
> would convey the underlying behaviour). I trust your expertise on this
> topic.
Yes, that seems reasonable.
>
> > >>>>> + - 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
Regards,
Naush
More information about the libcamera-devel
mailing list