[libcamera-devel] fix ControlInfo for Span Controls
diff mbox series

Message ID 20220321230246.663401-1-Rauch.Christian@gmx.de
State New
Headers show
Series
  • [libcamera-devel] fix ControlInfo for Span Controls
Related show

Commit Message

Christian Rauch March 21, 2022, 11:02 p.m. UTC 
Some control properties are typed with a Span to store an array of values.
Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix
and FrameDurationLimits. The value range and defaults in the ControlInfo of
those Controls is currently defined as scalar. This prevents accessing the
ControlInfo via the native Span type.

By defining the ControlInfo in terms of Spans, we can avoid this issue.
---
 include/libcamera/ipa/raspberrypi.h           | 52 ++++++++++++-------
 src/ipa/ipu3/ipu3.cpp                         |  6 +--
 .../ipa_data_serializer_test.cpp              |  8 +--
 3 files changed, 40 insertions(+), 26 deletions(-)

--
2.25.1

Comments

Christian Rauch March 28, 2022, 8:38 p.m. UTC | #1 
Hi,

This patch may have got lost because I was sending it multiple times in
different forms.

Is this something that can be reviewed?

Best,
Christian


Am 21.03.22 um 23:02 schrieb Christian Rauch:
> Some control properties are typed with a Span to store an array of values.
> Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix
> and FrameDurationLimits. The value range and defaults in the ControlInfo of
> those Controls is currently defined as scalar. This prevents accessing the
> ControlInfo via the native Span type.
>
> By defining the ControlInfo in terms of Spans, we can avoid this issue.
> ---
>  include/libcamera/ipa/raspberrypi.h           | 52 ++++++++++++-------
>  src/ipa/ipu3/ipu3.cpp                         |  6 +--
>  .../ipa_data_serializer_test.cpp              |  8 +--
>  3 files changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 7f705e49..fb5188a1 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -27,26 +27,38 @@ namespace RPi {
>   * and the pipeline handler may be reverted so that it aborts when an
>   * unsupported control is encountered.
>   */
> -static const ControlInfoMap Controls({
> -		{ &controls::AeEnable, ControlInfo(false, true) },
> -		{ &controls::ExposureTime, ControlInfo(0, 999999) },
> -		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> -		{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> -		{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> -		{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> -		{ &controls::ExposureValue, ControlInfo(0.0f, 16.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::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{}) },
> -		{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> -		{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> -	}, controls::controls);
> +static const ControlInfoMap Controls(
> +	{ { &controls::AeEnable, ControlInfo(false, true) },
> +	  { &controls::ExposureTime, ControlInfo(0, 999999) },
> +	  { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> +	  { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> +	  { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> +	  { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> +	  { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> +	  { &controls::AwbEnable, ControlInfo(false, true) },
> +	  { &controls::ColourGains, ControlInfo{
> +					    Span<const float>({ 0, 0 }),
> +					    Span<const float>({ 32, 32 }),
> +					    Span<const float>({ 0, 0 }),
> +				    } },
> +	  { &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::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> +	  { &controls::ColourCorrectionMatrix, ControlInfo{
> +						       Span<const float>({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),
> +						       Span<const float>({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),
> +						       Span<const float>({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),
> +					       } },
> +	  { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> +	  { &controls::FrameDurationLimits, ControlInfo{
> +						    Span<const int64_t>({ 1000, 1000 }),
> +						    Span<const int64_t>({ 1000000000, 1000000000 }),
> +						    Span<const int64_t>({ 1000, 1000 }),
> +					    } },
> +	  { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },
> +	controls::controls);
>
>  } /* namespace RPi */
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 1ea2c898..e64fc2bb 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>  		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
>  	}
>
> -	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> -							       frameDurations[1],
> -							       frameDurations[2]);
> +	controls[&controls::FrameDurationLimits] = ControlInfo{ Span<const int64_t>({ frameDurations[0], frameDurations[0] }),
> +								Span<const int64_t>({ frameDurations[1], frameDurations[1] }),
> +								Span<const int64_t>({ frameDurations[2], frameDurations[2] }) };
>
>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>  }
> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> index d2050a86..5503cc8a 100644
> --- a/test/serialization/ipa_data_serializer_test.cpp
> +++ b/test/serialization/ipa_data_serializer_test.cpp
> @@ -32,13 +32,15 @@
>  using namespace std;
>  using namespace libcamera;
>
> -static const ControlInfoMap Controls = ControlInfoMap({
> +static const ControlInfoMap Controls = ControlInfoMap(
> +	{
>  		{ &controls::AeEnable, ControlInfo(false, true) },
>  		{ &controls::ExposureTime, ControlInfo(0, 999999) },
>  		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> -		{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> +		{ &controls::ColourGains, ControlInfo{ Span<const float>({ 0, 0 }), Span<const float>({ 32, 32 }), Span<const float>({ 0, 0 }) } },
>  		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> -	}, controls::controls);
> +	},
> +	controls::controls);
>
>  namespace libcamera {
>
> --
> 2.25.1
>
Jacopo Mondi March 29, 2022, 7:08 a.m. UTC | #2 
Hi Christian

On Mon, Mar 28, 2022 at 09:38:54PM +0100, Christian Rauch via libcamera-devel wrote:
> Hi,
>
> This patch may have got lost because I was sending it multiple times in
> different forms.

Laurent has left review comments on your previous versions:
https://patchwork.libcamera.org/patch/15477/#22343

The most relevant of which is, in my opinion, about how to define what
max min and def are for arrays.

Feel free to reply to the comments on the previous version of the
patch.

Thanks
  j

>
> Is this something that can be reviewed?
>
> Best,
> Christian
>
>
> Am 21.03.22 um 23:02 schrieb Christian Rauch:
> > Some control properties are typed with a Span to store an array of values.
> > Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix
> > and FrameDurationLimits. The value range and defaults in the ControlInfo of
> > those Controls is currently defined as scalar. This prevents accessing the
> > ControlInfo via the native Span type.
> >
> > By defining the ControlInfo in terms of Spans, we can avoid this issue.
> > ---
> >  include/libcamera/ipa/raspberrypi.h           | 52 ++++++++++++-------
> >  src/ipa/ipu3/ipu3.cpp                         |  6 +--
> >  .../ipa_data_serializer_test.cpp              |  8 +--
> >  3 files changed, 40 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 7f705e49..fb5188a1 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -27,26 +27,38 @@ namespace RPi {
> >   * and the pipeline handler may be reverted so that it aborts when an
> >   * unsupported control is encountered.
> >   */
> > -static const ControlInfoMap Controls({
> > -		{ &controls::AeEnable, ControlInfo(false, true) },
> > -		{ &controls::ExposureTime, ControlInfo(0, 999999) },
> > -		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > -		{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > -		{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > -		{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > -		{ &controls::ExposureValue, ControlInfo(0.0f, 16.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::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{}) },
> > -		{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > -		{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> > -	}, controls::controls);
> > +static const ControlInfoMap Controls(
> > +	{ { &controls::AeEnable, ControlInfo(false, true) },
> > +	  { &controls::ExposureTime, ControlInfo(0, 999999) },
> > +	  { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > +	  { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > +	  { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > +	  { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > +	  { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > +	  { &controls::AwbEnable, ControlInfo(false, true) },
> > +	  { &controls::ColourGains, ControlInfo{
> > +					    Span<const float>({ 0, 0 }),
> > +					    Span<const float>({ 32, 32 }),
> > +					    Span<const float>({ 0, 0 }),
> > +				    } },
> > +	  { &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::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > +	  { &controls::ColourCorrectionMatrix, ControlInfo{
> > +						       Span<const float>({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),
> > +						       Span<const float>({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),
> > +						       Span<const float>({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),
> > +					       } },
> > +	  { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > +	  { &controls::FrameDurationLimits, ControlInfo{
> > +						    Span<const int64_t>({ 1000, 1000 }),
> > +						    Span<const int64_t>({ 1000000000, 1000000000 }),
> > +						    Span<const int64_t>({ 1000, 1000 }),
> > +					    } },
> > +	  { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },
> > +	controls::controls);
> >
> >  } /* namespace RPi */
> >
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 1ea2c898..e64fc2bb 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> >  		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> >  	}
> >
> > -	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> > -							       frameDurations[1],
> > -							       frameDurations[2]);
> > +	controls[&controls::FrameDurationLimits] = ControlInfo{ Span<const int64_t>({ frameDurations[0], frameDurations[0] }),
> > +								Span<const int64_t>({ frameDurations[1], frameDurations[1] }),
> > +								Span<const int64_t>({ frameDurations[2], frameDurations[2] }) };
> >
> >  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> >  }
> > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> > index d2050a86..5503cc8a 100644
> > --- a/test/serialization/ipa_data_serializer_test.cpp
> > +++ b/test/serialization/ipa_data_serializer_test.cpp
> > @@ -32,13 +32,15 @@
> >  using namespace std;
> >  using namespace libcamera;
> >
> > -static const ControlInfoMap Controls = ControlInfoMap({
> > +static const ControlInfoMap Controls = ControlInfoMap(
> > +	{
> >  		{ &controls::AeEnable, ControlInfo(false, true) },
> >  		{ &controls::ExposureTime, ControlInfo(0, 999999) },
> >  		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > -		{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > +		{ &controls::ColourGains, ControlInfo{ Span<const float>({ 0, 0 }), Span<const float>({ 32, 32 }), Span<const float>({ 0, 0 }) } },
> >  		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > -	}, controls::controls);
> > +	},
> > +	controls::controls);
> >
> >  namespace libcamera {
> >
> > --
> > 2.25.1
> >

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index 7f705e49..fb5188a1 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -27,26 +27,38 @@  namespace RPi {
  * and the pipeline handler may be reverted so that it aborts when an
  * unsupported control is encountered.
  */
-static const ControlInfoMap Controls({
-		{ &controls::AeEnable, ControlInfo(false, true) },
-		{ &controls::ExposureTime, ControlInfo(0, 999999) },
-		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
-		{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
-		{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
-		{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
-		{ &controls::ExposureValue, ControlInfo(0.0f, 16.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::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{}) },
-		{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
-		{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
-	}, controls::controls);
+static const ControlInfoMap Controls(
+	{ { &controls::AeEnable, ControlInfo(false, true) },
+	  { &controls::ExposureTime, ControlInfo(0, 999999) },
+	  { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
+	  { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
+	  { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
+	  { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
+	  { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
+	  { &controls::AwbEnable, ControlInfo(false, true) },
+	  { &controls::ColourGains, ControlInfo{
+					    Span<const float>({ 0, 0 }),
+					    Span<const float>({ 32, 32 }),
+					    Span<const float>({ 0, 0 }),
+				    } },
+	  { &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::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
+	  { &controls::ColourCorrectionMatrix, ControlInfo{
+						       Span<const float>({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),
+						       Span<const float>({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),
+						       Span<const float>({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),
+					       } },
+	  { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
+	  { &controls::FrameDurationLimits, ControlInfo{
+						    Span<const int64_t>({ 1000, 1000 }),
+						    Span<const int64_t>({ 1000000000, 1000000000 }),
+						    Span<const int64_t>({ 1000, 1000 }),
+					    } },
+	  { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },
+	controls::controls);

 } /* namespace RPi */

diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 1ea2c898..e64fc2bb 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -267,9 +267,9 @@  void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
 		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
 	}

-	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
-							       frameDurations[1],
-							       frameDurations[2]);
+	controls[&controls::FrameDurationLimits] = ControlInfo{ Span<const int64_t>({ frameDurations[0], frameDurations[0] }),
+								Span<const int64_t>({ frameDurations[1], frameDurations[1] }),
+								Span<const int64_t>({ frameDurations[2], frameDurations[2] }) };

 	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
 }
diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
index d2050a86..5503cc8a 100644
--- a/test/serialization/ipa_data_serializer_test.cpp
+++ b/test/serialization/ipa_data_serializer_test.cpp
@@ -32,13 +32,15 @@ 
 using namespace std;
 using namespace libcamera;

-static const ControlInfoMap Controls = ControlInfoMap({
+static const ControlInfoMap Controls = ControlInfoMap(
+	{
 		{ &controls::AeEnable, ControlInfo(false, true) },
 		{ &controls::ExposureTime, ControlInfo(0, 999999) },
 		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
-		{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
+		{ &controls::ColourGains, ControlInfo{ Span<const float>({ 0, 0 }), Span<const float>({ 32, 32 }), Span<const float>({ 0, 0 }) } },
 		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
-	}, controls::controls);
+	},
+	controls::controls);

 namespace libcamera {