[libcamera-devel] fix ControlInfo for Span Controls
Christian Rauch
Rauch.Christian at gmx.de
Thu Mar 31 01:14:30 CEST 2022
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 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).
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 {
>>
>
More information about the libcamera-devel
mailing list