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

[Naushir Patuck]

Hi Laurent,



On Thu, 26 Mar 2020 at 16:11, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

I could see the advantages of doing this!

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

Perhaps both controls could be present and the application chooses how
it wants to control this?

> --
> Regards,
>
> Laurent Pinchart

Regards,
Naush