[libcamera-devel] [PATCH v5 4/4] libcamera: controls: Apply explicit fixed-sized Span type casts

[Jacopo Mondi]

Hi Christian

On Thu, Jun 02, 2022 at 12:18:02AM +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           | 19 ++++++++++---------
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  4 ++--
>  src/qcam/dng_writer.cpp                       | 12 ++++++------
>  3 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 00600a2e..54a16ac2 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -505,18 +505,19 @@ 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) }));


That's a little more verbose, but I guess there are not many ways
around it :(

>  		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) }));

I think you can omit the static_casts, as uint16_t should be
implicitly convertible to int32_t ? (my compiler doesn't complain it
seems).

>
>  	FocusStatus *focusStatus = rpiMetadata_.GetLocked<FocusStatus>("focus.status");
>  	if (focusStatus && focusStatus->num == 12) {
> @@ -821,7 +822,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;
>  		}
>
> @@ -1105,8 +1106,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 556af882..af7c8927 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1706,9 +1706,9 @@ 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 =
> +		libcamera::Span<const float, 2> colourGains =
>  			controls.get(libcamera::controls::ColourGains).\
> -				value_or(libcamera::Span<const float>({ 0, 0 }));
> +				value_or(libcamera::Span<const float, 2>({ 0, 0 }));

You can omit the libcamera:: namespace specified in libcamera::Span<>.

Actually it shouldn't have been there from the beginning, or am I
missing something ?

>  		/* 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 e119ca52..030432e3 100644
> --- a/src/qcam/dng_writer.cpp
> +++ b/src/qcam/dng_writer.cpp
> @@ -438,8 +438,8 @@ 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).value_or(libcamera::Span<const float>({ 0, 0 }));
> +		Span<const float, 2> const &colourGains =
> +			metadata.get(controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));

Again I think libcamera:: can be omitted, even if the file seems to be
using a mixture of libcamera::Span<> and just Span<>

Thanks
   j

>  		if (colourGains[0] > eps && colourGains[1] > eps) {
>  			wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
>  			neutral[0] = 1.0 / colourGains[0]; /* red */
> @@ -447,8 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>  		}
>  	}
>  	if (metadata.contains(controls::ColourCorrectionMatrix)) {
> -		Span<const float> const &coeffs =
> -			metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
> +		Span<const float, 9> const &coeffs =
> +			metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float, 9>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
>  		Matrix3d ccmSupplied(coeffs);
>  		if (ccmSupplied.determinant() > eps)
>  			ccm = ccmSupplied;
> @@ -517,8 +517,8 @@ 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).value_or(Span<const int32_t>({ 0, 0, 0, 0 }));
> +		Span<const int32_t, 4> levels =
> +			metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t, 4>({ 0, 0, 0, 0 }));
>
>  		/*
>  		 * The black levels control is specified in R, Gr, Gb, B order.
> --
> 2.34.1
>