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

[Stefan Klug]

Hi Laurent,

On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:
> 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 :-)
>

What about "If ColourGains and ColourTemperature are specified at the
same time, ColourGains takes precedence. The same applies to
ColourCorrectionMatrix."?

We can discuss the final wording after feedback from David.

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

We could split that to "MeasuredColourTemperature" and
"AppliedColorTemperature". But there are always cases where one of them
(or both) is not available (as discussed on Patch 3). I think on rkisp
we could ensure that MeasuredColourTemperature is always available and
contains "something" as the statistics are always available. But in the
end as a user I'd prefer to know when the algorithm failed to deduce a
valid colour temperature.

Regards,
Stefan

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