[libcamera-devel] fix ControlInfo for Span Controls
Christian Rauch
Rauch.Christian at gmx.de
Mon Mar 21 15:24:25 CET 2022
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 at 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 at 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 {
>>
>
More information about the libcamera-devel
mailing list