[libcamera-devel] [PATCH v2 1/2] libcamera: controls: Update the ColourTemperature control to be writable

[Kieran Bingham]

Hi David,

Quoting David Plowman (2023-12-05 09:00:54)
> Hi Kieran
> 
> In our implementation setting a fixed colour temperature is just like
> setting fixed colour gains. The AWB is "disabled" at that point, and
> you don't get any feedback as to what it thinks the colour temperature
> really is (as AWB is not doing a search any more). Setting a fixed
> colour temperature overrides any fixed colour gains, and also vice
> versa, because they basically do the same thing (i.e. set fixed colour
> gains).
> 
> On the Pi, the AwbMode is a bit different because it still does a
> search, but over a restricted range. I could imagine that other
> platforms do it more like "setting a fixed colour temperature",
> though.
> 
> Do you think some of these points need further explanation?

Yes certainly, I think we need to clarify that to make sure the
relationship between the controls isn't ambiguous, and tell users what
to expect.

Particularly when one control impacts another. So the declaration of
"Setting a fixed colour temperature overrides fixed ColourGains" is
essential.

And likely then the ColourGains text needs to be updated to reflect that
condition too.

Any controls which have a relationship, should also reference each other
with the \sa tag.

I expect it would be 'invalid' to set AwbMode, ColourGains, and
ColourTemperature all at the same time. If someone did so, what should
happen? Would one take precedence? Would you report a failure? What
should we document to make it clear on expectaions to the user?


If you think there are 'groups' of controls that need to be docuemnted
together, rather than repeat the same documentation in each control we
could also add specific control 'topics' to Doxygen to have a more descriptive
overview of the relationships between controls.

Based on Dan's latest Doxygen updates, we could have a targetted control
page and add these topics to a page such as src/libcamera/controls.dox.


--
Kieran



> David
> 
> On Mon, 4 Dec 2023 at 23:21, Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > Sorry - Coming back to this one as there are things that aren't clear
> > enough to merge it yet.
> >
> > Quoting Kieran Bingham (2023-11-28 10:25:08)
> > > Quoting David Plowman via libcamera-devel (2023-11-23 13:09:04)
> > > > Implementations are allowed to expose this as a settable control so
> > > > that applications can demand a specific colour temperature. The
> > > > description is updated to this effect.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > >  src/libcamera/control_ids.yaml | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > index c3232abf..8fbcc5f6 100644
> > > > --- a/src/libcamera/control_ids.yaml
> > > > +++ b/src/libcamera/control_ids.yaml
> > > > @@ -312,9 +312,11 @@ controls:
> > > >
> > > >    - ColourTemperature:
> > > >        type: int32_t
> > > > -      description: Report the current estimate of the colour temperature, in
> > > > -        kelvin, for this frame. The ColourTemperature control can only be
> > > > -        returned in metadata.
> > > > +      description: |
> > > > +        Report the current estimate of the colour temperature, in kelvin, for
> > > > +        this frame. An implementation may also allow this control to be set so
> > > > +        as to disable the AWB algorithm whilst setting the colour gains to the
> > > > +        correct fixed ones for this colour temperature.
> > >
> > > This certainly frames it as a mostly metadata control rather than a
> > > control-control. So I think that's fine as that's possibly the
> > > expectation for the most part.
> > >
> > > I have in mind to do some documentation udpates in the near future so I
> > > wonder if I should look through all the controls and normalise how we
> > > present the descriptions if they are read-only or settable or only
> > > if supported by the pipeline handler...
> > >
> > > But I don't object to this for now so:
> >
> > How does this ColourTemperature relate to the AwbMode ? Which one takes
> > precedence if set? (I assume based on the current text that this one
> > overrides any setting of an AwbMode).
> >
> > What happens to ColourGains control if this is set? Does it override
> > those too?  Or do they take precedence? Or is this just a helper to be
> > able to set ColourGains based on the tuning?
> >
> > Does this control end up being an equivalent internal implementation
> > detail of setting the AwbModes, i.e. perhaps is setting AwbCloudy the
> > same as setting:
> >
> >   { ColourTemperature, 6000 },
> >
> > or perhaps setting AwbIncandescent would be:
> >
> >   { ColourTemperature, 2700 },
> >
> > And does such usage override the need/requirement to have an AwbCustom
> > mode (which currently looks quite ill defined?
> >
> > If the user sets ColourTemperature, will there still be anything that
> > can measure the ColourTemperature, and would return perhaps the
> > 'measured' ColourTemperature back in the metadata? or would it always
> > return what the user 'Set' it to ?
> >
> > --
> > Kieran
> >
> >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > >
> > > >    - Saturation:
> > > >        type: float
> > > > --
> > > > 2.39.2
> > > >