| Message ID | 20200309123319.630-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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' ? 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? > + 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? > > - 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. >
Hi Kieran, On Fri, 20 Mar 2020 at 15:40, Kieran Bingham <kieran.bingham@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@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
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@raspberrypi.com> wrote: > > Hi Kieran, > > > On Fri, 20 Mar 2020 at 15:40, Kieran Bingham > <kieran.bingham@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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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.
Hi Laurent, On Thu, 26 Mar 2020 at 16:11, Laurent Pinchart <laurent.pinchart@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@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
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@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 ?
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 + 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. - 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. ...
Switch to using floats for constrast and saturation control to be more in-line with end-user expectations. Expand on the descrption for the brightness, contrast and saturation controls. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/control_ids.yaml | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)