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

16127 diff mbox series

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.

Signed-off-by: Christian Rauch <Rauch.Christian@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(-)

--
2.34.1

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

Hi Laurent,

Thanks for looking at this.

Am 02.06.22 um 10:58 schrieb 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@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.

I stopped using the automated checker in my IDE because I got a lot of
"false positives" where it adapted the style according to the
clang-format file, that were later rejected on the mailing list. But I
will note down that this header order should be included.

>
>>
>>  #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.

This might have been a left-over from an earlier version of this patch
set. I am removing this in a future version as it is not necessary.

>
>>  	}
>>
>>  #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 ?

I initially replicated the old behaviour by returning a default
constructed object if the value is not valid. But now, I like your idea
better since "if (location)" makes this much more clear.

>
> 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("") + "' ";

I would not use "*props.get()" unless it was checked before that
"props.has_value()" is true. If the compliance test fails to recognise
an undefined "Model" in some situation,s then this will dereference a
NULL pointer, which is hard to debug.

So unless it is checked before that "props" is valid, I would always
fall back to the default value like:
"props.get(<propery>).value_or(<default>)"

>>  	}
>>
>>  	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.

Do you want to drop the "value_or" because it is somewhere else
guaranteed that this control exists? Or do you just explicitely set "0"
instead of the default constructed int64_t?

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

Hi Christian,

On Fri, Jun 03, 2022 at 12:48:15AM +0100, Christian Rauch via libcamera-devel wrote:
> Am 02.06.22 um 10:58 schrieb Laurent Pinchart:
> > 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@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.
> 
> I stopped using the automated checker in my IDE because I got a lot of
> "false positives" where it adapted the style according to the
> clang-format file, that were later rejected on the mailing list. But I
> will note down that this header order should be included.

Unfortunately we haven't been able to find a clang-format configuration
that works perfectly :-S I'd still recommend running checkstyle.py, and
cherry-picking the warnings that you think are appropriate. We keep
improving checkstyle.py (albeit at a slow pace), but the issues related
to clang-format are more difficult to address.

> >>
> >>  #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.
> 
> This might have been a left-over from an earlier version of this patch
> set. I am removing this in a future version as it is not necessary.
> 
> >>  	}
> >>
> >>  #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 ?
> 
> I initially replicated the old behaviour by returning a default
> constructed object if the value is not valid. But now, I like your idea
> better since "if (location)" makes this much more clear.
> 
> > 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("") + "' ";
> 
> I would not use "*props.get()" unless it was checked before that
> "props.has_value()" is true. If the compliance test fails to recognise
> an undefined "Model" in some situation,s then this will dereference a
> NULL pointer, which is hard to debug.
> 
> So unless it is checked before that "props" is valid, I would always
> fall back to the default value like:
> "props.get(<propery>).value_or(<default>)"

At the end of the day we need to ensure it won't crash, that's very
clear. .value_or() ensures that, so would the implicit knowledge that
the value is present due to the fact that the API requires it. The
latter can be further enforced through compliance testing, but I agree
it sounds more fragile, as compliance testing may not be 100% accurate.
I'm not sure where exactly to draw the line, it's the usual question of
how to handle API contracts that are not met, what should a caller do
when a callee returns something that is invalid according to the API
documentation ?

> >>  	}
> >>
> >>  	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.
> 
> Do you want to drop the "value_or" because it is somewhere else
> guaranteed that this control exists? Or do you just explicitely set "0"
> instead of the default constructed int64_t?

Either :-) Dropping .value_or() is one option (based on the outcome of
the discussion above), and if we want to keep it, I'd write .value_or(0)
instead of using the default-constructed int64_t as it's shorter and
more explicit.

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