[PATCH v4 1/6] libcamera: controls: Update the ColourTemperature control to be writable

[Stefan Klug]

Hi Laurent,

Thank you for the review.

On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:
> > For manual control it is helpful to be able to specify a fixed colour
> > temperature. It also provides an easy way to apply the temperature
> > specific CCMs and colour gains that are contained in the tuning files.
> > 
> > Document this and update the control dependencies.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
> > index cf40771d3896..04dcc4af67fc 100644
> > --- a/src/libcamera/control_ids_core.yaml
> > +++ b/src/libcamera/control_ids_core.yaml
> > @@ -252,9 +252,12 @@ controls:
> >    - AwbEnable:
> >        type: bool
> >        description: |
> > -        Enable or disable the AWB.
> > +        Enable or disable the AWB. Disabling AWB stops updates to the
> > +        ColourGains and to the ColourCorrectionMatrix.
> 
> Please split this in two paragraphs. The first paragraph is translated
> to a \brief doxygen command, and should be a single sentence. Same
> below.
> 
> I would like to clarify this a bit. Here's an attempt, is it what you
> mean ?
> 
> 	Enable or disable the AWB.
> 
> 	When AWB is enabled, the algorithm estimates the colour	temperature of
> 	the scene, and computes colour gains and the colour correction matrix
> 	automatically. The computed colour temperate, gains and correction
> 	matrix are reported in metadata. The corresponding controls are ignored
> 	if set in a request.
> 
> 	When AWB is disabled, the colour temperature, gains and correction
> 	matrix are not updated automatically and can be set manually in
> 	requests.

Looks good, I'll apply that.

> 
> >  
> > +        \sa ColourCorrectionMatrix
> >          \sa ColourGains
> > +        \sa ColourTemperature
> >  
> >    # AwbMode needs further attention:
> >    # - Auto-generate max enum value.
> > @@ -309,13 +312,24 @@ controls:
> >          disabled.
> >  
> >          \sa AwbEnable
> > +        \sa ColourTemperature
> >        size: [2]
> >  
> >    - 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 when
> 
> s/also //
> 
> > +        AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix
> > +        get set accordingly. If either ColourGains or ColourCorrectionMatrix are
> > +        specified at the same time, they take precedence.
> 
> maybe "they take precedence, and the ColourTemperature is ignored" ? Or,
> if an application sets ColourTemperature and ColourGains but not
> ColourCorrectionMatrix, do you expect the implementation to apply the
> requested ColourGains and set ColourCorrectionMatrix based on the
> temperature ?

The latter is exactly what happens. The following table lists the
possible options:

CT -> CG(CT), CCM(CT)
CT, CG -> CG, CCM(CT)
CT, CCM -> CG(CT), CCM
CT, CG, CCM -> CG, CCM

Do you have a nice sentence for that?

> 
> > +
> > +        The metadata will only report measured colour temperature values when
> > +        available, even if set manually.
> 
> I'm not sure to understand this.

This is based on the comment from Kieran:
https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare
for cases (RPi) where no temperature gets estimated. Only measurements
are returned in metadata. Manually set temperature is not reflected in
the metadata. This has the nice side effect, that you can set
AwbEnable=false, and still get temperature estimations in the metadata.

> 
> > +
> > +        \sa AwbEnable
> > +        \sa ColourCorrectionMatrix
> > +        \sa ColourGains
> >  
> >    - Saturation:
> >        type: float
> > @@ -365,6 +379,8 @@ controls:
> >          transformation. The 3x3 matrix is stored in conventional reading
> >          order in an array of 9 floating point values.
> >  
> > +        \sa AwbEnable
> > +        \sa ColourTemperature
> >        size: [3,3]
> >  
> >    - ScalerCrop:
> 
> -- 
> Regards,
> 
> Laurent Pinchart