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

[Laurent Pinchart]

Hi Naush,

On Fri, Mar 27, 2020 at 11:35:32AM +0000, Naushir Patuck wrote:
> On Thu, 26 Mar 2020 at 16:11, Laurent Pinchart wrote:
> > 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?

I've been giving this some more thoughts. I would like to avoid IPAs
having to handle multiple controls for the same purpose if possible. Do
you think it would be a good idea to handle this in upper layers
instead, with helpers for applications to set brightness, contrast and
saturation, that would fill the tonemap and RGB2RGB controls in the
request ?

-- 
Regards,

Laurent Pinchart