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

16143 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                  |  5 +++--
 src/android/camera_capabilities.cpp           |  8 +++----
 src/android/camera_device.cpp                 | 21 +++++++++----------
 src/android/camera_hal_manager.cpp            |  2 +-
 src/cam/main.cpp                              |  4 ++--
 src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  9 ++++----
 .../pipeline/raspberrypi/raspberrypi.cpp      |  9 ++++----
 src/qcam/dng_writer.cpp                       | 15 +++++++------
 9 files changed, 39 insertions(+), 36 deletions(-)

--
2.34.1

Hi Christian

On Fri, Jun 03, 2022 at 11:07:08PM +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.
>
> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> ---
>  include/libcamera/controls.h                  |  5 +++--
>  src/android/camera_capabilities.cpp           |  8 +++----
>  src/android/camera_device.cpp                 | 21 +++++++++----------
>  src/android/camera_hal_manager.cpp            |  2 +-
>  src/cam/main.cpp                              |  4 ++--
>  src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  9 ++++----
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  9 ++++----
>  src/qcam/dng_writer.cpp                       | 15 +++++++------
>  9 files changed, 39 insertions(+), 36 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 665bcac1..192be784 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -8,6 +8,7 @@
>  #pragma once
>
>  #include <assert.h>
> +#include <optional>
>  #include <set>
>  #include <stdint.h>
>  #include <string>
> @@ -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/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 6f197eb8..892bbc2b 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()
>  	/* Sensor static metadata. */
>  	std::array<int32_t, 2> pixelArraySize;
>  	{
> -		const Size &size = properties.get(properties::PixelArraySize);
> +		const Size &size = properties.get(properties::PixelArraySize).value_or(Size{});

We should probably wrap this and a few similar cases above with an

        if (properties.contains(...))

Not for this patch though.

>  		pixelArraySize[0] = size.width;
>  		pixelArraySize[1] = size.height;
>  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()
>  	}
>
>  	if (properties.contains(properties::UnitCellSize)) {
> -		const Size &cellSize = properties.get<Size>(properties::UnitCellSize);
> +		const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);
>  		std::array<float, 2> physicalSize{
>  			cellSize.width * pixelArraySize[0] / 1e6f,
>  			cellSize.height * pixelArraySize[1] / 1e6f
> @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()
>
>  	{
>  		const Span<const Rectangle> &rects =
> -			properties.get(properties::PixelArrayActiveAreas);
> +			properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
>  		std::vector<int32_t> data{
>  			static_cast<int32_t>(rects[0].x),
>  			static_cast<int32_t>(rects[0].y),
> @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()
>
>  	/* Report the color filter arrangement if the camera reports it. */
>  	if (properties.contains(properties::draft::ColorFilterArrangement)) {
> -		uint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);
> +		uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);
>  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
>  					  filterArr);
>  	}
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 8e804d4d..ec117101 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
>  	const ControlList &properties = camera_->properties();
>
>  	if (properties.contains(properties::Location)) {
> -		int32_t location = properties.get(properties::Location);
> +		int32_t location = *properties.get(properties::Location);
>  		switch (location) {
>  		case properties::CameraLocationFront:
>  			facing_ = CAMERA_FACING_FRONT;
> @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
>  	 * metadata.
>  	 */
>  	if (properties.contains(properties::Rotation)) {
> -		int rotation = properties.get(properties::Rotation);
> +		int rotation = *properties.get(properties::Rotation);
>  		orientation_ = (360 - rotation) % 360;
>  		if (cameraConfigData && cameraConfigData->rotation != -1 &&
>  		    orientation_ != cameraConfigData->rotation) {
> @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)
>  	 * as soon as possible, earlier than request completion time.
>  	 */
>  	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> -							 .get(controls::SensorTimestamp));
> +								 .get(controls::SensorTimestamp)
> +								 .value_or(0));
>  	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
>
>  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
>  				 rolling_shutter_skew);
>
>  	/* Add metadata tags reported by libcamera. */
> -	const int64_t timestamp = metadata.get(controls::SensorTimestamp);
> +	const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);
>  	resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);
>
>  	if (metadata.contains(controls::draft::PipelineDepth)) {
> -		uint8_t pipeline_depth =
> -			metadata.get<int32_t>(controls::draft::PipelineDepth);
> +		uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth);
>  		resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,
>  					 pipeline_depth);
>  	}
>
>  	if (metadata.contains(controls::ExposureTime)) {
> -		int64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;
> +		int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;
>  		resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);
>  	}
>
>  	if (metadata.contains(controls::FrameDuration)) {
> -		int64_t duration = metadata.get(controls::FrameDuration) * 1000;
> +		int64_t duration = *metadata.get(controls::FrameDuration) * 1000;
>  		resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,
>  					 duration);
>  	}
>
>  	if (metadata.contains(controls::ScalerCrop)) {
> -		Rectangle crop = metadata.get(controls::ScalerCrop);
> +		Rectangle crop = *metadata.get(controls::ScalerCrop);
>  		int32_t cropRect[] = {
>  			crop.x, crop.y, static_cast<int32_t>(crop.width),
>  			static_cast<int32_t>(crop.height),
> @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
>  	}
>
>  	if (metadata.contains(controls::draft::TestPatternMode)) {
> -		const int32_t testPatternMode =
> -			metadata.get(controls::draft::TestPatternMode);
> +		const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);
>  		resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,
>  					 testPatternMode);
>  	}
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 5f7bfe26..0b23397a 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)
>  	if (!properties.contains(properties::Location))
>  		return -1;
>
> -	return properties.get(properties::Location);
> +	return properties.get(properties::Location).value_or(0);

No need to value_or() as we return earlier if Location is not
available

>  }
>
>  CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 79875ed7..d8115cd8 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)) {
>  		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) + "' ";
>  	}
>
>  	name += "(" + camera->id() + ")";
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3b126bb5..f65a0680 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(0);

This should in theory be guaranteed to be there, as it's the pipeline
handler that populates it. However I'm not sure what's best. Either
ASSERT() that the control is there or default it to 0. frameTimestamp
is not used as a divider anywhere so there's no risk of undefined
behaviour.

>  	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..53332826 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));
>  		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));
>  	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(0),
>  				 info->statBuffer->cookie(), info->effectiveSensorControls);

Also in this case we should be guaranteed the sensor timestamp is
there.

I'm tempted to ASSERT here as well...

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

No need as we return earlier if !TestPatternMode

>
>  	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..a62afdd4 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(0);
>  	bool success;
>  	Transform rotationTransform = transformFromRotation(rotation, &success);
>  	if (!success)
> @@ -1706,7 +1706,8 @@ 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);
>  		/* The control wants linear gains in the order B, Gb, Gr, R. */
>  		ControlList ctrls(sensor_->controls());
>  		std::array<int32_t, 4> gains{
> @@ -2041,7 +2042,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);
>
>  		if (!nativeCrop.width || !nativeCrop.height)
>  			nativeCrop = { 0, 0, 1, 1 };
> @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
>  					Request *request)
>  {
>  	request->metadata().set(controls::SensorTimestamp,
> -				bufferControls.get(controls::SensorTimestamp));
> +				bufferControls.get(controls::SensorTimestamp).value_or(0));
>
>  	request->metadata().set(controls::ScalerCrop, scalerCrop_);
>  }
> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> index 34c8df5a..780f58f7 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);
>  		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);
>  		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);
>  		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);
>
>  		/*
>  		 * 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);
>  		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) / 1e6;
>  		TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
>  	}

Let's see what's Laurent comment on the possible assertion, but the
patch looks good to me and the few other nits can be changed when
applying if you agree with them

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> --
> 2.34.1
>

Hi Christian,
   spoke too soon

On Sat, Jun 04, 2022 at 09:31:13AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Christian
>
> On Fri, Jun 03, 2022 at 11:07:08PM +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.
> >
> > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > ---
> >  include/libcamera/controls.h                  |  5 +++--
> >  src/android/camera_capabilities.cpp           |  8 +++----
> >  src/android/camera_device.cpp                 | 21 +++++++++----------
> >  src/android/camera_hal_manager.cpp            |  2 +-
> >  src/cam/main.cpp                              |  4 ++--
> >  src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp          |  9 ++++----
> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  9 ++++----
> >  src/qcam/dng_writer.cpp                       | 15 +++++++------
> >  9 files changed, 39 insertions(+), 36 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 665bcac1..192be784 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -8,6 +8,7 @@
> >  #pragma once
> >
> >  #include <assert.h>
> > +#include <optional>
> >  #include <set>
> >  #include <stdint.h>
> >  #include <string>
> > @@ -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/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index 6f197eb8..892bbc2b 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	/* Sensor static metadata. */
> >  	std::array<int32_t, 2> pixelArraySize;
> >  	{
> > -		const Size &size = properties.get(properties::PixelArraySize);
> > +		const Size &size = properties.get(properties::PixelArraySize).value_or(Size{});
>
> We should probably wrap this and a few similar cases above with an
>
>         if (properties.contains(...))
>
> Not for this patch though.
>
> >  		pixelArraySize[0] = size.width;
> >  		pixelArraySize[1] = size.height;
> >  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	}
> >
> >  	if (properties.contains(properties::UnitCellSize)) {
> > -		const Size &cellSize = properties.get<Size>(properties::UnitCellSize);
> > +		const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);

Here you could remove the <Size> template argument if I'm not
mistaken. I know it was already there...

But, most important, I get the following with clang 14.0.0

(Showing with both <Size> and without)

src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
                const Size &cellSize = *properties.get(properties::UnitCellSize);

src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
                const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);

I guess the issue might be due to taking a reference to a temporary
value as:
const Size cellSize = *properties.get(properties::UnitCellSize);

as well as:
const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;

compile fine.

When it comes to the usage of value_or(), its documentation says:
[1] https://en.cppreference.com/w/cpp/utility/optional/value_or
-T must meet the requirements of CopyConstructible in order to use overload (1).
-T must meet the requirements of MoveConstructible in order to use overload (2).

which means the value is copied or moved back while operator*
[2] https://en.cppreference.com/w/cpp/utility/optional/operator*
1) Returns a pointer to the contained value.
2) Returns a reference to the contained value.

I guess the issue is due to trying to reference the content of a
temporary std::optional<T> instance, created to wrap the return value
of ControlValue::get<T>

  ControlList:
	template<typename T>
	std::optional<T> get(const Control<T> &ctrl) const
	{
		const ControlValue *val = find(ctrl.id());
		if (!val)
			return std::nullopt;

		return val->get<T>();
	}

This should be fine, as the object returned by ControlValue::get<>()
refers to valid memory, and as we could take references to it before
this series, we should be able to do so now, but the compiler worries
that dereferencing the temporary std::optional<> would leave us with a
pointer to invalid memory. Is my interpretation of the issue correct ?

> >  			cellSize.width * pixelArraySize[0] / 1e6f,
> >  			cellSize.height * pixelArraySize[1] / 1e6f
> > @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >
> >  	{
> >  		const Span<const Rectangle> &rects =
> > -			properties.get(properties::PixelArrayActiveAreas);
> > +			properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
> >  		std::vector<int32_t> data{
> >  			static_cast<int32_t>(rects[0].x),
> >  			static_cast<int32_t>(rects[0].y),
> > @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >
> >  	/* Report the color filter arrangement if the camera reports it. */
> >  	if (properties.contains(properties::draft::ColorFilterArrangement)) {
> > -		uint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);
> > +		uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);
> >  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> >  					  filterArr);
> >  	}
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 8e804d4d..ec117101 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> >  	const ControlList &properties = camera_->properties();
> >
> >  	if (properties.contains(properties::Location)) {
> > -		int32_t location = properties.get(properties::Location);
> > +		int32_t location = *properties.get(properties::Location);
> >  		switch (location) {
> >  		case properties::CameraLocationFront:
> >  			facing_ = CAMERA_FACING_FRONT;
> > @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> >  	 * metadata.
> >  	 */
> >  	if (properties.contains(properties::Rotation)) {
> > -		int rotation = properties.get(properties::Rotation);
> > +		int rotation = *properties.get(properties::Rotation);
> >  		orientation_ = (360 - rotation) % 360;
> >  		if (cameraConfigData && cameraConfigData->rotation != -1 &&
> >  		    orientation_ != cameraConfigData->rotation) {
> > @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)
> >  	 * as soon as possible, earlier than request completion time.
> >  	 */
> >  	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> > -							 .get(controls::SensorTimestamp));
> > +								 .get(controls::SensorTimestamp)
> > +								 .value_or(0));
> >  	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >
> >  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> > @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >  				 rolling_shutter_skew);
> >
> >  	/* Add metadata tags reported by libcamera. */
> > -	const int64_t timestamp = metadata.get(controls::SensorTimestamp);
> > +	const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);
> >  	resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);
> >
> >  	if (metadata.contains(controls::draft::PipelineDepth)) {
> > -		uint8_t pipeline_depth =
> > -			metadata.get<int32_t>(controls::draft::PipelineDepth);
> > +		uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth);
> >  		resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,
> >  					 pipeline_depth);
> >  	}
> >
> >  	if (metadata.contains(controls::ExposureTime)) {
> > -		int64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;
> > +		int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;
> >  		resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);
> >  	}
> >
> >  	if (metadata.contains(controls::FrameDuration)) {
> > -		int64_t duration = metadata.get(controls::FrameDuration) * 1000;
> > +		int64_t duration = *metadata.get(controls::FrameDuration) * 1000;
> >  		resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,
> >  					 duration);
> >  	}
> >
> >  	if (metadata.contains(controls::ScalerCrop)) {
> > -		Rectangle crop = metadata.get(controls::ScalerCrop);
> > +		Rectangle crop = *metadata.get(controls::ScalerCrop);
> >  		int32_t cropRect[] = {
> >  			crop.x, crop.y, static_cast<int32_t>(crop.width),
> >  			static_cast<int32_t>(crop.height),
> > @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >  	}
> >
> >  	if (metadata.contains(controls::draft::TestPatternMode)) {
> > -		const int32_t testPatternMode =
> > -			metadata.get(controls::draft::TestPatternMode);
> > +		const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);
> >  		resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,
> >  					 testPatternMode);
> >  	}
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > index 5f7bfe26..0b23397a 100644
> > --- a/src/android/camera_hal_manager.cpp
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)
> >  	if (!properties.contains(properties::Location))
> >  		return -1;
> >
> > -	return properties.get(properties::Location);
> > +	return properties.get(properties::Location).value_or(0);
>
> No need to value_or() as we return earlier if Location is not
> available
>
> >  }
> >
> >  CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 79875ed7..d8115cd8 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)) {
> >  		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) + "' ";
> >  	}
> >
> >  	name += "(" + camera->id() + ")";
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 3b126bb5..f65a0680 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(0);
>
> This should in theory be guaranteed to be there, as it's the pipeline
> handler that populates it. However I'm not sure what's best. Either
> ASSERT() that the control is there or default it to 0. frameTimestamp
> is not used as a divider anywhere so there's no risk of undefined
> behaviour.
>
> >  	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..53332826 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));
> >  		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));
> >  	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(0),
> >  				 info->statBuffer->cookie(), info->effectiveSensorControls);
>
> Also in this case we should be guaranteed the sensor timestamp is
> there.
>
> I'm tempted to ASSERT here as well...
>
> >  }
> >
> > @@ -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(0);
>
> No need as we return earlier if !TestPatternMode
>
> >
> >  	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..a62afdd4 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(0);
> >  	bool success;
> >  	Transform rotationTransform = transformFromRotation(rotation, &success);
> >  	if (!success)
> > @@ -1706,7 +1706,8 @@ 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);
> >  		/* The control wants linear gains in the order B, Gb, Gr, R. */
> >  		ControlList ctrls(sensor_->controls());
> >  		std::array<int32_t, 4> gains{
> > @@ -2041,7 +2042,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);
> >
> >  		if (!nativeCrop.width || !nativeCrop.height)
> >  			nativeCrop = { 0, 0, 1, 1 };
> > @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
> >  					Request *request)
> >  {
> >  	request->metadata().set(controls::SensorTimestamp,
> > -				bufferControls.get(controls::SensorTimestamp));
> > +				bufferControls.get(controls::SensorTimestamp).value_or(0));
> >
> >  	request->metadata().set(controls::ScalerCrop, scalerCrop_);
> >  }
> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> > index 34c8df5a..780f58f7 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);
> >  		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);
> >  		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);
> >  		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);
> >
> >  		/*
> >  		 * 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);
> >  		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) / 1e6;
> >  		TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
> >  	}
>
> Let's see what's Laurent comment on the possible assertion, but the
> patch looks good to me and the few other nits can be changed when
> applying if you agree with them
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
>
> >
> > --
> > 2.34.1
> >

Hi Jacopo,

Am 04.06.22 um 09:29 schrieb Jacopo Mondi:
> Hi Christian,
>    spoke too soon
>
> On Sat, Jun 04, 2022 at 09:31:13AM +0200, Jacopo Mondi via libcamera-devel wrote:
>> Hi Christian
>>
>> On Fri, Jun 03, 2022 at 11:07:08PM +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.
>>>
>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>> ---
>>>  include/libcamera/controls.h                  |  5 +++--
>>>  src/android/camera_capabilities.cpp           |  8 +++----
>>>  src/android/camera_device.cpp                 | 21 +++++++++----------
>>>  src/android/camera_hal_manager.cpp            |  2 +-
>>>  src/cam/main.cpp                              |  4 ++--
>>>  src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
>>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  9 ++++----
>>>  .../pipeline/raspberrypi/raspberrypi.cpp      |  9 ++++----
>>>  src/qcam/dng_writer.cpp                       | 15 +++++++------
>>>  9 files changed, 39 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> index 665bcac1..192be784 100644
>>> --- a/include/libcamera/controls.h
>>> +++ b/include/libcamera/controls.h
>>> @@ -8,6 +8,7 @@
>>>  #pragma once
>>>
>>>  #include <assert.h>
>>> +#include <optional>
>>>  #include <set>
>>>  #include <stdint.h>
>>>  #include <string>
>>> @@ -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/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
>>> index 6f197eb8..892bbc2b 100644
>>> --- a/src/android/camera_capabilities.cpp
>>> +++ b/src/android/camera_capabilities.cpp
>>> @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()
>>>  	/* Sensor static metadata. */
>>>  	std::array<int32_t, 2> pixelArraySize;
>>>  	{
>>> -		const Size &size = properties.get(properties::PixelArraySize);
>>> +		const Size &size = properties.get(properties::PixelArraySize).value_or(Size{});
>>
>> We should probably wrap this and a few similar cases above with an
>>
>>         if (properties.contains(...))
>>
>> Not for this patch though.
>>
>>>  		pixelArraySize[0] = size.width;
>>>  		pixelArraySize[1] = size.height;
>>>  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
>>> @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()
>>>  	}
>>>
>>>  	if (properties.contains(properties::UnitCellSize)) {
>>> -		const Size &cellSize = properties.get<Size>(properties::UnitCellSize);
>>> +		const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);
>
> Here you could remove the <Size> template argument if I'm not
> mistaken. I know it was already there...
>
> But, most important, I get the following with clang 14.0.0
>
> (Showing with both <Size> and without)
>
> src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
>                 const Size &cellSize = *properties.get(properties::UnitCellSize);
>
> src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
>                 const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);
>
> I guess the issue might be due to taking a reference to a temporary
> value as:
> const Size cellSize = *properties.get(properties::UnitCellSize);
>
> as well as:
> const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;
>
> compile fine.

Thanks for pointing this out. I could reproduce this with clang
("CC=clang CXX=clang++ meson build -Dandroid=enabled") but not with gcc.
Even "-Dwarning_level=3" did not show these errors.

Does libcamera run a build server somewhere? This would be helpful to
discover errors in non-standard configurations (e.g. with clang in this
case or with "android" enabled).

>
> When it comes to the usage of value_or(), its documentation says:
> [1] https://en.cppreference.com/w/cpp/utility/optional/value_or
> -T must meet the requirements of CopyConstructible in order to use overload (1).
> -T must meet the requirements of MoveConstructible in order to use overload (2).
>
> which means the value is copied or moved back while operator*
> [2] https://en.cppreference.com/w/cpp/utility/optional/operator*
> 1) Returns a pointer to the contained value.
> 2) Returns a reference to the contained value.
>
> I guess the issue is due to trying to reference the content of a
> temporary std::optional<T> instance, created to wrap the return value
> of ControlValue::get<T>
>
>   ControlList:
> 	template<typename T>
> 	std::optional<T> get(const Control<T> &ctrl) const
> 	{
> 		const ControlValue *val = find(ctrl.id());
> 		if (!val)
> 			return std::nullopt;
>
> 		return val->get<T>();
> 	}
>
> This should be fine, as the object returned by ControlValue::get<>()
> refers to valid memory, and as we could take references to it before
> this series, we should be able to do so now, but the compiler worries
> that dereferencing the temporary std::optional<> would leave us with a
> pointer to invalid memory. Is my interpretation of the issue correct ?
>

Yes, I think what is happening here is that the optional returns the
pointer to a value that can become "invalid" later. This could happen
when the "std::optional<T>::reset()" is called or when "std::nullopt" is
assigned.

Taking the reference of this would be dangerous. We know that the
reference will "cellSize" be destroyed shortly after, but generally,
there is no guarantee. For the compiler, it makes sense to complain
about this.

I only see two ways to solve this:

1. copy the content:

  const Size cellSize = *properties.get<Size>(properties::UnitCellSize);

2. use the "std::optional" directly:

  const auto cellSize = properties.get<Size>(properties::UnitCellSize);

  and then access it via the "->" operator:

  cellSize->width

I personally prefer the second option since it avoids copying the entire
content, which could be expensive for other control types.


>>>  			cellSize.width * pixelArraySize[0] / 1e6f,
>>>  			cellSize.height * pixelArraySize[1] / 1e6f
>>> @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()
>>>
>>>  	{
>>>  		const Span<const Rectangle> &rects =
>>> -			properties.get(properties::PixelArrayActiveAreas);
>>> +			properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
>>>  		std::vector<int32_t> data{
>>>  			static_cast<int32_t>(rects[0].x),
>>>  			static_cast<int32_t>(rects[0].y),
>>> @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()
>>>
>>>  	/* Report the color filter arrangement if the camera reports it. */
>>>  	if (properties.contains(properties::draft::ColorFilterArrangement)) {
>>> -		uint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);
>>> +		uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);
>>>  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
>>>  					  filterArr);
>>>  	}
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index 8e804d4d..ec117101 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
>>>  	const ControlList &properties = camera_->properties();
>>>
>>>  	if (properties.contains(properties::Location)) {
>>> -		int32_t location = properties.get(properties::Location);
>>> +		int32_t location = *properties.get(properties::Location);
>>>  		switch (location) {
>>>  		case properties::CameraLocationFront:
>>>  			facing_ = CAMERA_FACING_FRONT;
>>> @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
>>>  	 * metadata.
>>>  	 */
>>>  	if (properties.contains(properties::Rotation)) {
>>> -		int rotation = properties.get(properties::Rotation);
>>> +		int rotation = *properties.get(properties::Rotation);
>>>  		orientation_ = (360 - rotation) % 360;
>>>  		if (cameraConfigData && cameraConfigData->rotation != -1 &&
>>>  		    orientation_ != cameraConfigData->rotation) {
>>> @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)
>>>  	 * as soon as possible, earlier than request completion time.
>>>  	 */
>>>  	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
>>> -							 .get(controls::SensorTimestamp));
>>> +								 .get(controls::SensorTimestamp)
>>> +								 .value_or(0));
>>>  	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
>>>
>>>  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
>>> @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
>>>  				 rolling_shutter_skew);
>>>
>>>  	/* Add metadata tags reported by libcamera. */
>>> -	const int64_t timestamp = metadata.get(controls::SensorTimestamp);
>>> +	const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);
>>>  	resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);
>>>
>>>  	if (metadata.contains(controls::draft::PipelineDepth)) {
>>> -		uint8_t pipeline_depth =
>>> -			metadata.get<int32_t>(controls::draft::PipelineDepth);
>>> +		uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth);
>>>  		resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,
>>>  					 pipeline_depth);
>>>  	}
>>>
>>>  	if (metadata.contains(controls::ExposureTime)) {
>>> -		int64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;
>>> +		int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;
>>>  		resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);
>>>  	}
>>>
>>>  	if (metadata.contains(controls::FrameDuration)) {
>>> -		int64_t duration = metadata.get(controls::FrameDuration) * 1000;
>>> +		int64_t duration = *metadata.get(controls::FrameDuration) * 1000;
>>>  		resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,
>>>  					 duration);
>>>  	}
>>>
>>>  	if (metadata.contains(controls::ScalerCrop)) {
>>> -		Rectangle crop = metadata.get(controls::ScalerCrop);
>>> +		Rectangle crop = *metadata.get(controls::ScalerCrop);
>>>  		int32_t cropRect[] = {
>>>  			crop.x, crop.y, static_cast<int32_t>(crop.width),
>>>  			static_cast<int32_t>(crop.height),
>>> @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
>>>  	}
>>>
>>>  	if (metadata.contains(controls::draft::TestPatternMode)) {
>>> -		const int32_t testPatternMode =
>>> -			metadata.get(controls::draft::TestPatternMode);
>>> +		const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);
>>>  		resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,
>>>  					 testPatternMode);
>>>  	}
>>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
>>> index 5f7bfe26..0b23397a 100644
>>> --- a/src/android/camera_hal_manager.cpp
>>> +++ b/src/android/camera_hal_manager.cpp
>>> @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)
>>>  	if (!properties.contains(properties::Location))
>>>  		return -1;
>>>
>>> -	return properties.get(properties::Location);
>>> +	return properties.get(properties::Location).value_or(0);
>>
>> No need to value_or() as we return earlier if Location is not
>> available
>>
>>>  }
>>>
>>>  CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)
>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
>>> index 79875ed7..d8115cd8 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)) {
>>>  		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) + "' ";
>>>  	}
>>>
>>>  	name += "(" + camera->id() + ")";
>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>> index 3b126bb5..f65a0680 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(0);
>>
>> This should in theory be guaranteed to be there, as it's the pipeline
>> handler that populates it. However I'm not sure what's best. Either
>> ASSERT() that the control is there or default it to 0. frameTimestamp
>> is not used as a divider anywhere so there's no risk of undefined
>> behaviour.
>>
>>>  	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..53332826 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));
>>>  		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));
>>>  	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(0),
>>>  				 info->statBuffer->cookie(), info->effectiveSensorControls);
>>
>> Also in this case we should be guaranteed the sensor timestamp is
>> there.
>>
>> I'm tempted to ASSERT here as well...
>>
>>>  }
>>>
>>> @@ -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(0);
>>
>> No need as we return earlier if !TestPatternMode
>>
>>>
>>>  	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..a62afdd4 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(0);
>>>  	bool success;
>>>  	Transform rotationTransform = transformFromRotation(rotation, &success);
>>>  	if (!success)
>>> @@ -1706,7 +1706,8 @@ 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);
>>>  		/* The control wants linear gains in the order B, Gb, Gr, R. */
>>>  		ControlList ctrls(sensor_->controls());
>>>  		std::array<int32_t, 4> gains{
>>> @@ -2041,7 +2042,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);
>>>
>>>  		if (!nativeCrop.width || !nativeCrop.height)
>>>  			nativeCrop = { 0, 0, 1, 1 };
>>> @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
>>>  					Request *request)
>>>  {
>>>  	request->metadata().set(controls::SensorTimestamp,
>>> -				bufferControls.get(controls::SensorTimestamp));
>>> +				bufferControls.get(controls::SensorTimestamp).value_or(0));
>>>
>>>  	request->metadata().set(controls::ScalerCrop, scalerCrop_);
>>>  }
>>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
>>> index 34c8df5a..780f58f7 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);
>>>  		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);
>>>  		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);
>>>  		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);
>>>
>>>  		/*
>>>  		 * 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);
>>>  		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) / 1e6;
>>>  		TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
>>>  	}
>>
>> Let's see what's Laurent comment on the possible assertion, but the
>> patch looks good to me and the few other nits can be changed when
>> applying if you agree with them
>>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Thanks
>>    j
>>
>>>
>>> --
>>> 2.34.1
>>>

Hi Christian

On Sat, Jun 04, 2022 at 09:50:27PM +0100, Christian Rauch via libcamera-devel wrote:
> Hi Jacopo,
>
> Am 04.06.22 um 09:29 schrieb Jacopo Mondi:
> > Hi Christian,
> >    spoke too soon
> >
> > On Sat, Jun 04, 2022 at 09:31:13AM +0200, Jacopo Mondi via libcamera-devel wrote:
> >> Hi Christian
> >>
> >> On Fri, Jun 03, 2022 at 11:07:08PM +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.
> >>>
> >>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >>> ---
> >>>  include/libcamera/controls.h                  |  5 +++--
> >>>  src/android/camera_capabilities.cpp           |  8 +++----
> >>>  src/android/camera_device.cpp                 | 21 +++++++++----------
> >>>  src/android/camera_hal_manager.cpp            |  2 +-
> >>>  src/cam/main.cpp                              |  4 ++--
> >>>  src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
> >>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  9 ++++----
> >>>  .../pipeline/raspberrypi/raspberrypi.cpp      |  9 ++++----
> >>>  src/qcam/dng_writer.cpp                       | 15 +++++++------
> >>>  9 files changed, 39 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>> index 665bcac1..192be784 100644
> >>> --- a/include/libcamera/controls.h
> >>> +++ b/include/libcamera/controls.h
> >>> @@ -8,6 +8,7 @@
> >>>  #pragma once
> >>>
> >>>  #include <assert.h>
> >>> +#include <optional>
> >>>  #include <set>
> >>>  #include <stdint.h>
> >>>  #include <string>
> >>> @@ -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/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> >>> index 6f197eb8..892bbc2b 100644
> >>> --- a/src/android/camera_capabilities.cpp
> >>> +++ b/src/android/camera_capabilities.cpp
> >>> @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>>  	/* Sensor static metadata. */
> >>>  	std::array<int32_t, 2> pixelArraySize;
> >>>  	{
> >>> -		const Size &size = properties.get(properties::PixelArraySize);
> >>> +		const Size &size = properties.get(properties::PixelArraySize).value_or(Size{});
> >>
> >> We should probably wrap this and a few similar cases above with an
> >>
> >>         if (properties.contains(...))
> >>
> >> Not for this patch though.
> >>
> >>>  		pixelArraySize[0] = size.width;
> >>>  		pixelArraySize[1] = size.height;
> >>>  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> >>> @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>>  	}
> >>>
> >>>  	if (properties.contains(properties::UnitCellSize)) {
> >>> -		const Size &cellSize = properties.get<Size>(properties::UnitCellSize);
> >>> +		const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);
> >
> > Here you could remove the <Size> template argument if I'm not
> > mistaken. I know it was already there...
> >
> > But, most important, I get the following with clang 14.0.0
> >
> > (Showing with both <Size> and without)
> >
> > src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
> >                 const Size &cellSize = *properties.get(properties::UnitCellSize);
> >
> > src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
> >                 const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);
> >
> > I guess the issue might be due to taking a reference to a temporary
> > value as:
> > const Size cellSize = *properties.get(properties::UnitCellSize);
> >
> > as well as:
> > const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;
> >
> > compile fine.
>
> Thanks for pointing this out. I could reproduce this with clang
> ("CC=clang CXX=clang++ meson build -Dandroid=enabled") but not with gcc.
> Even "-Dwarning_level=3" did not show these errors.
>
> Does libcamera run a build server somewhere? This would be helpful to
> discover errors in non-standard configurations (e.g. with clang in this
> case or with "android" enabled).
>

Unfortunately not for public use. I run a buildbot instance as a best
effort service for internal (aka people who can push to master)
pre-integration testing. It's not stable or maintained well enough
for public usage yet.

We do also have the linux-media Jenkins instance, but that only runs
on master afaict

> >
> > When it comes to the usage of value_or(), its documentation says:
> > [1] https://en.cppreference.com/w/cpp/utility/optional/value_or
> > -T must meet the requirements of CopyConstructible in order to use overload (1).
> > -T must meet the requirements of MoveConstructible in order to use overload (2).
> >
> > which means the value is copied or moved back while operator*
> > [2] https://en.cppreference.com/w/cpp/utility/optional/operator*
> > 1) Returns a pointer to the contained value.
> > 2) Returns a reference to the contained value.
> >
> > I guess the issue is due to trying to reference the content of a
> > temporary std::optional<T> instance, created to wrap the return value
> > of ControlValue::get<T>
> >
> >   ControlList:
> > 	template<typename T>
> > 	std::optional<T> get(const Control<T> &ctrl) const
> > 	{
> > 		const ControlValue *val = find(ctrl.id());
> > 		if (!val)
> > 			return std::nullopt;
> >
> > 		return val->get<T>();
> > 	}
> >
> > This should be fine, as the object returned by ControlValue::get<>()
> > refers to valid memory, and as we could take references to it before
> > this series, we should be able to do so now, but the compiler worries
> > that dereferencing the temporary std::optional<> would leave us with a
> > pointer to invalid memory. Is my interpretation of the issue correct ?
> >
>
> Yes, I think what is happening here is that the optional returns the
> pointer to a value that can become "invalid" later. This could happen
> when the "std::optional<T>::reset()" is called or when "std::nullopt" is
> assigned.
>
> Taking the reference of this would be dangerous. We know that the
> reference will "cellSize" be destroyed shortly after, but generally,
> there is no guarantee. For the compiler, it makes sense to complain
> about this.
>
> I only see two ways to solve this:
>
> 1. copy the content:
>
>   const Size cellSize = *properties.get<Size>(properties::UnitCellSize);
>
> 2. use the "std::optional" directly:
>
>   const auto cellSize = properties.get<Size>(properties::UnitCellSize);
>
>   and then access it via the "->" operator:
>
>   cellSize->width
>
> I personally prefer the second option since it avoids copying the entire
> content, which could be expensive for other control types.
>

I agree copies should be avoided as they can become expensive for
larger objects. We also have the issue that using value_or() might
introduce a copy operation that might get unnoticed..

Using std::optional<> directly might get a bit heavy for controls of
type Span<>

		const auto &rects = properties.get(properties::PixelArrayActiveAreas);
		std::vector<int32_t> data{
			static_cast<int32_t>((*rects)[0].x),
			static_cast<int32_t>((*rects)[0].y),
			static_cast<int32_t>((*rects)[0].width),
			static_cast<int32_t>((*rects)[0].height),
		};

Specifically because of the need to dereference the std::optiona<>
before accessing its content with operator[]

It's not something huge, and I still think the series has ways more
pro than cons, but I wonder if we shouln't wrap std::optional<> in
some libcamera::controls specific type and expose that in the public
API instead of the raw std::optional<>

Not asking you to do so for this series of course.

Thanks
   j

>
> >>>  			cellSize.width * pixelArraySize[0] / 1e6f,
> >>>  			cellSize.height * pixelArraySize[1] / 1e6f
> >>> @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>>
> >>>  	{
> >>>  		const Span<const Rectangle> &rects =
> >>> -			properties.get(properties::PixelArrayActiveAreas);
> >>> +			properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
> >>>  		std::vector<int32_t> data{
> >>>  			static_cast<int32_t>(rects[0].x),
> >>>  			static_cast<int32_t>(rects[0].y),
> >>> @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>>
> >>>  	/* Report the color filter arrangement if the camera reports it. */
> >>>  	if (properties.contains(properties::draft::ColorFilterArrangement)) {
> >>> -		uint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);
> >>> +		uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);
> >>>  		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> >>>  					  filterArr);
> >>>  	}
> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>> index 8e804d4d..ec117101 100644
> >>> --- a/src/android/camera_device.cpp
> >>> +++ b/src/android/camera_device.cpp
> >>> @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> >>>  	const ControlList &properties = camera_->properties();
> >>>
> >>>  	if (properties.contains(properties::Location)) {
> >>> -		int32_t location = properties.get(properties::Location);
> >>> +		int32_t location = *properties.get(properties::Location);
> >>>  		switch (location) {
> >>>  		case properties::CameraLocationFront:
> >>>  			facing_ = CAMERA_FACING_FRONT;
> >>> @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> >>>  	 * metadata.
> >>>  	 */
> >>>  	if (properties.contains(properties::Rotation)) {
> >>> -		int rotation = properties.get(properties::Rotation);
> >>> +		int rotation = *properties.get(properties::Rotation);
> >>>  		orientation_ = (360 - rotation) % 360;
> >>>  		if (cameraConfigData && cameraConfigData->rotation != -1 &&
> >>>  		    orientation_ != cameraConfigData->rotation) {
> >>> @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)
> >>>  	 * as soon as possible, earlier than request completion time.
> >>>  	 */
> >>>  	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> >>> -							 .get(controls::SensorTimestamp));
> >>> +								 .get(controls::SensorTimestamp)
> >>> +								 .value_or(0));
> >>>  	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >>>
> >>>  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> >>> @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >>>  				 rolling_shutter_skew);
> >>>
> >>>  	/* Add metadata tags reported by libcamera. */
> >>> -	const int64_t timestamp = metadata.get(controls::SensorTimestamp);
> >>> +	const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);
> >>>  	resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);
> >>>
> >>>  	if (metadata.contains(controls::draft::PipelineDepth)) {
> >>> -		uint8_t pipeline_depth =
> >>> -			metadata.get<int32_t>(controls::draft::PipelineDepth);
> >>> +		uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth);
> >>>  		resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,
> >>>  					 pipeline_depth);
> >>>  	}
> >>>
> >>>  	if (metadata.contains(controls::ExposureTime)) {
> >>> -		int64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;
> >>> +		int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;
> >>>  		resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);
> >>>  	}
> >>>
> >>>  	if (metadata.contains(controls::FrameDuration)) {
> >>> -		int64_t duration = metadata.get(controls::FrameDuration) * 1000;
> >>> +		int64_t duration = *metadata.get(controls::FrameDuration) * 1000;
> >>>  		resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,
> >>>  					 duration);
> >>>  	}
> >>>
> >>>  	if (metadata.contains(controls::ScalerCrop)) {
> >>> -		Rectangle crop = metadata.get(controls::ScalerCrop);
> >>> +		Rectangle crop = *metadata.get(controls::ScalerCrop);
> >>>  		int32_t cropRect[] = {
> >>>  			crop.x, crop.y, static_cast<int32_t>(crop.width),
> >>>  			static_cast<int32_t>(crop.height),
> >>> @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >>>  	}
> >>>
> >>>  	if (metadata.contains(controls::draft::TestPatternMode)) {
> >>> -		const int32_t testPatternMode =
> >>> -			metadata.get(controls::draft::TestPatternMode);
> >>> +		const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);
> >>>  		resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,
> >>>  					 testPatternMode);
> >>>  	}
> >>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> >>> index 5f7bfe26..0b23397a 100644
> >>> --- a/src/android/camera_hal_manager.cpp
> >>> +++ b/src/android/camera_hal_manager.cpp
> >>> @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)
> >>>  	if (!properties.contains(properties::Location))
> >>>  		return -1;
> >>>
> >>> -	return properties.get(properties::Location);
> >>> +	return properties.get(properties::Location).value_or(0);
> >>
> >> No need to value_or() as we return earlier if Location is not
> >> available
> >>
> >>>  }
> >>>
> >>>  CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)
> >>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> >>> index 79875ed7..d8115cd8 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)) {
> >>>  		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) + "' ";
> >>>  	}
> >>>
> >>>  	name += "(" + camera->id() + ")";
> >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >>> index 3b126bb5..f65a0680 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(0);
> >>
> >> This should in theory be guaranteed to be there, as it's the pipeline
> >> handler that populates it. However I'm not sure what's best. Either
> >> ASSERT() that the control is there or default it to 0. frameTimestamp
> >> is not used as a divider anywhere so there's no risk of undefined
> >> behaviour.
> >>
> >>>  	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..53332826 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));
> >>>  		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));
> >>>  	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(0),
> >>>  				 info->statBuffer->cookie(), info->effectiveSensorControls);
> >>
> >> Also in this case we should be guaranteed the sensor timestamp is
> >> there.
> >>
> >> I'm tempted to ASSERT here as well...
> >>
> >>>  }
> >>>
> >>> @@ -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(0);
> >>
> >> No need as we return earlier if !TestPatternMode
> >>
> >>>
> >>>  	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..a62afdd4 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(0);
> >>>  	bool success;
> >>>  	Transform rotationTransform = transformFromRotation(rotation, &success);
> >>>  	if (!success)
> >>> @@ -1706,7 +1706,8 @@ 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);
> >>>  		/* The control wants linear gains in the order B, Gb, Gr, R. */
> >>>  		ControlList ctrls(sensor_->controls());
> >>>  		std::array<int32_t, 4> gains{
> >>> @@ -2041,7 +2042,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);
> >>>
> >>>  		if (!nativeCrop.width || !nativeCrop.height)
> >>>  			nativeCrop = { 0, 0, 1, 1 };
> >>> @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
> >>>  					Request *request)
> >>>  {
> >>>  	request->metadata().set(controls::SensorTimestamp,
> >>> -				bufferControls.get(controls::SensorTimestamp));
> >>> +				bufferControls.get(controls::SensorTimestamp).value_or(0));
> >>>
> >>>  	request->metadata().set(controls::ScalerCrop, scalerCrop_);
> >>>  }
> >>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> >>> index 34c8df5a..780f58f7 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);
> >>>  		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);
> >>>  		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);
> >>>  		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);
> >>>
> >>>  		/*
> >>>  		 * 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);
> >>>  		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) / 1e6;
> >>>  		TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
> >>>  	}
> >>
> >> Let's see what's Laurent comment on the possible assertion, but the
> >> patch looks good to me and the few other nits can be changed when
> >> applying if you agree with them
> >>
> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Thanks
> >>    j
> >>
> >>>
> >>> --
> >>> 2.34.1
> >>>

Hi All,

Quoting Jacopo Mondi via libcamera-devel (2022-06-05 10:13:32)
> > > I guess the issue might be due to taking a reference to a temporary
> > > value as:
> > > const Size cellSize = *properties.get(properties::UnitCellSize);
> > >
> > > as well as:
> > > const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;
> > >
> > > compile fine.
> >
> > Thanks for pointing this out. I could reproduce this with clang
> > ("CC=clang CXX=clang++ meson build -Dandroid=enabled") but not with gcc.
> > Even "-Dwarning_level=3" did not show these errors.
> >
> > Does libcamera run a build server somewhere? This would be helpful to
> > discover errors in non-standard configurations (e.g. with clang in this
> > case or with "android" enabled).
> >
> 
> Unfortunately not for public use. I run a buildbot instance as a best
> effort service for internal (aka people who can push to master)
> pre-integration testing. It's not stable or maintained well enough
> for public usage yet.
> 
> We do also have the linux-media Jenkins instance, but that only runs
> on master afaict

And I have my build machine too.

I think one possible solution would be to get a gitlab instance on
freedesktop.org and add a .gitlab-ci.yml - this could then trigger build
jobs for others to push to a gitlab instance.

I tried setting up and maintaining my own gitlab instance, but that
lasted a week before it fell over :S ... so I don't want to run one
myself. If we can use an external such as F.D.O that will help.

--
Kieran