[libcamera-devel] fix ControlInfo for Span Controls

15477 diff mbox series

Hello,

The attached patch fixes the ControlInfo access of Span Control (see
https://bugs.libcamera.org/show_bug.cgi?id=101).

The ControlInfo for Spans was defined in scalar values (i.e. single
values instead of arrays). Thus, while a Control was defined as Span,
its corresponding ControlInfo was defined as scalar, causing access errors.

Best,
Christian

Hi Christian,
   thanks for the patch.

Would you mind re-sending the patch with git send-email instead of
attaching it, otherwise it would be hard for us to review and reply.

Thanks
   j

On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:
> Hello,
>
> The attached patch fixes the ControlInfo access of Span Control (see
> https://bugs.libcamera.org/show_bug.cgi?id=101).
>
> The ControlInfo for Spans was defined in scalar values (i.e. single
> values instead of arrays). Thus, while a Control was defined as Span,
> its corresponding ControlInfo was defined as scalar, causing access errors.
>
> Best,
> Christian

Hi Christian,

Thank you for the patch, and sorry for the late reply.

As Jacopo mentioned, patches should be sent inline and not as
attachments, but I can still review this version.

On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:
> Hello,
> 
> The attached patch fixes the ControlInfo access of Span Control (see
> https://bugs.libcamera.org/show_bug.cgi?id=101).
> 
> The ControlInfo for Spans was defined in scalar values (i.e. single
> values instead of arrays). Thus, while a Control was defined as Span,
> its corresponding ControlInfo was defined as scalar, causing access errors.
> 
> Best,
> Christian

> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001
> From: Christian Rauch <Rauch.Christian@gmx.de>
> Date: Thu, 17 Mar 2022 01:58:10 +0000
> Subject: [PATCH 1/3] fix ControlInfo for Span Controls
> 
> 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) },

The change in indentation makes the patch difficult to review, as it
mexes indentiation changes with other changes. If you think the
formatting is wrong, please send a patch to fix that first, without any
functional change, and a second patch with the functional changes. Both
can be sent as part of the same series (git format-patch --cover-letter
helps creating a series with a cover letter).

> +	  { &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 }),

We have a first issue here, if we were to define an array control that
had, let's say, 256 elements, initialization would be very inconvenient.
We probably need a bit of a better syntax for the common case where all
values are equal. I also think it would be nice to avoid the Span cast,
possibly by using initializer lists instead, but I'm not entirely sure
how that would look like, and if it's actually doable.

There's also the question of how to handle arrays with a variable number
of elements (although I don't think we have use cases for that yet, and
maybe we never will).

The second issue, which is probably more important, is how we define
minimum, default and maximum values for array controls. At the moment,
we report those values for array elements, not for the whole array. Your
patch shifts that to covering the whole array. For the default value,
this allows expressing defaults where all elements are not identical,
which is very nice. For the minimum and maximum values, however, it's a
bit more complicated.

How do we define minimum and maximum values for an array ? Is it per
element, or do we introduce a concept of minimum and maximum values for
the array as a whole ? The latter is likely quite ill-defined, but the
former isn't without issues either. If the minimum and maximum values
for each element are taken in isolation, the array minimum and maximum
in the ControlInfo may not be valid values for the control. For
instance, we could have an array control with two elements, where each
element can be between -16 and 16, but with a requirement that the sum
of the elements is always equal to 0. The minimum and maximum, could
then be { -16, -16 } and { 16, 16 }, but neither would be value values
for the control.

This is likely unsolvable in a generic manner, so I think we need to
focus on the main use case(s), and see what applications using this API
need. The documentation for ControlInfo should be updated accordingly,
to clarify what minimum, default and maximum mean for array controls.

> +					       } },
> +	  { &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 {
>

Hello,

Sorry, I am not familiar with sending patches via mailing-lists. I
usually use web frontends like GitLab or GitHub. I tried to configure my
system as described in the "PulseAudio How to Use git send-email" [1]
but it fails with "Unable to initialize SMTP properly.". The libcamera
doc at [2] does not provide much information on how to submit patches.

I just copy&pasted the patch file content at the end of my message. This
may look broken because of automatic line breaks.

In the future, will there be a way to contribute to libcamrea via a web
frontend? I've seen pull-requests sent to a GitHub project [3] that got
reviewed there.

Best,
Christian

[1]
https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/
[2] https://libcamera.org/contributing.html
[3] https://github.com/kbingham/libcamera



----------------------------------------------------------------------

From d2e7eec801a0c77bd2b411a0b8b4c5bbe112da99 Mon Sep 17 00:00:00 2001

From: Christian Rauch <Rauch.Christian@gmx.de>

Date: Thu, 17 Mar 2022 01:58:10 +0000

Subject: [PATCH] fix ControlInfo for Span Controls



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




Am 21.03.22 um 13:47 schrieb Laurent Pinchart:
> Hi Christian,
>
> Thank you for the patch, and sorry for the late reply.
>
> As Jacopo mentioned, patches should be sent inline and not as
> attachments, but I can still review this version.
>
> On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:
>> Hello,
>>
>> The attached patch fixes the ControlInfo access of Span Control (see
>> https://bugs.libcamera.org/show_bug.cgi?id=101).
>>
>> The ControlInfo for Spans was defined in scalar values (i.e. single
>> values instead of arrays). Thus, while a Control was defined as Span,
>> its corresponding ControlInfo was defined as scalar, causing access errors.
>>
>> Best,
>> Christian
>
>> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001
>> From: Christian Rauch <Rauch.Christian@gmx.de>
>> Date: Thu, 17 Mar 2022 01:58:10 +0000
>> Subject: [PATCH 1/3] fix ControlInfo for Span Controls
>>
>> 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) },
>
> The change in indentation makes the patch difficult to review, as it
> mexes indentiation changes with other changes. If you think the
> formatting is wrong, please send a patch to fix that first, without any
> functional change, and a second patch with the functional changes. Both
> can be sent as part of the same series (git format-patch --cover-letter
> helps creating a series with a cover letter).
>
>> +	  { &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 }),
>
> We have a first issue here, if we were to define an array control that
> had, let's say, 256 elements, initialization would be very inconvenient.
> We probably need a bit of a better syntax for the common case where all
> values are equal. I also think it would be nice to avoid the Span cast,
> possibly by using initializer lists instead, but I'm not entirely sure
> how that would look like, and if it's actually doable.
>
> There's also the question of how to handle arrays with a variable number
> of elements (although I don't think we have use cases for that yet, and
> maybe we never will).
>
> The second issue, which is probably more important, is how we define
> minimum, default and maximum values for array controls. At the moment,
> we report those values for array elements, not for the whole array. Your
> patch shifts that to covering the whole array. For the default value,
> this allows expressing defaults where all elements are not identical,
> which is very nice. For the minimum and maximum values, however, it's a
> bit more complicated.
>
> How do we define minimum and maximum values for an array ? Is it per
> element, or do we introduce a concept of minimum and maximum values for
> the array as a whole ? The latter is likely quite ill-defined, but the
> former isn't without issues either. If the minimum and maximum values
> for each element are taken in isolation, the array minimum and maximum
> in the ControlInfo may not be valid values for the control. For
> instance, we could have an array control with two elements, where each
> element can be between -16 and 16, but with a requirement that the sum
> of the elements is always equal to 0. The minimum and maximum, could
> then be { -16, -16 } and { 16, 16 }, but neither would be value values
> for the control.
>
> This is likely unsolvable in a generic manner, so I think we need to
> focus on the main use case(s), and see what applications using this API
> need. The documentation for ControlInfo should be updated accordingly,
> to clarify what minimum, default and maximum mean for array controls.
>
>> +					       } },
>> +	  { &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 {
>>
>

Hi Christian,

On Mon, Mar 21, 2022 at 02:24:25PM +0000, Christian Rauch wrote:
> Hello,
> 
> Sorry, I am not familiar with sending patches via mailing-lists. I
> usually use web frontends like GitLab or GitHub. I tried to configure my
> system as described in the "PulseAudio How to Use git send-email" [1]
> but it fails with "Unable to initialize SMTP properly.". The libcamera
> doc at [2] does not provide much information on how to submit patches.

Would this help
https://stackoverflow.com/questions/68238912/how-to-configure-and-use-git-send-email-to-work-with-gmail-to-email-patches-to
? If it works, we should update [2] with additional instructions, as
gmail is quite common.

> I just copy&pasted the patch file content at the end of my message. This
> may look broken because of automatic line breaks.

Yes, that's not great, copy&paste will most likely result in broken
patches with most e-mail clients (and especially the gmail web UI).

> In the future, will there be a way to contribute to libcamrea via a web
> frontend? I've seen pull-requests sent to a GitHub project [3] that got
> reviewed there.

That's not planned at this point. Submission through a web UI would be
nice, but it would need to end up on the mailing list for review. The
git..b review web UIs are barely more than a joke I'm afraid.

It would be nice to have a service to automate submission of patches, by
pushing to a git tree, but if we made that open without authentication
there would be a risk of abuse (and spamming in particular). I don't
know if it's possible to have a repository on gitlab where any
authenticated user could push, with a script that would then posted
patches to the mailing list. If someone likes playing with gitlab, it
could be interesting to develop that.

> [1] https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/
> [2] https://libcamera.org/contributing.html
> [3] https://github.com/kbingham/libcamera

Dear Laurent,

Yes, that worked. Thanks. It turned out that I was just using the wrong
port. I adapted the configuration from Thunderbird, which uses SSL/TLS
port 465. For "git send-email" I had to use 587.

Adding information about how to configure and use "git send-email" to
the contribution documentation would help a lot for new contributors
that are not familiar with this way of contributing patches.

Thanks,
Christian


Am 21.03.22 um 14:47 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Mon, Mar 21, 2022 at 02:24:25PM +0000, Christian Rauch wrote:
>> Hello,
>>
>> Sorry, I am not familiar with sending patches via mailing-lists. I
>> usually use web frontends like GitLab or GitHub. I tried to configure my
>> system as described in the "PulseAudio How to Use git send-email" [1]
>> but it fails with "Unable to initialize SMTP properly.". The libcamera
>> doc at [2] does not provide much information on how to submit patches.
>
> Would this help
> https://stackoverflow.com/questions/68238912/how-to-configure-and-use-git-send-email-to-work-with-gmail-to-email-patches-to
> ? If it works, we should update [2] with additional instructions, as
> gmail is quite common.
>
>> I just copy&pasted the patch file content at the end of my message. This
>> may look broken because of automatic line breaks.
>
> Yes, that's not great, copy&paste will most likely result in broken
> patches with most e-mail clients (and especially the gmail web UI).
>
>> In the future, will there be a way to contribute to libcamrea via a web
>> frontend? I've seen pull-requests sent to a GitHub project [3] that got
>> reviewed there.
>
> That's not planned at this point. Submission through a web UI would be
> nice, but it would need to end up on the mailing list for review. The
> git..b review web UIs are barely more than a joke I'm afraid.
>
> It would be nice to have a service to automate submission of patches, by
> pushing to a git tree, but if we made that open without authentication
> there would be a risk of abuse (and spamming in particular). I don't
> know if it's possible to have a repository on gitlab where any
> authenticated user could push, with a script that would then posted
> patches to the mailing list. If someone likes playing with gitlab, it
> could be interesting to develop that.
>
>> [1] https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/
>> [2] https://libcamera.org/contributing.html
>> [3] https://github.com/kbingham/libcamera
>

Dear Laurent,

I initially overlooked your review of my patch because of the inline
response.

There are a lot of questions raised about the definition and
interpretation of the limits and default values in the ControlInfo.

The main purpose of this patch is to fully define the ControlInfo with
the same type and dimensionality as the Control it is defined for. The
reason for this is that otherwise, there is no information about the
dimensionality of a Control.

I am going to answer your remaining questions inline below.


Am 21.03.22 um 13:47 schrieb Laurent Pinchart:
> Hi Christian,
>
> Thank you for the patch, and sorry for the late reply.
>
> As Jacopo mentioned, patches should be sent inline and not as
> attachments, but I can still review this version.
>
> On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:
>> Hello,
>>
>> The attached patch fixes the ControlInfo access of Span Control (see
>> https://bugs.libcamera.org/show_bug.cgi?id=101).
>>
>> The ControlInfo for Spans was defined in scalar values (i.e. single
>> values instead of arrays). Thus, while a Control was defined as Span,
>> its corresponding ControlInfo was defined as scalar, causing access errors.
>>
>> Best,
>> Christian
>
>> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001
>> From: Christian Rauch <Rauch.Christian@gmx.de>
>> Date: Thu, 17 Mar 2022 01:58:10 +0000
>> Subject: [PATCH 1/3] fix ControlInfo for Span Controls
>>
>> 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) },
>
> The change in indentation makes the patch difficult to review, as it
> mexes indentiation changes with other changes. If you think the
> formatting is wrong, please send a patch to fix that first, without any
> functional change, and a second patch with the functional changes. Both
> can be sent as part of the same series (git format-patch --cover-letter
> helps creating a series with a cover letter).

Parts of this change is caused by the applied .clang-format style. I
will split those changes once I send a new version of the patches.

>
>> +	  { &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 }),
>
> We have a first issue here, if we were to define an array control that
> had, let's say, 256 elements, initialization would be very inconvenient.
> We probably need a bit of a better syntax for the common case where all
> values are equal. I also think it would be nice to avoid the Span cast,
> possibly by using initializer lists instead, but I'm not entirely sure
> how that would look like, and if it's actually doable.

To initialise a large Span with the same values, we could implement a
function that just creates a Span given a length and a value.
Initialising a ControlInfo via an initialiser list would still require
explicit casts. We should avoid casting every element like:
ControlInfo({int(1), int(2), int(3), ...}, {...}, {...}).

>
> There's also the question of how to handle arrays with a variable number
> of elements (although I don't think we have use cases for that yet, and
> maybe we never will).

All those Spans are already variable-sized. They do not have a fixed
size. This is the main reason why I propose to initialise them with the
correct length. Otherwise, there is no user-visible information about
the dimensions of these spans. Information about the length of Spans is
only directly available via the "control_ids.yaml", but this information
is not used by the "gen-controls.py" script, and implicitly by reading
through the code.

>
> The second issue, which is probably more important, is how we define
> minimum, default and maximum values for array controls. At the moment,
> we report those values for array elements, not for the whole array. Your
> patch shifts that to covering the whole array. For the default value,
> this allows expressing defaults where all elements are not identical,
> which is very nice. For the minimum and maximum values, however, it's a
> bit more complicated.

While using scalar values makes the ControlInfo definition simpler, it
is not well defined and loses a lot of its meaning.

Examples:
- The 3x3 ColourCorrectionMatrix by default should be an identity
mapping that does not apply a correction. But what does this mean in
terms of a scalar value? Would this be 0 (as in no mapping) or 1 (as in
identity)? Defining this as a 3x3 identity matrix makes this crystal
clear without any ambiguous formulation.
- The FrameDurationLimits stores a minimum and maximum frame duration
(i.e. the "FrameDuration min/max") between which a sensor can choose its
frame duration from. Those two values should not have the same
"ControlInfo min/max" limits as this would allow setting the
FrameDurationLimits min and max to the same values.

Also semantically, I think that the value range of a control property
should have the same dimensionality as the control property itself.

>
> How do we define minimum and maximum values for an array ? Is it per
> element, or do we introduce a concept of minimum and maximum values for
> the array as a whole ? The latter is likely quite ill-defined, but the
> former isn't without issues either. If the minimum and maximum values
> for each element are taken in isolation, the array minimum and maximum
> in the ControlInfo may not be valid values for the control. For
> instance, we could have an array control with two elements, where each
> element can be between -16 and 16, but with a requirement that the sum
> of the elements is always equal to 0. The minimum and maximum, could
> then be { -16, -16 } and { 16, 16 }, but neither would be value values
> for the control.

This case ("sum of elements is equal to 0") could also not be handled by
the current definitions. Those cases that cannot be expressed by lower
and upper bounds must be handled "somewhere" else. There are already
configurations, such as setting an "ExposureValue" while
"AeExposureMode" is on, which cannot be handled by the current API. If
this invalid configuration state happens, the request is currently just
cancelled.

>
> This is likely unsolvable in a generic manner, so I think we need to
> focus on the main use case(s), and see what applications using this API
> need. The documentation for ControlInfo should be updated accordingly,
> to clarify what minimum, default and maximum mean for array controls.

I agree that a lot of those questions cannot be answered right now, and
they are definitely out-of-scope of this patch.

>
>> +					       } },
>> +	  { &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 {
>>
>

Hi Christian,

On Thu, Mar 31, 2022 at 12:14:30AM +0100, Christian Rauch wrote:
> Dear Laurent,
> 
> I initially overlooked your review of my patch because of the inline
> response.

No worries :-)

> There are a lot of questions raised about the definition and
> interpretation of the limits and default values in the ControlInfo.
> 
> The main purpose of this patch is to fully define the ControlInfo with
> the same type and dimensionality as the Control it is defined for. The
> reason for this is that otherwise, there is no information about the
> dimensionality of a Control.
> 
> I am going to answer your remaining questions inline below.
> 
> Am 21.03.22 um 13:47 schrieb Laurent Pinchart:
> > Hi Christian,
> >
> > Thank you for the patch, and sorry for the late reply.
> >
> > As Jacopo mentioned, patches should be sent inline and not as
> > attachments, but I can still review this version.
> >
> > On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:
> >> Hello,
> >>
> >> The attached patch fixes the ControlInfo access of Span Control (see
> >> https://bugs.libcamera.org/show_bug.cgi?id=101).
> >>
> >> The ControlInfo for Spans was defined in scalar values (i.e. single
> >> values instead of arrays). Thus, while a Control was defined as Span,
> >> its corresponding ControlInfo was defined as scalar, causing access errors.
> >>
> >> Best,
> >> Christian
> >
> >> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001
> >> From: Christian Rauch <Rauch.Christian@gmx.de>
> >> Date: Thu, 17 Mar 2022 01:58:10 +0000
> >> Subject: [PATCH 1/3] fix ControlInfo for Span Controls
> >>
> >> 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) },
> >
> > The change in indentation makes the patch difficult to review, as it
> > mexes indentiation changes with other changes. If you think the
> > formatting is wrong, please send a patch to fix that first, without any
> > functional change, and a second patch with the functional changes. Both
> > can be sent as part of the same series (git format-patch --cover-letter
> > helps creating a series with a cover letter).
> 
> Parts of this change is caused by the applied .clang-format style. I
> will split those changes once I send a new version of the patches.

Please note that the .clang-format file tries to match our preferred
coding style as closely as possibly, but that's not a 100% match. There
are exceptions that improve readability (at least in our opinion) and
can't be expressed in .clang-format (or we haven't found a way to do
so). The output of checkstyle.py is thus informative, it doesn't have to
be followed 100% (especially when refactoring existing code, already
existing departure from what clang-format produces are likely
intentional).

> >> +	  { &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 }),
> >
> > We have a first issue here, if we were to define an array control that
> > had, let's say, 256 elements, initialization would be very inconvenient.
> > We probably need a bit of a better syntax for the common case where all
> > values are equal. I also think it would be nice to avoid the Span cast,
> > possibly by using initializer lists instead, but I'm not entirely sure
> > how that would look like, and if it's actually doable.
> 
> To initialise a large Span with the same values, we could implement a
> function that just creates a Span given a length and a value.

That could possibly be a specialized ControlInfo constructor, but I'm
not sure it's possible to express all of our needs as different
constructor overloads that could be resolved without ambiguity. A
separate function could be an option too, but I'm not sure where it
should then live, as I'd like to avoid departing from the std::span API
for the Span class.

> Initialising a ControlInfo via an initialiser list would still require
> explicit casts. We should avoid casting every element like:
> ControlInfo({int(1), int(2), int(3), ...}, {...}, {...}).

That would look awful indeed.

> > There's also the question of how to handle arrays with a variable number
> > of elements (although I don't think we have use cases for that yet, and
> > maybe we never will).
> 
> All those Spans are already variable-sized. They do not have a fixed
> size. This is the main reason why I propose to initialise them with the
> correct length. Otherwise, there is no user-visible information about
> the dimensions of these spans. Information about the length of Spans is
> only directly available via the "control_ids.yaml", but this information
> is not used by the "gen-controls.py" script, and implicitly by reading
> through the code.

That's right, we don't expose that information programmatically. That's
mostly because we haven't had any need to do so yet :-) I'm certainly
interested in fixing that (thanks for your patch series that starts
working in that direction), although I'm not entirely sure what the use
cases are. If the only tangible result is that compile-time type
checking is improved, that's already a win.

> > The second issue, which is probably more important, is how we define
> > minimum, default and maximum values for array controls. At the moment,
> > we report those values for array elements, not for the whole array. Your
> > patch shifts that to covering the whole array. For the default value,
> > this allows expressing defaults where all elements are not identical,
> > which is very nice. For the minimum and maximum values, however, it's a
> > bit more complicated.
> 
> While using scalar values makes the ControlInfo definition simpler, it
> is not well defined and loses a lot of its meaning.
> 
> Examples:
> - The 3x3 ColourCorrectionMatrix by default should be an identity
> mapping that does not apply a correction. But what does this mean in
> terms of a scalar value? Would this be 0 (as in no mapping) or 1 (as in
> identity)? Defining this as a 3x3 identity matrix makes this crystal
> clear without any ambiguous formulation.

I fully agree with you that, for default values, having a full array has
very interesting use cases (such as the one you mention here). It will
increase memory consumption for large arrays, but as long as we don't
copy ControlInfo instances, that's fine.

> - The FrameDurationLimits stores a minimum and maximum frame duration
> (i.e. the "FrameDuration min/max") between which a sensor can choose its
> frame duration from. Those two values should not have the same
> "ControlInfo min/max" limits as this would allow setting the
> FrameDurationLimits min and max to the same values.

Actually that's useful, to express fixed frame rates.

> Also semantically, I think that the value range of a control property
> should have the same dimensionality as the control property itself.

I don't disagree. I still think minimum and maximum are quite
ill-defined in that case (as discussed below), but they're also
ill-defined when expressed as scalars.

> > How do we define minimum and maximum values for an array ? Is it per
> > element, or do we introduce a concept of minimum and maximum values for
> > the array as a whole ? The latter is likely quite ill-defined, but the
> > former isn't without issues either. If the minimum and maximum values
> > for each element are taken in isolation, the array minimum and maximum
> > in the ControlInfo may not be valid values for the control. For
> > instance, we could have an array control with two elements, where each
> > element can be between -16 and 16, but with a requirement that the sum
> > of the elements is always equal to 0. The minimum and maximum, could
> > then be { -16, -16 } and { 16, 16 }, but neither would be value values
> > for the control.
> 
> This case ("sum of elements is equal to 0") could also not be handled by
> the current definitions. Those cases that cannot be expressed by lower
> and upper bounds must be handled "somewhere" else. There are already
> configurations, such as setting an "ExposureValue" while
> "AeExposureMode" is on, which cannot be handled by the current API. If
> this invalid configuration state happens, the request is currently just
> cancelled.
> 
> > This is likely unsolvable in a generic manner, so I think we need to
> > focus on the main use case(s), and see what applications using this API
> > need. The documentation for ControlInfo should be updated accordingly,
> > to clarify what minimum, default and maximum mean for array controls.
> 
> I agree that a lot of those questions cannot be answered right now, and
> they are definitely out-of-scope of this patch.

We seem to agree, good :-)

> >> +					       } },
> >> +	  { &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 {
> >>

Hi Laurent,

To give you a bit of background on my two patch sets:

I am interested in fully specifying the Control dimensions via the
user-visible API to present all Controls in a generic way to a user. I
could hard code the dimensions of control on my side, but this would not
scale if new controls are added or the control types change.
Essentially, I want to iterate over the "Camera::controls()" and present
all controls to a user. In order to let a user choose values and set
them on the camera, I have to know the full control type (scalar type,
Span, size). Currently, this information is not available via the API.

I also left a couple of inline comments below.


Am 05.04.22 um 18:17 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Thu, Mar 31, 2022 at 12:14:30AM +0100, Christian Rauch wrote:
>> Dear Laurent,
>>
>> I initially overlooked your review of my patch because of the inline
>> response.
>
> No worries :-)
>
>> There are a lot of questions raised about the definition and
>> interpretation of the limits and default values in the ControlInfo.
>>
>> The main purpose of this patch is to fully define the ControlInfo with
>> the same type and dimensionality as the Control it is defined for. The
>> reason for this is that otherwise, there is no information about the
>> dimensionality of a Control.
>>
>> I am going to answer your remaining questions inline below.
>>
>> Am 21.03.22 um 13:47 schrieb Laurent Pinchart:
>>> Hi Christian,
>>>
>>> Thank you for the patch, and sorry for the late reply.
>>>
>>> As Jacopo mentioned, patches should be sent inline and not as
>>> attachments, but I can still review this version.
>>>
>>> On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:
>>>> Hello,
>>>>
>>>> The attached patch fixes the ControlInfo access of Span Control (see
>>>> https://bugs.libcamera.org/show_bug.cgi?id=101).
>>>>
>>>> The ControlInfo for Spans was defined in scalar values (i.e. single
>>>> values instead of arrays). Thus, while a Control was defined as Span,
>>>> its corresponding ControlInfo was defined as scalar, causing access errors.
>>>>
>>>> Best,
>>>> Christian
>>>
>>>> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001
>>>> From: Christian Rauch <Rauch.Christian@gmx.de>
>>>> Date: Thu, 17 Mar 2022 01:58:10 +0000
>>>> Subject: [PATCH 1/3] fix ControlInfo for Span Controls
>>>>
>>>> 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) },
>>>
>>> The change in indentation makes the patch difficult to review, as it
>>> mexes indentiation changes with other changes. If you think the
>>> formatting is wrong, please send a patch to fix that first, without any
>>> functional change, and a second patch with the functional changes. Both
>>> can be sent as part of the same series (git format-patch --cover-letter
>>> helps creating a series with a cover letter).
>>
>> Parts of this change is caused by the applied .clang-format style. I
>> will split those changes once I send a new version of the patches.
>
> Please note that the .clang-format file tries to match our preferred
> coding style as closely as possibly, but that's not a 100% match. There
> are exceptions that improve readability (at least in our opinion) and
> can't be expressed in .clang-format (or we haven't found a way to do
> so). The output of checkstyle.py is thus informative, it doesn't have to
> be followed 100% (especially when refactoring existing code, already
> existing departure from what clang-format produces are likely
> intentional).

I am going to drop these changes in a future version of this patch.

>
>>>> +	  { &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 }),
>>>
>>> We have a first issue here, if we were to define an array control that
>>> had, let's say, 256 elements, initialization would be very inconvenient.
>>> We probably need a bit of a better syntax for the common case where all
>>> values are equal. I also think it would be nice to avoid the Span cast,
>>> possibly by using initializer lists instead, but I'm not entirely sure
>>> how that would look like, and if it's actually doable.
>>
>> To initialise a large Span with the same values, we could implement a
>> function that just creates a Span given a length and a value.
>
> That could possibly be a specialized ControlInfo constructor, but I'm
> not sure it's possible to express all of our needs as different
> constructor overloads that could be resolved without ambiguity. A
> separate function could be an option too, but I'm not sure where it
> should then live, as I'd like to avoid departing from the std::span API
> for the Span class.

I think the "std::span" class also works with "std::array". A custom
function could just create a std::array<T,N> with identical values.

>
>> Initialising a ControlInfo via an initialiser list would still require
>> explicit casts. We should avoid casting every element like:
>> ControlInfo({int(1), int(2), int(3), ...}, {...}, {...}).
>
> That would look awful indeed.
>
>>> There's also the question of how to handle arrays with a variable number
>>> of elements (although I don't think we have use cases for that yet, and
>>> maybe we never will).
>>
>> All those Spans are already variable-sized. They do not have a fixed
>> size. This is the main reason why I propose to initialise them with the
>> correct length. Otherwise, there is no user-visible information about
>> the dimensions of these spans. Information about the length of Spans is
>> only directly available via the "control_ids.yaml", but this information
>> is not used by the "gen-controls.py" script, and implicitly by reading
>> through the code.
>
> That's right, we don't expose that information programmatically. That's
> mostly because we haven't had any need to do so yet :-) I'm certainly
> interested in fixing that (thanks for your patch series that starts
> working in that direction), although I'm not entirely sure what the use
> cases are. If the only tangible result is that compile-time type
> checking is improved, that's already a win.

Setting the ControlInfo with the full type (Span + size) is one way to
inform the user about the expected Control dimensions. I am not sure
those can be checked at compile time. For a compile-time check, my other
patch series is required.

>
>>> The second issue, which is probably more important, is how we define
>>> minimum, default and maximum values for array controls. At the moment,
>>> we report those values for array elements, not for the whole array. Your
>>> patch shifts that to covering the whole array. For the default value,
>>> this allows expressing defaults where all elements are not identical,
>>> which is very nice. For the minimum and maximum values, however, it's a
>>> bit more complicated.
>>
>> While using scalar values makes the ControlInfo definition simpler, it
>> is not well defined and loses a lot of its meaning.
>>
>> Examples:
>> - The 3x3 ColourCorrectionMatrix by default should be an identity
>> mapping that does not apply a correction. But what does this mean in
>> terms of a scalar value? Would this be 0 (as in no mapping) or 1 (as in
>> identity)? Defining this as a 3x3 identity matrix makes this crystal
>> clear without any ambiguous formulation.
>
> I fully agree with you that, for default values, having a full array has
> very interesting use cases (such as the one you mention here). It will
> increase memory consumption for large arrays, but as long as we don't
> copy ControlInfo instances, that's fine.
>
>> - The FrameDurationLimits stores a minimum and maximum frame duration
>> (i.e. the "FrameDuration min/max") between which a sensor can choose its
>> frame duration from. Those two values should not have the same
>> "ControlInfo min/max" limits as this would allow setting the
>> FrameDurationLimits min and max to the same values.
>
> Actually that's useful, to express fixed frame rates.

Setting the limits to the same value will definitely force any "auto
mode" to select the specified frame duration. But for this specific use
case, there is already the "FrameDuration" control. It's a bit
confusing, but my interpreation is now that the min/max limits of
controls are for users, e.g. a user can chose a fixed "FrameDuration"
within the controls min/max bounds, but the "FrameDurationLimits" are
user chosen bounds another algorithm (e.g. auto exposure) can chose
from, where the min/max bounds define the "physical" sensor limits.

>
>> Also semantically, I think that the value range of a control property
>> should have the same dimensionality as the control property itself.
>
> I don't disagree. I still think minimum and maximum are quite
> ill-defined in that case (as discussed below), but they're also
> ill-defined when expressed as scalars.
>
>>> How do we define minimum and maximum values for an array ? Is it per
>>> element, or do we introduce a concept of minimum and maximum values for
>>> the array as a whole ? The latter is likely quite ill-defined, but the
>>> former isn't without issues either. If the minimum and maximum values
>>> for each element are taken in isolation, the array minimum and maximum
>>> in the ControlInfo may not be valid values for the control. For
>>> instance, we could have an array control with two elements, where each
>>> element can be between -16 and 16, but with a requirement that the sum
>>> of the elements is always equal to 0. The minimum and maximum, could
>>> then be { -16, -16 } and { 16, 16 }, but neither would be value values
>>> for the control.
>>
>> This case ("sum of elements is equal to 0") could also not be handled by
>> the current definitions. Those cases that cannot be expressed by lower
>> and upper bounds must be handled "somewhere" else. There are already
>> configurations, such as setting an "ExposureValue" while
>> "AeExposureMode" is on, which cannot be handled by the current API. If
>> this invalid configuration state happens, the request is currently just
>> cancelled.
>>
>>> This is likely unsolvable in a generic manner, so I think we need to
>>> focus on the main use case(s), and see what applications using this API
>>> need. The documentation for ControlInfo should be updated accordingly,
>>> to clarify what minimum, default and maximum mean for array controls.
>>
>> I agree that a lot of those questions cannot be answered right now, and
>> they are definitely out-of-scope of this patch.
>
> We seem to agree, good :-)
>
>>>> +					       } },
>>>> +	  { &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 {
>>>>
>