[libcamera-devel,v5,4/4] libcamera: controls: Apply explicit fixed-sized Span type casts
diff mbox series

Message ID 20220601231802.16735-5-Rauch.Christian@gmx.de
State Superseded
Headers show
Series
  • generate and use fixed-sized Span Control types
Related show

Commit Message

Christian Rauch June 1, 2022, 11:18 p.m. UTC
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@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(-)

--
2.34.1

Comments

Jacopo Mondi June 3, 2022, 7:59 a.m. UTC | #1
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@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
>

Patch
diff mbox series

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) }));
 		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) {
@@ -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 }));
 		/* 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 }));
 		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.