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

[Laurent Pinchart]

Hello,

On Mon, Mar 23, 2020 at 04:15:29PM +0000, David Plowman wrote:
> 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.

I agree regarding ranges, if we can standardize them, that's best.

> 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).
> 
> On Mon, 23 Mar 2020 at 15:53, Naushir Patuck wrote:
> > On Fri, 20 Mar 2020 at 15:40, Kieran Bingham wrote:
> > > 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 :)

I've been thinking about these controls. I would first like to point out
that they were added more as initial placeholders than as real controls,
to have something to experiment with. If there's any control that you
think doesn't make much sense, please feel free to say so.

Then, regarding contrast and brightness, I wonder if we should go for
support of more complex tonemap curves. Gamma comes to mind in this
context, and the Android camera HAL exposes a configurable curve as a
set of points (with the camera reporting the number of points it
supports, so a camera that only supports contrast and brightness would
only accept two points), maybe that could be worth considering. It
doesn't have to be done now, we can change it later, as long as we do so
before freezing the API.

For saturation I similarly wonder if we shouldn't handle it through a
configurable RGB to RGB matrix.

-- 
Regards,

Laurent Pinchart