[libcamera-devel] [PATCH v2 3/6] libcamera: controls: Add AE related controls
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 23 17:42:27 CET 2020
Hi Naush,
On 23/03/2020 15:29, Naushir Patuck wrote:
> Hi Kieran,
>
> On Fri, 20 Mar 2020 at 14:58, Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
>>
>> 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?
>>
>
> 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 ...
>
>>> + - 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
>
>>> + 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?
>>
>
> 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
>> --
>> Kieran
>
> Regards,
> Naush
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list