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

Naushir Patuck naush at raspberrypi.com
Mon Sep 2 09:28:24 CEST 2024 

Hi Laurent,

David's away this week, so I'll answer on his behalf.

On Sat, 31 Aug 2024 at 16:16, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:
> > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:
> > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:
> > > > 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 ?
> >
> > I'm guessing the question here is really what to do if an application
> > sets both CT and CG together? The other cases seem fairly
> > uncontroversial (?).
>
> As far as I understand, for the rkisp1, the colour gains and the CCM are
> both calculated based on the colour temperature. The case where the
> application sets CT and CG is conceptually as problematic as the case
> where it sets CT and CCM. Stefan, is that correct ?
>
> > For us, I'm not sure it really makes much sense to supply both CT and
> > CG at once. When someone sets CG, the algorithm estimates the CT and
> > passes this to other algorithms, like lens shading and CCM. (So we
> > actually have a "CG -> CG, CT(CG), CCM(CT)" kind of case going on.)
>
> When an application sets CG, does the algorithm estimate CT from the
> stats, or from the CG alone for RPi ? Stefan, how about your plans for
> rkisp1 ?

When an application sets manual CGs, the algorithm estimates the CT
from the CT-curve in the tuning directly.

>
> > When I add this new control, it will calculate CG from the calibrated
> > colour temperature curve in the camera tuning and use those, and pass
> > the CT to those other algorithms (so like the first row in the table).
>
> That's the main use, and I think we all agree on the behaviour. It seems
> quite uncontroversial.
>
> > I don't think I particularly want to implement cases where it takes a
> > CG value, uses them, and also a random CT which it then passes on?
> > Possibly I'd rather leave these cases (where we have both CT and CG on
> > the LHS) as either "implementation dependent" or just "undefined", and
> > recommend setting one or other.
>
> I dislike implementation-dependent behaviours if we can avoid them, at
> least in cases that we consider valid use cases. Is there a valid use
> case for setting CT along with CG or CCM ? It sounds to me like there
> wouldn't be one for RPi. Stefan, how about you ?

I don't think RPi has a use case that requires the CT and CGs to be both set.

>
> > Does that help or just confuse further?
>
> Let's see how the discussion progresses :-) Thank you for your input.
>
> > > > > 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.
>
> If the algorithm failed to estimate the colour temperature, that should
> be indicated by the absence of an estimated colour temperature in
> metadata.
>
> David, why do you (if I understand correctly) stop estimating the colour
> temperature from the statistics in manual AWB mode ?

I think it's to keep things consistent.  The CT-curve in the tuning
file is taken as the canonical truth. So if manual CGs are applied,
and assuming the users know what they are doing(!), we return a simple
lookup without going through all the computations again.  Of course,
this is simple enough to change in our code if the need arises.

Regards,
Naush

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

More information about the libcamera-devel mailing list