[libcamera-devel] ipa: raspberrypi: Correct some of the ControlInfo ranges and defaults
diff mbox series

Message ID 20220328100544.13477-1-david.plowman@raspberrypi.com
State Accepted
Commit 8dc2699bb87f5328dd3aae540d6e9d858212f774
Headers show
Series
  • [libcamera-devel] ipa: raspberrypi: Correct some of the ControlInfo ranges and defaults
Related show

Commit Message

David Plowman March 28, 2022, 10:05 a.m. UTC 
Some of the values were listed incorrectly. Specifically:

ExposureValue: the range is now centred correctly on zero
Brightness: the default value (0.0) is made explicit
Contrast: the default value is corrected to be 1.0
Saturation: the default value is corrected to be 1.0

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart March 28, 2022, 10:13 a.m. UTC | #1 
Hi David,

Thank you for the patch.

On Mon, Mar 28, 2022 at 11:05:44AM +0100, David Plowman via libcamera-devel wrote:
> Some of the values were listed incorrectly. Specifically:
> 
> ExposureValue: the range is now centred correctly on zero
> Brightness: the default value (0.0) is made explicit
> Contrast: the default value is corrected to be 1.0
> Saturation: the default value is corrected to be 1.0
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/libcamera/ipa/raspberrypi.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 7f705e49..6a56b008 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -34,13 +34,13 @@ static const ControlInfoMap Controls({
>  		{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>  		{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>  		{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> -		{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> +		{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
>  		{ &controls::AwbEnable, ControlInfo(false, true) },
>  		{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>  		{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> -		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> -		{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> -		{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> +		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> +		{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> +		{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>  		{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>  		{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>  		{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
Kieran Bingham April 2, 2022, 1:21 p.m. UTC | #2 
Quoting David Plowman via libcamera-devel (2022-03-28 11:05:44)
> Some of the values were listed incorrectly. Specifically:
> 
> ExposureValue: the range is now centred correctly on zero
> Brightness: the default value (0.0) is made explicit
> Contrast: the default value is corrected to be 1.0
> Saturation: the default value is corrected to be 1.0
> 

Looks reasonable to me - but I think a review from Naush is probably
more important than me.

Either way:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 7f705e49..6a56b008 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -34,13 +34,13 @@ static const ControlInfoMap Controls({
>                 { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>                 { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>                 { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> -               { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> +               { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
>                 { &controls::AwbEnable, ControlInfo(false, true) },
>                 { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>                 { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> -               { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> -               { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> +               { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> +               { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> +               { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>                 { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>                 { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>                 { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> -- 
> 2.30.2
>
Laurent Pinchart April 5, 2022, 11:12 p.m. UTC | #3 
On Sat, Apr 02, 2022 at 02:21:25PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting David Plowman via libcamera-devel (2022-03-28 11:05:44)
> > Some of the values were listed incorrectly. Specifically:
> > 
> > ExposureValue: the range is now centred correctly on zero
> > Brightness: the default value (0.0) is made explicit
> > Contrast: the default value is corrected to be 1.0
> > Saturation: the default value is corrected to be 1.0
> 
> Looks reasonable to me - but I think a review from Naush is probably
> more important than me.

Naush, do you plan to review this ? Otherwise I'll merge it as-is.

> Either way:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 7f705e49..6a56b008 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -34,13 +34,13 @@ static const ControlInfoMap Controls({
> >                 { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> >                 { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> >                 { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > -               { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > +               { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> >                 { &controls::AwbEnable, ControlInfo(false, true) },
> >                 { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> >                 { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > -               { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> > -               { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > +               { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> > +               { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> > +               { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> >                 { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >                 { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> >                 { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
Naushir Patuck April 6, 2022, 6:56 a.m. UTC | #4 
Hi David,

Thank you for your work.

On Mon, 28 Mar 2022 at 11:06, David Plowman via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> Some of the values were listed incorrectly. Specifically:
>
> ExposureValue: the range is now centred correctly on zero
> Brightness: the default value (0.0) is made explicit
> Contrast: the default value is corrected to be 1.0
> Saturation: the default value is corrected to be 1.0
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> ---
>  include/libcamera/ipa/raspberrypi.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> index 7f705e49..6a56b008 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -34,13 +34,13 @@ static const ControlInfoMap Controls({
>                 { &controls::AeMeteringMode,
> ControlInfo(controls::AeMeteringModeValues) },
>                 { &controls::AeConstraintMode,
> ControlInfo(controls::AeConstraintModeValues) },
>                 { &controls::AeExposureMode,
> ControlInfo(controls::AeExposureModeValues) },
> -               { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> +               { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f)
> },
>                 { &controls::AwbEnable, ControlInfo(false, true) },
>                 { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>                 { &controls::AwbMode, ControlInfo(controls::AwbModeValues)
> },
> -               { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> -               { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> +               { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> +               { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> +               { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>                 { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>                 { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f,
> 16.0f) },
>                 { &controls::ScalerCrop, ControlInfo(Rectangle{},
> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> --
> 2.30.2
>
>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index 7f705e49..6a56b008 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -34,13 +34,13 @@  static const ControlInfoMap Controls({
 		{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
 		{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
 		{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
-		{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
+		{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
 		{ &controls::AwbEnable, ControlInfo(false, true) },
 		{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
 		{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
-		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
-		{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },
-		{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
+		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
+		{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
+		{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
 		{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
 		{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
 		{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },