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