[v1,1/6] libcamera: controls: Update the ColourTemperature control to be writable

20771 diff mbox series

For manual control it is helpful to be able to specify a fixed colour
temperature. It also provides a 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(-)

Hi Stefan,

Thanks for updating this

Technically, I think this patch might be at v3, given:

  https://patchwork.libcamera.org/patch/19231/
[libcamera-devel,v2,1/2] libcamera: controls: Update the ColourTemperature control to be writable

But lets continue ;-)

Quoting Stefan Klug (2024-08-05 13:05:02)
> For manual control it is helpful to be able to specify a fixed colour
> temperature. It also provides a easy way to apply the temperature

s/a/an/

> 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 9d413a94e0ee..90a52bccaa98 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 takes precedence.

I presume both ColourGains and ColourCorrectionMatrix can operate
independently of each other though.

> +
> +        Note that the metadata always contains the measured value, even when the
> +        value was set manually.

Will this always be true? Or is it possible to always be true? I see
that David stated that in the Raspberry Pi implementation

https://patchwork.libcamera.org/patch/19231/#28242 :
"
> 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).
"

Presumably if no colour temperature is measured though - it simply
doesn't get returned in the metadata ... so perhaps this isn't an issue.

Instead of 'always' which implies it will be guaranteed to be set .. I
might reword slightly to:

"The metadata will only report measured colour temperature values when
available even if set manually."

Maybe that's closer to what would be implemented?


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

Thanks for the review.

On Mon, Aug 05, 2024 at 02:41:18PM +0100, Kieran Bingham wrote:
> Hi Stefan,
> 
> Thanks for updating this
> 
> Technically, I think this patch might be at v3, given:
> 
>   https://patchwork.libcamera.org/patch/19231/
> [libcamera-devel,v2,1/2] libcamera: controls: Update the ColourTemperature control to be writable
> 

Yes you're right. This commit was the last one I added to the series. So
it felt like a v1 :-)

> But lets continue ;-)
> 
> Quoting Stefan Klug (2024-08-05 13:05:02)
> > For manual control it is helpful to be able to specify a fixed colour
> > temperature. It also provides a easy way to apply the temperature
> 
> s/a/an/
> 
> > 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 9d413a94e0ee..90a52bccaa98 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 takes precedence.
> 
> I presume both ColourGains and ColourCorrectionMatrix can operate
> independently of each other though.

Yes they can, but if you set ColorTemperature, both get set.

> 
> > +
> > +        Note that the metadata always contains the measured value, even when the
> > +        value was set manually.
> 
> Will this always be true? Or is it possible to always be true? I see
> that David stated that in the Raspberry Pi implementation
> 
> https://patchwork.libcamera.org/patch/19231/#28242 :
> "
> > 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).
> "
> 
> Presumably if no colour temperature is measured though - it simply
> doesn't get returned in the metadata ... so perhaps this isn't an issue.
> 
> Instead of 'always' which implies it will be guaranteed to be set .. I
> might reword slightly to:
> 
> "The metadata will only report measured colour temperature values when
> available even if set manually."
> 
> Maybe that's closer to what would be implemented?
> 

Sure. I updated it for v2 (or v4 :-) ).

Cheers,
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:
> > -- 
> > 2.43.0
> >

Stefan Klug <stefan.klug@ideasonboard.com> writes:

> For manual control it is helpful to be able to specify a fixed colour
> temperature. It also provides a 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 9d413a94e0ee..90a52bccaa98 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 takes precedence.

s/takes/take/

> +        Note that the metadata always contains the measured value, even when the
> +        value was 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: