[libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use std::optional to handle invalid control values

[Laurent Pinchart]

Hi Christian,

Thank you for the patch.

On Thu, Jun 02, 2022 at 12:17:59AM +0100, Christian Rauch via libcamera-devel wrote:
> Previously, ControlList::get<T>() would use default constructed objects to
> indicate that a ControlList does not have the requested Control. This has
> several disadvantages: 1) It requires types to be default constructible,
> 2) it does not differentiate between a default constructed object and an
> object that happens to have the same state as a default constructed object.
> 
> std::optional<T> additionally stores the information if the object is valid
> or not, and therefore is more expressive than a default constructed object.

I think I like this :-)

> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
> ---
>  include/libcamera/controls.h                      |  7 ++++---
>  src/cam/main.cpp                                  |  4 ++--
>  src/ipa/raspberrypi/raspberrypi.cpp               |  2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp              |  9 ++++-----
>  .../pipeline/raspberrypi/raspberrypi.cpp          | 10 ++++++----
>  src/qcam/dng_writer.cpp                           | 15 +++++++++------
>  6 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 665bcac1..363e7809 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -13,6 +13,7 @@
>  #include <string>
>  #include <unordered_map>
>  #include <vector>
> +#include <optional>

Please keep headers alphabetically sorted. Are you using the
checkstyle.py style checker ? You can copy utils/hooks/post-commit (or
pre-commit if preferred) git hook to .git/hooks/ to automate this.

> 
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/span.h>
> @@ -167,7 +168,7 @@ public:
> 
>  		using V = typename T::value_type;
>  		const V *value = reinterpret_cast<const V *>(data().data());
> -		return { value, numElements_ };
> +		return T{ value, numElements_ };

This seems unrelated. It makes the code more explicit though so I think
it's good, but I'd at least mention it in the commit message as a "While
at it, ..." item.

>  	}
> 
>  #ifndef __DOXYGEN__
> @@ -373,11 +374,11 @@ public:
>  	bool contains(unsigned int id) const;
> 
>  	template<typename T>
> -	T get(const Control<T> &ctrl) const
> +	std::optional<T> get(const Control<T> &ctrl) const
>  	{
>  		const ControlValue *val = find(ctrl.id());
>  		if (!val)
> -			return T{};
> +			return std::nullopt;
> 
>  		return val->get<T>();
>  	}
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 79875ed7..9b773931 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)
>  	 * is only used if the location isn't present or is set to External.
>  	 */
>  	if (props.contains(properties::Location)) {
> -		switch (props.get(properties::Location)) {
> +		switch (props.get(properties::Location).value_or(int32_t{})) {

Given that the props.contains() check guarantees that the value is
present, we could use

		switch (*props.get(properties::Location)) {

here. Alternatively, the code could be changed to

	const auto location = props.get(properties::Location);
	if (location) {
		switch (*location) {

which avoids the double lookup. I think I like this one better. Any
opinion from anyone ?

The same comment applies to several locations below.

>  		case properties::CameraLocationFront:
>  			addModel = false;
>  			name = "Internal front camera ";
> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)
>  		 * If the camera location is not availble use the camera model
>  		 * to build the camera name.
>  		 */
> -		name = "'" + props.get(properties::Model) + "' ";
> +		name = "'" + props.get(properties::Model).value_or(std::string{}) + "' ";

This brings the question of what we should do for properties (and
controls below) that are mandatory. One option would be to simply write

		name = "'" + *props.get(properties::Model) + "' ";

This leads to undefined behaviour if the property isn't present, which
isn't very nice. On the other hand, the runtime check is in theory not
necessary, as the property is mandatory (which should be ensured through
compliance testing).

If we want to keep the check, I'd like to shorten the line a bit with

		name = "'" + props.get(properties::Model).value_or("") + "' ";
>  	}
> 
>  	name += "(" + camera->id() + ")";
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3b126bb5..00600a2e 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> 
>  void IPARPi::prepareISP(const ISPConfig &data)
>  {
> -	int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> +	int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});

Same comment here, we could drop the value_or(), or call value_or(0).
Same in other locations below where applicable.

>  	RPiController::Metadata lastMetadata;
>  	Span<uint8_t> embeddedBuffer;
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index fd989e61..1e9e5081 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()
>  		/* Convert the sensor rotation to a transformation */
>  		int32_t rotation = 0;
>  		if (data->properties_.contains(properties::Rotation))
> -			rotation = data->properties_.get(properties::Rotation);
> +			rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});
>  		else
>  			LOG(IPU3, Warning) << "Rotation control not exposed by "
>  					   << cio2->sensor()->id()
> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>  	request->metadata().set(controls::draft::PipelineDepth, 3);
>  	/* \todo Actually apply the scaler crop region to the ImgU. */
>  	if (request->controls().contains(controls::ScalerCrop))
> -		cropRegion_ = request->controls().get(controls::ScalerCrop);
> +		cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});
>  	request->metadata().set(controls::ScalerCrop, cropRegion_);
> 
>  	if (frameInfos_.tryComplete(info))
> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  		return;
>  	}
> 
> -	ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),
> +	ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(int64_t{}),
>  				 info->statBuffer->cookie(), info->effectiveSensorControls);
>  }
> 
> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)
>  	if (!request->controls().contains(controls::draft::TestPatternMode))
>  		return;
> 
> -	const int32_t testPatternMode = request->controls().get(
> -		controls::draft::TestPatternMode);
> +	const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});
> 
>  	int ret = cio2_.sensor()->setTestPatternMode(
>  		static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index adc397e8..556af882 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  	 * error means the platform can never run. Let's just print a warning
>  	 * and continue regardless; the rotation is effectively set to zero.
>  	 */
> -	int32_t rotation = data_->sensor_->properties().get(properties::Rotation);
> +	int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});
>  	bool success;
>  	Transform rotationTransform = transformFromRotation(rotation, &success);
>  	if (!success)
> @@ -1706,7 +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 = controls.get(libcamera::controls::ColourGains);
> +		libcamera::Span<const float> colourGains =
> +			controls.get(libcamera::controls::ColourGains).\
> +				value_or(libcamera::Span<const float>({ 0, 0 }));
>  		/* The control wants linear gains in the order B, Gb, Gr, R. */
>  		ControlList ctrls(sensor_->controls());
>  		std::array<int32_t, 4> gains{
> @@ -2041,7 +2043,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const
>  void RPiCameraData::applyScalerCrop(const ControlList &controls)
>  {
>  	if (controls.contains(controls::ScalerCrop)) {
> -		Rectangle nativeCrop = controls.get<Rectangle>(controls::ScalerCrop);
> +		Rectangle nativeCrop = controls.get<Rectangle>(controls::ScalerCrop).value_or(Rectangle{});
> 
>  		if (!nativeCrop.width || !nativeCrop.height)
>  			nativeCrop = { 0, 0, 1, 1 };
> @@ -2079,7 +2081,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
>  					Request *request)
>  {
>  	request->metadata().set(controls::SensorTimestamp,
> -				bufferControls.get(controls::SensorTimestamp));
> +				bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));
> 
>  	request->metadata().set(controls::ScalerCrop, scalerCrop_);
>  }
> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> index 34c8df5a..e119ca52 100644
> --- a/src/qcam/dng_writer.cpp
> +++ b/src/qcam/dng_writer.cpp
> @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>  	TIFFSetField(tif, TIFFTAG_MAKE, "libcamera");
> 
>  	if (cameraProperties.contains(properties::Model)) {
> -		std::string model = cameraProperties.get(properties::Model);
> +		std::string model = cameraProperties.get(properties::Model).value_or(std::string{});
>  		TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());
>  		/* \todo set TIFFTAG_UNIQUECAMERAMODEL. */
>  	}
> @@ -438,7 +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);
> +		Span<const float> const &colourGains =
> +			metadata.get(controls::ColourGains).value_or(libcamera::Span<const float>({ 0, 0 }));
>  		if (colourGains[0] > eps && colourGains[1] > eps) {
>  			wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
>  			neutral[0] = 1.0 / colourGains[0]; /* red */
> @@ -446,7 +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);
> +		Span<const float> const &coeffs =
> +			metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
>  		Matrix3d ccmSupplied(coeffs);
>  		if (ccmSupplied.determinant() > eps)
>  			ccm = ccmSupplied;
> @@ -515,7 +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);
> +		Span<const int32_t> levels =
> +			metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t>({ 0, 0, 0, 0 }));
> 
>  		/*
>  		 * The black levels control is specified in R, Gr, Gb, B order.
> @@ -593,13 +596,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>  	TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);
> 
>  	if (metadata.contains(controls::AnalogueGain)) {
> -		float gain = metadata.get(controls::AnalogueGain);
> +		float gain = metadata.get(controls::AnalogueGain).value_or(float{});
>  		uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);
>  		TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);
>  	}
> 
>  	if (metadata.contains(controls::ExposureTime)) {
> -		float exposureTime = metadata.get(controls::ExposureTime) / 1e6;
> +		float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;
>  		TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
>  	}
> 

-- 
Regards,

Laurent Pinchart