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

[David Plowman]

Just to add to some of that.

The brightness/contrast controls do the following to an image:

pixel_out = (pixel_in - 32768) * contrast + 32768

which is a relatively "standard" thing (our APIs always treat pixel
values as 16-bit). I'd be very much in favour of controls _not_ using
"arbitrary" ranges as applications would then have to remember, and
you suspect different people would choose different ranges and things
might wind up non-portable. So trying to stick close to the real
pixels in some mathematically meaningful way is my preference - not
least because you can explain it! But I think there's a discussion to
have there.

For reference that saturation control does:

RGB_out = YCbCr2RGB * Diag(1, saturation, saturation) * RGB2YCbCr * RGB_in

where:
RGB2YCbCr is a 3x3 matrix that converts RGB to Y plus (signed) blue
and red chrominance values,
Diag(a, b, c) makes a 3x3 matrix with a, b, c, down the diagonal, and
YCbCr2RGB turns the Y plus chrominance back to RGB (inverse matrix of
RGB2YCbCr).

David


On Mon, 23 Mar 2020 at 15:53, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel