| Message ID | 20240813084451.44099-2-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@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. > > + \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 > + 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. > + > + The metadata will only report measured colour temperature values when > + available, even if set manually. > + > + \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: > -- > 2.43.0 >
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@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. > > + \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 metadata will only report measured colour temperature values when > + available, even if set manually. I'm not sure to understand this. > + > + \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:
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@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
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@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:
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@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
Hi everyone On Fri, 30 Aug 2024 at 12:21, Stefan Klug <stefan.klug@ideasonboard.com> wrote: > > 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@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 (?). 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 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). 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. Does that help or just confuse further? Thanks! David > > > > > 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
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@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 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 ? > 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 ? > > > > > > + > > > > > > + \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:
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@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@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
On Sat, Aug 31, 2024 at 06:16:10PM +0300, 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@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 ? Only CCM is set based on CT. CG is set based on the stats and these are also used to calculate the CT. As David noted CG -> CT(CG) -> CCM(CT) is possible. CT(CCM) is not possible afaik. So the cases are similar but not the same. > > > 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 ? At the moment, we do not estimate CT from CG. But that might be worth to do. > > > 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. Yes, I think on that one we are all fine. So CT -> CG(CT), CCM(CT) is accepted. Metadata might be an open point here. > > > 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 ? To me the most prominent use case is manual WB. So the user takes a picture of a gray patch and the application (not libcamera) calculates wb gains based on that. If you compare to a typical digital camera I think one would expect the following to happen: CG -> CT(CG) -> CCM(CT) In rkisp1 we don't have that CG->CT(CG) step, but that doesn't mean we shouldn't do it. Looking at that with the eyes of a machine-vision user it would be counter intuitive that the CCM suddenly changes when I change the CG. But that can be worked around from user side, by supplying a manual CCM in the request. Which brings me to the next point. The CCM is the only parameter here, that is basically impossible to handcraft in an application. So there might be use cases where I'd like to set a CCM based on a color temperature and then hand tweak the CG. So I would be fine if we say CG -> CG, CCM(CT(CG)). That is new to me and I haven't look into the implementation effort yet. CG, CT -> CG, CCM(CT) is something I'd like to have, but I don't know if we need to force it (@David: would it be much effort on your side, just for completeness sake?) CCM, CT -> CG(CT), CCM also feels natural to me, but without any known usecase. (Maybe in calibration, where you want to force a unit ccm and check the correctness of the gains) CT, CG, CCM -> CG, CCM I think we can agree on that one. It is overspecified, but the lower level controls should win. > > > 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 ? > > > > > > > > + > > > > > > > + \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 Regards, Stefan
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. + \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 + 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. + + The metadata will only report measured colour temperature values when + available, even if set manually. + + \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:
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@ideasonboard.com> --- src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)