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

[Laurent Pinchart]

Hi Stefan,

(CC'ing David)

On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:
> On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:
> > 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

David, is this the behaviour you would also expect for Raspberry Pi ?

> Do you have a nice sentence for that?

/me checks... I have a set of French proverbs, but none of the seem very
appropriate. Sorry :-)

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

This means that whether or not the estimated colour temperature will be
returned in metadata will be platform-dependent. Can we avoid that ? It
makes writing portable applications much more difficult.

My other concern is that metadata is supposed to report the setting
applied to a frame. The general rule is that, if a control can be set,
the value that has been set for a frame is reported in metadata. There
are exception for trigger-like controls. As this example clearly shows,
having multiple controls that ultimately set the same values is also
problematic from this point of view. Do we need to set a rule that
higher-level controls that get translated to lower-level controls are
never reported in metadata ?

If we do that, then we'll have ColourTemperature as a control being
defined differently from ColourTemperature as metadata. I'm not sure I
like it much. Should we have two different controls ?

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