[PATCH v4 1/6] libcamera: controls: Update the ColourTemperature control to be writable
Stefan Klug
stefan.klug at ideasonboard.com
Tue Dec 10 17:14:30 CET 2024
Hi Laurent,
maybe it's easiest to post the updated documentation diff before posting
the v6. I believe the rest is easy...
diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
index d45cf8e56187..2317515d6e8d 100644
--- a/src/libcamera/control_ids_core.yaml
+++ b/src/libcamera/control_ids_core.yaml
@@ -283,7 +283,19 @@ controls:
description: |
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 temperature, 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.
+
+ \sa ColourCorrectionMatrix
\sa ColourGains
+ \sa ColourTemperature
# AwbMode needs further attention:
# - Auto-generate max enum value.
@@ -338,17 +350,34 @@ controls:
Pair of gain values for the Red and Blue colour channels, in that
order.
- ColourGains can only be applied in a Request when the AWB is disabled.
+ ColourGains can only be applied in a Request when the AWB is disabled.
+ If ColourGains is set in a request but ColourTemperature is not, the
+ implementation shall calculate and set the ColourTemperature based on
+ the ColourGains.
\sa AwbEnable
+ \sa ColourTemperature
size: [2]
- ColourTemperature:
type: int32_t
description: |
- Report the estimate of the colour temperature for the frame, in kelvin.
+ ColourTemperature of the frame, in kelvin.
+
+ ColourTemperature can only be applied in a Request when the AWB is
+ disabled.
+
+ If ColourTemperature is set in a request but ColourGains is not, the
+ implementation shall calculate and set the ColourGains based on the
+ given ColourTemperature. If ColourTemperature is set but
+ ColourCorrectionMarix is not, the ColourCorrectionMarix is updated based
+ on the ColourTemperature.
+
+ The ColourTemperature used to process the frame is reported in metadata.
- The ColourTemperature control can only be returned in metadata.
+ \sa AwbEnable
+ \sa ColourCorrectionMatrix
+ \sa ColourGains
- Saturation:
type: float
@@ -405,6 +434,11 @@ controls:
stored in conventional reading order in an array of 9 floating point
values.
+ ColourCorrectionMatrix can only be applied in a Request when the AWB is
+ disabled.
+
+ \sa AwbEnable
+ \sa ColourTemperature
size: [3,3]
- ScalerCrop:
Cheers,
Stefan
On Tue, Dec 10, 2024 at 04:34:11PM +0100, Stefan Klug wrote:
> Hi Laurent,
>
> On Mon, Dec 09, 2024 at 03:02:09AM +0200, Laurent Pinchart wrote:
> > Looks like I forgot to reply to this.
> >
> > On Mon, Sep 02, 2024 at 08:28:24AM +0100, Naushir Patuck wrote:
> > > On Sat, 31 Aug 2024 at 16:16, Laurent Pinchart 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.
> >
> > I'll try to recap what we have (hopefully) agreed on so far.
> >
> > OK, so in manual AWB mode, CT and CG are interchangeable, one will be
> > calculated from the other based on the tuning data, without taking
> > statistics into account. As this is a pure software implementation, if
> > both controls are specified, we can freely specify which one takes
> > precedence, and this can be applied to all platforms. I have a
> > preference for making the gains take precedence (as Stefan did in v5) as
> > they provide more precise control.
>
> As written in the next paragraph, I propose for no precedence between CG
> and CT at all as we would unnecessarily limit our options.
>
> >
> > In auto AWB mode, CG and CT are both calculated by the AWB algorithm and
> > any value set through controls is ignored.
> >
> > So far, so good ?
> >
> > The next question is how to handle CCM. Auto mode is easy, CCM is
> > computed automatically, and any value set through controls is ignored.
> > In manual mode, if CCM is set in a request, its value should be applied.
> > What if a request contains CG or CT and no CCM ? Would CCM be computed
> > by the AWB algorithm (in manual mode), or its previous value retained ?
> >
>
> I prepared a new update to the controls documentation, that I'll post
> shortly. The proposal there is (for manual mode only):
>
> - CG updates CT if CT is not set.
> - CT updates CG if CG is not set.
> - CT updates CCM if CCM is not set (Which could be triggered by the CG ->
> CT -> CCM chain)
>
> That seems to be easiest to explain at the moment captures most use
> cases.
>
> > > > > 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.
> >
> > I'm fine with that behaviour, but I would like to keep the metadata
> > behaviour consistent between platforms.
> >
> > For auto-mode, I think we all agree that the CG, CT and CCM metadata
> > will report the values computed by the AWB algorithm (please wave if
> > you disagree).
> >
> > For manual mode, we've discussed two different behaviours that apply to
> > CG, CT and CCM:
> >
> > - Report the controls that are applied to the device. This matches the
> > standard libcamera metadata behaviour.
> >
> > - Let the AWB algorithm continue to process statistics, and report the
> > estimated values. As far as I understand, this was requested for CT
> > estimation only, not for CG or CCM estimation.
> >
> > While I understand that continuous CT estimation in manual mode can be
> > tempting, I think it would require more discussions to specify it
> > unambiguously. Stefan, Kieran, do you have a use case for this now, or
> > is this something you considered as a nice-to-have feature ?
>
> The discussion roughly went as follows:
>
> There are valid cases for both colour temperatures. The one from the
> statistics engine (measuredTemp) would be very useful to do any manual
> regulation (outside of libcamera). The one used in the ISP for
> processing the frame at hand (usedTemp) is useful to store in image
> metadata or similar things. But for none of them there is a hard use
> case.
>
> The issue with usedTemp is that it is not well defined in manual mode.
> At the time this was discussed we also had no way on rkisp1 to map from
> CG to CT. As you lean towards that value I propose the following:
>
> CT: usedTemp = CT, correct
> CG: usedTemp = CT(CG), correct (now possible on rkisp1 also)
> CT,CG: usedTemp = CT, incorrect because CG not taken into account but best
> effort
> CCM: usedTemp is unchanged, incorrect but best effort
>
> I think we can live with that incorrectness as it only affects corner
> cases.
>
> For the measuredTemp we can easily introduce a MeasuredColourTemperature
> metadata at a later stage.
>
> 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
More information about the libcamera-devel
mailing list