[libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update usage and description for existing controls

[Naushir Patuck]

Hi Kieran,


On Fri, 20 Mar 2020 at 15:40, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 09/03/2020 12:33, Naushir Patuck wrote:
> > Switch to using floats for constrast and saturation control to be more
>
> s/constrast/contrast/
>
> > in-line with end-user expectations.
> >
> > Expand on the descrption for the brightness, contrast and saturation
>
> s/descrption/description/
>
> I really should find some time to resume work on adding spell check to
> checkstyle.py :-S - it would be useful to everyone I'm sure.
>
> Maybe a task to give to an internship or put on our todo pages ;-)
>
> > controls.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9a33094a..3d1b058f 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -192,13 +192,20 @@ controls:
> >
> >    - Brightness:
> >        type: int32_t
> > -      description: Specify a fixed brightness parameter
> > +      description: |
> > +        Specify a fixed brightness parameter. Positive values (up to 65535)
> > +        produce brighter images; negative values (up to -65536) produce darker
>
> I think 'up to' still makes sense here ... but only asking to be devils
> advocate... should this be 'down to -65536' ?
>

Yes, that sounds correct :)

> I assume it's then up to the pipelines to convert that brightness range
> to whatever capabilities the hardware has anyway.
>
> Wouldn't the control specify the range through a ControlRange to
> indicate what range is actually supported?

So this was something we were questioning ourselves.  Should the
libcamera controls be explicit about range independent of the IPA?  If
no, listing the range here is not the right thing.  If yes, then it
ensures any libcamera application will use a defined range, and the
IPAs translate this to something they understand.

>
> > +        images and 0 leaves pixels unchanged.
> >
> >    - Contrast:
> > -      type: int32_t
> > -      description: Specify a fixed contrast parameter
> > +      type: float
> > +      description:  |
> > +        Specify a fixed contrast parameter. Normal contrast is given by the
> > +        value 1.0; larger values produce images with more contrast.
>
> What does a value less than 1.0 provide? Do we need to document that?
>
> If normal contrast is 1.0, and increasing the contrast is the range:
>   (1.0 > ~)
> does that mean 'decreasing' the contrast is only the range:
>   0.0 > 1.0 ?
>
> Presumably 0 contrast would then give a single colour image?
>
> Would this be more effective to use a scale of 0 == no change, > 0
> increases contrast, and < 0 decreases?
>

Essentially contrast controls the slope of the output pixel to input
pixel mapping and brightness the offset.  So contrast values of < 1.0
is certainly possible (but perhaps not that meaningful).  Perhaps we
should expand on the description a bit more?

> >
> >    - Saturation:
> > -      type: int32_t
> > -      description: Specify a fixed saturation parameter
> > +      type: float
> > +      description:  |
> > +        Specify a fixed saturation parameter. Normal saturation is given by
> > +        the value 1.0; larger values produce more saturated colours.
> >  ...
>
> Same comments I as above I think.

Again, saturation is essentially a multiplier on our colour matrix,
hence 1.0 is no change.

For all these controls, we were looking to make the usage as
mathematically "correct" (i.e, the values apply in some plausible
way), but try to keep them user friendly :)

>
> >
>
> --
> Regards
> --
> Kieran

Thanks,
Naush