| Message ID | 20231123130905.103934-1-david.plowman@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting David Plowman via libcamera-devel (2023-11-23 13:09:04) > Implementations are allowed to expose this as a settable control so > that applications can demand a specific colour temperature. The > description is updated to this effect. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/control_ids.yaml | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index c3232abf..8fbcc5f6 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -312,9 +312,11 @@ controls: > > - 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 so > + as to disable the AWB algorithm whilst setting the colour gains to the > + correct fixed ones for this colour temperature. This certainly frames it as a mostly metadata control rather than a control-control. So I think that's fine as that's possibly the expectation for the most part. I have in mind to do some documentation udpates in the near future so I wonder if I should look through all the controls and normalise how we present the descriptions if they are read-only or settable or only if supported by the pipeline handler... But I don't object to this for now so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > - Saturation: > type: float > -- > 2.39.2 >
Hi David, Sorry - Coming back to this one as there are things that aren't clear enough to merge it yet. Quoting Kieran Bingham (2023-11-28 10:25:08) > Quoting David Plowman via libcamera-devel (2023-11-23 13:09:04) > > Implementations are allowed to expose this as a settable control so > > that applications can demand a specific colour temperature. The > > description is updated to this effect. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/control_ids.yaml | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index c3232abf..8fbcc5f6 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -312,9 +312,11 @@ controls: > > > > - 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 so > > + as to disable the AWB algorithm whilst setting the colour gains to the > > + correct fixed ones for this colour temperature. > > This certainly frames it as a mostly metadata control rather than a > control-control. So I think that's fine as that's possibly the > expectation for the most part. > > I have in mind to do some documentation udpates in the near future so I > wonder if I should look through all the controls and normalise how we > present the descriptions if they are read-only or settable or only > if supported by the pipeline handler... > > But I don't object to this for now so: How does this ColourTemperature relate to the AwbMode ? Which one takes precedence if set? (I assume based on the current text that this one overrides any setting of an AwbMode). What happens to ColourGains control if this is set? Does it override those too? Or do they take precedence? Or is this just a helper to be able to set ColourGains based on the tuning? Does this control end up being an equivalent internal implementation detail of setting the AwbModes, i.e. perhaps is setting AwbCloudy the same as setting: { ColourTemperature, 6000 }, or perhaps setting AwbIncandescent would be: { ColourTemperature, 2700 }, And does such usage override the need/requirement to have an AwbCustom mode (which currently looks quite ill defined? If the user sets ColourTemperature, will there still be anything that can measure the ColourTemperature, and would return perhaps the 'measured' ColourTemperature back in the metadata? or would it always return what the user 'Set' it to ? -- Kieran > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > - Saturation: > > type: float > > -- > > 2.39.2 > >
Hi Kieran In our implementation setting a fixed colour temperature is just like setting fixed colour gains. The AWB is "disabled" at that point, and you don't get any feedback as to what it thinks the colour temperature really is (as AWB is not doing a search any more). Setting a fixed colour temperature overrides any fixed colour gains, and also vice versa, because they basically do the same thing (i.e. set fixed colour gains). On the Pi, the AwbMode is a bit different because it still does a search, but over a restricted range. I could imagine that other platforms do it more like "setting a fixed colour temperature", though. Do you think some of these points need further explanation? David On Mon, 4 Dec 2023 at 23:21, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi David, > > Sorry - Coming back to this one as there are things that aren't clear > enough to merge it yet. > > Quoting Kieran Bingham (2023-11-28 10:25:08) > > Quoting David Plowman via libcamera-devel (2023-11-23 13:09:04) > > > Implementations are allowed to expose this as a settable control so > > > that applications can demand a specific colour temperature. The > > > description is updated to this effect. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/control_ids.yaml | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > index c3232abf..8fbcc5f6 100644 > > > --- a/src/libcamera/control_ids.yaml > > > +++ b/src/libcamera/control_ids.yaml > > > @@ -312,9 +312,11 @@ controls: > > > > > > - 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 so > > > + as to disable the AWB algorithm whilst setting the colour gains to the > > > + correct fixed ones for this colour temperature. > > > > This certainly frames it as a mostly metadata control rather than a > > control-control. So I think that's fine as that's possibly the > > expectation for the most part. > > > > I have in mind to do some documentation udpates in the near future so I > > wonder if I should look through all the controls and normalise how we > > present the descriptions if they are read-only or settable or only > > if supported by the pipeline handler... > > > > But I don't object to this for now so: > > How does this ColourTemperature relate to the AwbMode ? Which one takes > precedence if set? (I assume based on the current text that this one > overrides any setting of an AwbMode). > > What happens to ColourGains control if this is set? Does it override > those too? Or do they take precedence? Or is this just a helper to be > able to set ColourGains based on the tuning? > > Does this control end up being an equivalent internal implementation > detail of setting the AwbModes, i.e. perhaps is setting AwbCloudy the > same as setting: > > { ColourTemperature, 6000 }, > > or perhaps setting AwbIncandescent would be: > > { ColourTemperature, 2700 }, > > And does such usage override the need/requirement to have an AwbCustom > mode (which currently looks quite ill defined? > > If the user sets ColourTemperature, will there still be anything that > can measure the ColourTemperature, and would return perhaps the > 'measured' ColourTemperature back in the metadata? or would it always > return what the user 'Set' it to ? > > -- > Kieran > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > - Saturation: > > > type: float > > > -- > > > 2.39.2 > > >
Hi David, Quoting David Plowman (2023-12-05 09:00:54) > Hi Kieran > > In our implementation setting a fixed colour temperature is just like > setting fixed colour gains. The AWB is "disabled" at that point, and > you don't get any feedback as to what it thinks the colour temperature > really is (as AWB is not doing a search any more). Setting a fixed > colour temperature overrides any fixed colour gains, and also vice > versa, because they basically do the same thing (i.e. set fixed colour > gains). > > On the Pi, the AwbMode is a bit different because it still does a > search, but over a restricted range. I could imagine that other > platforms do it more like "setting a fixed colour temperature", > though. > > Do you think some of these points need further explanation? Yes certainly, I think we need to clarify that to make sure the relationship between the controls isn't ambiguous, and tell users what to expect. Particularly when one control impacts another. So the declaration of "Setting a fixed colour temperature overrides fixed ColourGains" is essential. And likely then the ColourGains text needs to be updated to reflect that condition too. Any controls which have a relationship, should also reference each other with the \sa tag. I expect it would be 'invalid' to set AwbMode, ColourGains, and ColourTemperature all at the same time. If someone did so, what should happen? Would one take precedence? Would you report a failure? What should we document to make it clear on expectaions to the user? If you think there are 'groups' of controls that need to be docuemnted together, rather than repeat the same documentation in each control we could also add specific control 'topics' to Doxygen to have a more descriptive overview of the relationships between controls. Based on Dan's latest Doxygen updates, we could have a targetted control page and add these topics to a page such as src/libcamera/controls.dox. -- Kieran > David > > On Mon, 4 Dec 2023 at 23:21, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Hi David, > > > > Sorry - Coming back to this one as there are things that aren't clear > > enough to merge it yet. > > > > Quoting Kieran Bingham (2023-11-28 10:25:08) > > > Quoting David Plowman via libcamera-devel (2023-11-23 13:09:04) > > > > Implementations are allowed to expose this as a settable control so > > > > that applications can demand a specific colour temperature. The > > > > description is updated to this effect. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/control_ids.yaml | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > > index c3232abf..8fbcc5f6 100644 > > > > --- a/src/libcamera/control_ids.yaml > > > > +++ b/src/libcamera/control_ids.yaml > > > > @@ -312,9 +312,11 @@ controls: > > > > > > > > - 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 so > > > > + as to disable the AWB algorithm whilst setting the colour gains to the > > > > + correct fixed ones for this colour temperature. > > > > > > This certainly frames it as a mostly metadata control rather than a > > > control-control. So I think that's fine as that's possibly the > > > expectation for the most part. > > > > > > I have in mind to do some documentation udpates in the near future so I > > > wonder if I should look through all the controls and normalise how we > > > present the descriptions if they are read-only or settable or only > > > if supported by the pipeline handler... > > > > > > But I don't object to this for now so: > > > > How does this ColourTemperature relate to the AwbMode ? Which one takes > > precedence if set? (I assume based on the current text that this one > > overrides any setting of an AwbMode). > > > > What happens to ColourGains control if this is set? Does it override > > those too? Or do they take precedence? Or is this just a helper to be > > able to set ColourGains based on the tuning? > > > > Does this control end up being an equivalent internal implementation > > detail of setting the AwbModes, i.e. perhaps is setting AwbCloudy the > > same as setting: > > > > { ColourTemperature, 6000 }, > > > > or perhaps setting AwbIncandescent would be: > > > > { ColourTemperature, 2700 }, > > > > And does such usage override the need/requirement to have an AwbCustom > > mode (which currently looks quite ill defined? > > > > If the user sets ColourTemperature, will there still be anything that > > can measure the ColourTemperature, and would return perhaps the > > 'measured' ColourTemperature back in the metadata? or would it always > > return what the user 'Set' it to ? > > > > -- > > Kieran > > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > - Saturation: > > > > type: float > > > > -- > > > > 2.39.2 > > > >
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index c3232abf..8fbcc5f6 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -312,9 +312,11 @@ controls: - 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 so + as to disable the AWB algorithm whilst setting the colour gains to the + correct fixed ones for this colour temperature. - Saturation: type: float
Implementations are allowed to expose this as a settable control so that applications can demand a specific colour temperature. The description is updated to this effect. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/control_ids.yaml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)