[libcamera-devel,v4,3/5] libcamera: controls: Reorder and update description of existing controls

3515 diff mbox series

Group AE, AWB, etc. controls together for accessibility.

Update descriptions for Contrast, Brightness, and Saturation controls.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/control_ids.yaml | 41 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 17 deletions(-)

Hi Naush,

Thank you for the patch.

On Fri, Apr 24, 2020 at 11:46:58AM +0100, Naushir Patuck wrote:
> Group AE, AWB, etc. controls together for accessibility.
> 
> Update descriptions for Contrast, Brightness, and Saturation controls.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 41 ++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index d8bdb382..f7403081 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -25,23 +25,6 @@ controls:
>  
>          \sa AeEnable
>  
> -  - AwbEnable:
> -      type: bool
> -      description: |
> -        Enable or disable the AWB.
> -
> -  - Brightness:
> -      type: int32_t
> -      description: Specify a fixed brightness parameter
> -
> -  - Contrast:
> -      type: int32_t
> -      description: Specify a fixed contrast parameter
> -
> -  - Saturation:
> -      type: int32_t
> -      description: Specify a fixed saturation parameter
> -
>    - ExposureTime:
>        type: int32_t
>        description: |
> @@ -58,4 +41,28 @@ controls:
>          colour channels. This value cannot be lower than 1.0.
>  
>          \sa ExposureTime AeEnable
> +
> +  - Brightness:
> +      type: float
> +      description: |
> +        Specify a fixed brightness parameter. Positive values (up to 65535.0)
> +        produce brighter images; negative values (up to -65536.0) produce darker
> +        images and 0.0 leaves pixels unchanged.

Maybe a bit of a stupid question, but if we use a float, the proposed
range seems a bit awkward to me. Should we go for a [-1.0, +1.0] range ?

> +
> +  - Contrast:
> +      type: float
> +      description:  |
> +        Specify a fixed contrast parameter. Normal contrast is given by the
> +        value 1.0; larger values produce images with more contrast.
> +
> +  - AwbEnable:
> +      type: bool
> +      description: |
> +        Enable or disable the AWB.
> +
> +  - Saturation:
> +      type: float
> +      description:  |
> +        Specify a fixed saturation parameter. Normal saturation is given by
> +        the value 1.0; larger values produce more saturated colours.

Is it worth adding that 0.0 produces greyscale images ?

Let's not spend too much time on this, I'm sure we'll have a chance to
rework these controls later anyway.

With the above two issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can also update the patch when applying if you're fine with the above
proposals.

>  ...

Hi Laurent,

On Fri, 24 Apr 2020 at 14:11, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Apr 24, 2020 at 11:46:58AM +0100, Naushir Patuck wrote:
> > Group AE, AWB, etc. controls together for accessibility.
> >
> > Update descriptions for Contrast, Brightness, and Saturation controls.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml | 41 ++++++++++++++++++++--------------
> >  1 file changed, 24 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index d8bdb382..f7403081 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -25,23 +25,6 @@ controls:
> >
> >          \sa AeEnable
> >
> > -  - AwbEnable:
> > -      type: bool
> > -      description: |
> > -        Enable or disable the AWB.
> > -
> > -  - Brightness:
> > -      type: int32_t
> > -      description: Specify a fixed brightness parameter
> > -
> > -  - Contrast:
> > -      type: int32_t
> > -      description: Specify a fixed contrast parameter
> > -
> > -  - Saturation:
> > -      type: int32_t
> > -      description: Specify a fixed saturation parameter
> > -
> >    - ExposureTime:
> >        type: int32_t
> >        description: |
> > @@ -58,4 +41,28 @@ controls:
> >          colour channels. This value cannot be lower than 1.0.
> >
> >          \sa ExposureTime AeEnable
> > +
> > +  - Brightness:
> > +      type: float
> > +      description: |
> > +        Specify a fixed brightness parameter. Positive values (up to 65535.0)
> > +        produce brighter images; negative values (up to -65536.0) produce darker
> > +        images and 0.0 leaves pixels unchanged.
>
> Maybe a bit of a stupid question, but if we use a float, the proposed
> range seems a bit awkward to me. Should we go for a [-1.0, +1.0] range ?
>
> > +
> > +  - Contrast:
> > +      type: float
> > +      description:  |
> > +        Specify a fixed contrast parameter. Normal contrast is given by the
> > +        value 1.0; larger values produce images with more contrast.
> > +
> > +  - AwbEnable:
> > +      type: bool
> > +      description: |
> > +        Enable or disable the AWB.
> > +
> > +  - Saturation:
> > +      type: float
> > +      description:  |
> > +        Specify a fixed saturation parameter. Normal saturation is given by
> > +        the value 1.0; larger values produce more saturated colours.
>
> Is it worth adding that 0.0 produces greyscale images ?
>
> Let's not spend too much time on this, I'm sure we'll have a chance to
> rework these controls later anyway.
>

Both suggestions here are good with us.  Please go ahead and make the
changes before merging if that's ok.

Regards,
Naush

> With the above two issues addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I can also update the patch when applying if you're fine with the above
> proposals.
>
> >  ...
>
> --
> Regards,
>
> Laurent Pinchart