[libcamera-devel] [PATCH 3/3] libcamera: controls: Add AE/AWB mode, manual and EV controls.

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Feb 17, 2020 at 02:26:09PM +0000, Naushir Patuck wrote:
> AeMode is a new std::string type control used to set the AE algorithm
> mode. The mode may not always be supported by all platforms.
> 
> AwbMode is a new std::string type control used to set the AWB algorithm
> illuminant mode. The mode may not always be supported by all platforms.
> 
> EV is a new double type control used to set the log2 exposure adjustment
> for the AE algorithm.
> 
> ManualGainR is a new double type control used to set the Red channel
> gain in manual AWB mode.
> 
> ManualGainB is a new double type control used to set the Blue channel
> gain in manual AWB mode.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.org>
> ---
>  src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 33062d6..21d2065 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -50,4 +50,44 @@ controls:
>        type: double
>        description: Specify a fixed gain parameter.
>  
> +  - AeMode:
> +      type: std::string
> +      description: |
> +        Specify an exposure mode for the AE algorithm to use. The exposure modes
> +        supported are platform spcific, and not all modes may be supported.

s/spcific/specific/

> +
> +        Examples include "normal", "sport", "low light", etc.
> +
> +  - AwbMode:
> +      type: std::string
> +      description: |
> +        Specify the range of illuminant to use for the AWB algorihtm. The modes
> +        supported are platform specific, and not all modes may be supported.
> +
> +        Examples include "auto", "incandescent", "daylight", etc.

I think we should use enumerated values instead of strings for both
controls. I do agree that strings have the nice benefit of being easily
extensible, but they will be much more difficult to use for
applications. A typical end-user camera application will display icons
for the different modes, and will thus need to map strings to icons. In
order to allow doing so, we would need to specify what mode strings are
supported in this spec, which will remove the extensibility feature of
strings. Strings would then have no advantage over numerical values
anymore.

I do however agree that an extension mechanism can be useful, but to
design it properly, I think we need to discuss how such extensions would
be used by applications.

> +
> +  - EV:

Would it make sense to spell it out to ExposureValue ?

> +      type: double
> +      description: |
> +        Specify an Exposure Value (EV) parameter.
> +
> +        By convention, EV adjusts the exposure by a factor of 2^EV. 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].

You should also explain here how this interacts with the AeEnable
control. One option would be to mention that the control only applies
when AE is enabled, and that if AE is disabled, the EV value being set
will take effect next time AE is enabled. I'm open to discussing
alternative proposals.

I would also like to know what values you envision being supported for
this control. Would an IPA typically support a continuous range, or
discrete values ?

> +  - ManualGainR:
> +      type: double
> +      description: |
> +        Specify a fixed gain parameter for the Red colour channel. This must be
> +        set together with ManualGainB to be applied.
> +
> +        \sa ManualGainB
> +
> +  - ManualGainB:
> +      type: double
> +      description:  |
> +        Specify a fixed gain parameter for the Green colour channel. This must
> +        bet together with AwbManualGainR to be applied.
> +
> +        \sa ManualGainR

How do those two controls interact with ManualGain ? And where in the
pipeline do they apply ?

>  ...

-- 
Regards,

Laurent Pinchart