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

[Naushir Patuck]

Hi Laurent,

On Tue, 18 Feb 2020 at 00:59, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

Yes, this is a tricky one, and the choice here will not please
everyone.  I will switch to using enums in the next version of the
patchset.  One strong argument for using strings is to allow users
(who may not be technically inclined) to add new modes to the tuning
and be able to use them without having the need to every recompile any
code.  To that end, what if I add, for example, {Custom1 Custom2
Custom3} enum values to the AE Mode?  This will allow the user to set
the mode in the tuning configuration, and select it in the application
without needing to ever recompile.  What do you think?  On a related
note, do you expect any mechanism where the IPAs advertise which enums
values are supported - this will be entirely platform/sensor/tuning
dependent.

>
> > +
> > +  - EV:
>
> Would it make sense to spell it out to ExposureValue ?
>

Yes, that sounds better.

> > +      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 think having EV applied only when AE is enabled makes sense - and
indeed, this is what our platform will do.  I will update the
description.

> 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 ?
>

Our platform supports a continuous range, I expect others will do as
well.  Normally I would expect discretizing the allowable values
should be handled by the camera application.

> > +  - 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 ?
>

I took ManualGain to be the sensor analogue gain value (although, it
could equally be applied to sensor digital gain I suppose).  This
pairs with the ManualExposure sensor ctrl.  ManualGainR and
ManualGainB are specific to the white balance control when AWB is off
and we provide these fixed gains to apply in the ISP pipeline.  So
there is no link between ManualGain and ManualGainR/ManualGainB.  Does
this seem reasonable?

> >  ...
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Naush