[libcamera-devel] fix ControlInfo for Span Controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 21 14:47:19 CET 2022
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 {
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list