[libcamera-devel] [PATCH v3 3/4] apply explicit fixed-sized Span type casts
Christian Rauch
Rauch.Christian at gmx.de
Wed Apr 20 00:47:54 CEST 2022
Hi Laurent,
In short: We can skip a lot of casts, if those values would already be
in the "target type".
Am 19.04.22 um 21:46 schrieb Laurent Pinchart:
> Hi Christian,
>
> Thank you for the patch.
>
> On Fri, Apr 08, 2022 at 02:42:30AM +0100, Christian Rauch via libcamera-devel wrote:
>> The change of types of some Controls from variable- to fixed-sized requires
>> explicit casting of FrameDurationLimits, ColourGains and SensorBlackLevels.
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
>> ---
>> src/ipa/raspberrypi/raspberrypi.cpp | 18 +++++++++---------
>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-
>> src/qcam/dng_writer.cpp | 6 +++---
>> 3 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 89767a9d..5a5cdf66 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -500,18 +500,18 @@ void IPARPi::reportMetadata()
>>
>> AwbStatus *awbStatus = rpiMetadata_.GetLocked<AwbStatus>("awb.status");
>> if (awbStatus) {
>> - libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gain_r),
>> - static_cast<float>(awbStatus->gain_b) });
>> + libcameraMetadata_.set(controls::ColourGains, Span<const float, 2>({ static_cast<float>(awbStatus->gain_r),
>> + static_cast<float>(awbStatus->gain_b) }));
>
> Those lines are too long. I'm bothered here, as I think it's very nice
> to use fixed-extent spans to catch errors at compilation time, but the
> cast is ugly :-( This is caused by the explicit constructor for
> fixed-extent spans, which we can't change if we want to remain
> compatible with std::span.
>
> I was also wondering it we could drop the static_cast<float> to
> compensate for the additional explicit constructor, but that generates
> other errors:
>
> ../../src/ipa/raspberrypi/raspberrypi.cpp:505:72: error: non-constant-expression cannot be narrowed from type 'double' to 'libcamera::Span<const float, 2>::element_type' (aka 'const float') in initializer list [-Wc++11-narrowing]
> libcameraMetadata_.set(controls::ColourGains, Span<const float, 2>({ awbStatus->gain_r,
> ^~~~~~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/raspberrypi.cpp:505:72: note: insert an explicit cast to silence this issue
> libcameraMetadata_.set(controls::ColourGains, Span<const float, 2>({ awbStatus->gain_r,
> ^~~~~~~~~~~~~~~~~
> static_cast<element_type>( )
> ../../src/ipa/raspberrypi/raspberrypi.cpp:506:16: error: non-constant-expression cannot be narrowed from type 'double' to 'libcamera::Span<const float, 2>::element_type' (aka 'const float') in initializer list [-Wc++11-narrowing]
> awbStatus->gain_b }));
> ^~~~~~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/raspberrypi.cpp:506:16: note: insert an explicit cast to silence this issue
> awbStatus->gain_b }));
> ^~~~~~~~~~~~~~~~~
> static_cast<element_type>( )
> 2 errors generated.
>
> I suppose stars don't always align.
The "AwbStatus" stores its values as "double". If you either change that
struct to "float" or the "ColourGains" Control to "double", then you can
skip the explicit casts. You still have to call the explicit "Span"
constructor.
There are a couple of other places where these "static_cast" are required.
>
> With proper line wraps, such as
>
> libcameraMetadata_.set(controls::ColourGains,
> Span<const float, 2>({ static_cast<float>(awbStatus->gain_r),
> static_cast<float>(awbStatus->gain_b) }));
>
> or maybe
>
> libcameraMetadata_.set(controls::ColourGains, Span<const float, 2>({
> static_cast<float>(awbStatus->gain_r),
> static_cast<float>(awbStatus->gain_b) }));
>
> I'm OK with this patch. I'd like to hear from others though. Bonus
> points if someone can find a nicer way to express all this. We could,
> for instance, add new ControlList::set() overloads specialized for
> spans.
>
>> libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperature_K);
>> }
>>
>> BlackLevelStatus *blackLevelStatus = rpiMetadata_.GetLocked<BlackLevelStatus>("black_level.status");
>> if (blackLevelStatus)
>> libcameraMetadata_.set(controls::SensorBlackLevels,
>> - { static_cast<int32_t>(blackLevelStatus->black_level_r),
>> - static_cast<int32_t>(blackLevelStatus->black_level_g),
>> - static_cast<int32_t>(blackLevelStatus->black_level_g),
>> - static_cast<int32_t>(blackLevelStatus->black_level_b) });
>> + Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->black_level_r),
>> + static_cast<int32_t>(blackLevelStatus->black_level_g),
>> + static_cast<int32_t>(blackLevelStatus->black_level_g),
>> + static_cast<int32_t>(blackLevelStatus->black_level_b) }));
>>
>> FocusStatus *focusStatus = rpiMetadata_.GetLocked<FocusStatus>("focus.status");
>> if (focusStatus && focusStatus->num == 12) {
>> @@ -816,7 +816,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>> if (gains[0] != 0.0f && gains[1] != 0.0f)
>> /* A gain of 0.0f will switch back to auto mode. */
>> libcameraMetadata_.set(controls::ColourGains,
>> - { gains[0], gains[1] });
>> + Span<const float, 2>({ gains[0], gains[1] }));
>> break;
>> }
>>
>> @@ -1100,8 +1100,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
>>
>> /* Return the validated limits via metadata. */
>> libcameraMetadata_.set(controls::FrameDurationLimits,
>> - { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
>> - static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
>> + Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
>> + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }));
>>
>> /*
>> * Calculate the maximum exposure time possible for the AGC to use.
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index d6148724..0fa294d4 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -1696,7 +1696,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
>> */
>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {
>> - libcamera::Span<const float> colourGains = controls.get(libcamera::controls::ColourGains);
>> + libcamera::Span<const float, 2> colourGains = controls.get(libcamera::controls::ColourGains);
>> /* The control wants linear gains in the order B, Gb, Gr, R. */
>> ControlList ctrls(sensor_->controls());
>> std::array<int32_t, 4> gains{
>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
>> index 34c8df5a..2fb527d8 100644
>> --- a/src/qcam/dng_writer.cpp
>> +++ b/src/qcam/dng_writer.cpp
>> @@ -438,7 +438,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> const double eps = 1e-2;
>>
>> if (metadata.contains(controls::ColourGains)) {
>> - Span<const float> const &colourGains = metadata.get(controls::ColourGains);
>> + Span<const float, 2> const &colourGains = metadata.get(controls::ColourGains);
>> if (colourGains[0] > eps && colourGains[1] > eps) {
>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
>> neutral[0] = 1.0 / colourGains[0]; /* red */
>> @@ -446,7 +446,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> }
>> }
>> if (metadata.contains(controls::ColourCorrectionMatrix)) {
>> - Span<const float> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
>> + Span<const float, 9> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
>> Matrix3d ccmSupplied(coeffs);
>> if (ccmSupplied.determinant() > eps)
>> ccm = ccmSupplied;
>> @@ -515,7 +515,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;
>>
>> if (metadata.contains(controls::SensorBlackLevels)) {
>> - Span<const int32_t> levels = metadata.get(controls::SensorBlackLevels);
>> + Span<const int32_t, 4> levels = metadata.get(controls::SensorBlackLevels);
>>
>> /*
>> * The black levels control is specified in R, Gr, Gb, B order.
>
More information about the libcamera-devel
mailing list