[libcamera-devel,v3,4/4] use std::optional to handle invalid control values

15656 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                      |  6 +++---
 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 +++++++++------
 6 files changed, 24 insertions(+), 21 deletions(-)

--
2.25.1

Hi Christian,

Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)
> 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.

This looks like a really good way to express the controls from a list. I
really like the value_or() that it brings to allow the code to set a
default.


I expect this will have knock-on effects to other out of tree
applications using the control framework so we might want to coordinate
the merge of this.

Though I notice there's fairly minimal changes to cam and qcam. Do you
know if your build includes the v4l2 adaptation layer and gstreamer?
Does this API change cause definate breakage to users?

(It's ok if it does, that's preciesly why we are not ABI stable).


> 
> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> ---
>  include/libcamera/controls.h                      |  6 +++---
>  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 +++++++++------
>  6 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 665bcac1..57b777e9 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -167,7 +167,7 @@ public:
> 
>                 using V = typename T::value_type;
>                 const V *value = reinterpret_cast<const V *>(data().data());
> -               return { value, numElements_ };
> +               return T{ value, numElements_ };
>         }
> 
>  #ifndef __DOXYGEN__
> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -293,7 +293,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{})) {

Is there a way to do this without the value_or() in conditions where the
value has already been guaranteed to exist?

Here we have just checked that the lists contains a
properties::Lcoation, so we 'know' that it will never process the
'_or()' part.

Looking at https://en.cppreference.com/w/cpp/utility/optional

I guess we can just use .value() in the locations where we already check
for the presence. I suspect this could lead to a code refactor to just
use the optional to determine the properties existance instead of
.contains() - but that could certainly be done on top.

Perhaps it might be better for consistency to use the value_or() variant
on occasions though - even if we know it must already exist?


>                 case properties::CameraLocationFront:
>                         addModel = false;
>                         name = "Internal front camera ";
> @@ -313,7 +313,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{}) + "' ";
>         }
> 
>         name += "(" + camera->id() + ")";
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 5a5cdf66..93b32e94 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> 
>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>  {
> -       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> +       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1146,7 +1146,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()
> @@ -1341,7 +1341,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))
> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>         ev.op = ipa::ipu3::EventStatReady;
>         ev.frame = info->id;
>         ev.bufferId = info->statBuffer->cookie();
> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
> +       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
>         ev.sensorControls = info->effectiveSensorControls;
>         ipa_->processEvent(ev);
>  }
> @@ -1477,8 +1477,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{});

This looks like a section of code that could now use optional for
cleaner code I think. I see above we return early if the control is not
present, and only call setTestPatternMode if it is set.


Again, I think this patch is just bringing in the std::optional - so it
shouldn't have to 'make everything use the best implementation' - but I
can see benefits it can bring.

I think even though we know it's guaranteed to exist here, the use of
value_or() is fine with me, as it highlights that this code could
perhaps be simplified later.


> 
>         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 0fa294d4..63d57033 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)
> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
> +               libcamera::Span<const float, 2> colourGains =
> +                       controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
>                 /* The control wants linear gains in the order B, Gb, Gr, R. */
>                 ControlList ctrls(sensor_->controls());
>                 std::array<int32_t, 4> gains{
> @@ -2031,7 +2032,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{});

I'm starting to wonder if a templated get_or would be useful as the type
would be defined there (doesn't have to be here, just an idea)

It would reduce line length on null initialisers:

   controls.get_or<Rectangle>(controls::ScalerCrop, {});

And easily allow default parameters to be defined:

   controls.get_or<Rectangle>(controls::ScalerCrop, {640,480});

I suspect seeing how this all gets used will determine if it has value
though.

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

Hi Kieran,

Am 08.04.22 um 13:06 schrieb Kieran Bingham:
> Hi Christian,
>
> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)
>> 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.
>
> This looks like a really good way to express the controls from a list. I
> really like the value_or() that it brings to allow the code to set a
> default.
>
>
> I expect this will have knock-on effects to other out of tree
> applications using the control framework so we might want to coordinate
> the merge of this.
>
> Though I notice there's fairly minimal changes to cam and qcam. Do you
> know if your build includes the v4l2 adaptation layer and gstreamer?
> Does this API change cause definate breakage to users?
>
> (It's ok if it does, that's preciesly why we are not ABI stable).

This is definitely a breaking change as it changes the public API. But
the changes that have to be made are quite trivial. You only have to add
".value()" or ".value_or(...)" to the old code.

I don't know about the v4l2 wrapper and gstreamer. There might be some
code that is not compiled on my setup. But the "qcam" application still
works. And I think this one is using the v4l2 wrapper.

>
>
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>> ---
>>  include/libcamera/controls.h                      |  6 +++---
>>  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 +++++++++------
>>  6 files changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 665bcac1..57b777e9 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -167,7 +167,7 @@ public:
>>
>>                 using V = typename T::value_type;
>>                 const V *value = reinterpret_cast<const V *>(data().data());
>> -               return { value, numElements_ };
>> +               return T{ value, numElements_ };
>>         }
>>
>>  #ifndef __DOXYGEN__
>> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
>> --- a/src/cam/main.cpp
>> +++ b/src/cam/main.cpp
>> @@ -293,7 +293,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{})) {
>
> Is there a way to do this without the value_or() in conditions where the
> value has already been guaranteed to exist?
>
> Here we have just checked that the lists contains a
> properties::Lcoation, so we 'know' that it will never process the
> '_or()' part.
>
> Looking at https://en.cppreference.com/w/cpp/utility/optional
>
> I guess we can just use .value() in the locations where we already check
> for the presence. I suspect this could lead to a code refactor to just
> use the optional to determine the properties existance instead of
> .contains() - but that could certainly be done on top.
>
> Perhaps it might be better for consistency to use the value_or() variant
> on occasions though - even if we know it must already exist?

".value()" will throw an exception if the "std::optional" does not
contain a value. If you can guarantee that a ControlValue contains a
value, then you can skip the check via ".has_value()" or the fallback
via ".value_or(...)" and use ".value()" directly.

>
>
>>                 case properties::CameraLocationFront:
>>                         addModel = false;
>>                         name = "Internal front camera ";
>> @@ -313,7 +313,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{}) + "' ";
>>         }
>>
>>         name += "(" + camera->id() + ")";
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 5a5cdf66..93b32e94 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>>
>>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>>  {
>> -       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
>> +       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -1146,7 +1146,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()
>> @@ -1341,7 +1341,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))
>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>         ev.op = ipa::ipu3::EventStatReady;
>>         ev.frame = info->id;
>>         ev.bufferId = info->statBuffer->cookie();
>> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
>> +       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
>>         ev.sensorControls = info->effectiveSensorControls;
>>         ipa_->processEvent(ev);
>>  }
>> @@ -1477,8 +1477,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{});
>
> This looks like a section of code that could now use optional for
> cleaner code I think. I see above we return early if the control is not
> present, and only call setTestPatternMode if it is set.
>
>
> Again, I think this patch is just bringing in the std::optional - so it
> shouldn't have to 'make everything use the best implementation' - but I
> can see benefits it can bring.
>
> I think even though we know it's guaranteed to exist here, the use of
> value_or() is fine with me, as it highlights that this code could
> perhaps be simplified later.
>

The current ".value_or(...)" implementation is the closest to the old
behaviour, which would return a default contructed object in case of
failure. You certainly can change that behaviour if you arec ertain that
a value exists.

>
>>
>>         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 0fa294d4..63d57033 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)
>> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
>> +               libcamera::Span<const float, 2> colourGains =
>> +                       controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
>>                 /* The control wants linear gains in the order B, Gb, Gr, R. */
>>                 ControlList ctrls(sensor_->controls());
>>                 std::array<int32_t, 4> gains{
>> @@ -2031,7 +2032,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{});
>
> I'm starting to wonder if a templated get_or would be useful as the type
> would be defined there (doesn't have to be here, just an idea)
>
> It would reduce line length on null initialisers:
>
>    controls.get_or<Rectangle>(controls::ScalerCrop, {});
>
> And easily allow default parameters to be defined:
>
>    controls.get_or<Rectangle>(controls::ScalerCrop, {640,480});
>
> I suspect seeing how this all gets used will determine if it has value
> though.

This is probably dependent on the situation, but I don't think that
initialising control values with the default is a good idea in every
case. The biggest advantage of "std::optional" is that you can properly
test for errors. In most cases, it is probably better to notify the user
about missing controls etc. instead of silently replacing the requested
values with the defaults.

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

Hi Kieran,

Is my patch series, including the std::optional change, something you
would consider? I think it's a useful addition as it properly "types"
the Span Controls and makes the handling of invalid return "get" values
explicit.

Best,
Christian


Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:
> Hi Kieran,
>
> Am 08.04.22 um 13:06 schrieb Kieran Bingham:
>> Hi Christian,
>>
>> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)
>>> 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.
>>
>> This looks like a really good way to express the controls from a list. I
>> really like the value_or() that it brings to allow the code to set a
>> default.
>>
>>
>> I expect this will have knock-on effects to other out of tree
>> applications using the control framework so we might want to coordinate
>> the merge of this.
>>
>> Though I notice there's fairly minimal changes to cam and qcam. Do you
>> know if your build includes the v4l2 adaptation layer and gstreamer?
>> Does this API change cause definate breakage to users?
>>
>> (It's ok if it does, that's preciesly why we are not ABI stable).
>
> This is definitely a breaking change as it changes the public API. But
> the changes that have to be made are quite trivial. You only have to add
> ".value()" or ".value_or(...)" to the old code.
>
> I don't know about the v4l2 wrapper and gstreamer. There might be some
> code that is not compiled on my setup. But the "qcam" application still
> works. And I think this one is using the v4l2 wrapper.
>
>>
>>
>>>
>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>> ---
>>>  include/libcamera/controls.h                      |  6 +++---
>>>  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 +++++++++------
>>>  6 files changed, 24 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> index 665bcac1..57b777e9 100644
>>> --- a/include/libcamera/controls.h
>>> +++ b/include/libcamera/controls.h
>>> @@ -167,7 +167,7 @@ public:
>>>
>>>                 using V = typename T::value_type;
>>>                 const V *value = reinterpret_cast<const V *>(data().data());
>>> -               return { value, numElements_ };
>>> +               return T{ value, numElements_ };
>>>         }
>>>
>>>  #ifndef __DOXYGEN__
>>> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
>>> --- a/src/cam/main.cpp
>>> +++ b/src/cam/main.cpp
>>> @@ -293,7 +293,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{})) {
>>
>> Is there a way to do this without the value_or() in conditions where the
>> value has already been guaranteed to exist?
>>
>> Here we have just checked that the lists contains a
>> properties::Lcoation, so we 'know' that it will never process the
>> '_or()' part.
>>
>> Looking at https://en.cppreference.com/w/cpp/utility/optional
>>
>> I guess we can just use .value() in the locations where we already check
>> for the presence. I suspect this could lead to a code refactor to just
>> use the optional to determine the properties existance instead of
>> .contains() - but that could certainly be done on top.
>>
>> Perhaps it might be better for consistency to use the value_or() variant
>> on occasions though - even if we know it must already exist?
>
> ".value()" will throw an exception if the "std::optional" does not
> contain a value. If you can guarantee that a ControlValue contains a
> value, then you can skip the check via ".has_value()" or the fallback
> via ".value_or(...)" and use ".value()" directly.
>
>>
>>
>>>                 case properties::CameraLocationFront:
>>>                         addModel = false;
>>>                         name = "Internal front camera ";
>>> @@ -313,7 +313,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{}) + "' ";
>>>         }
>>>
>>>         name += "(" + camera->id() + ")";
>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>> index 5a5cdf66..93b32e94 100644
>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>>>
>>>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>>>  {
>>> -       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
>>> +       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -1146,7 +1146,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()
>>> @@ -1341,7 +1341,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))
>>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>         ev.op = ipa::ipu3::EventStatReady;
>>>         ev.frame = info->id;
>>>         ev.bufferId = info->statBuffer->cookie();
>>> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
>>> +       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
>>>         ev.sensorControls = info->effectiveSensorControls;
>>>         ipa_->processEvent(ev);
>>>  }
>>> @@ -1477,8 +1477,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{});
>>
>> This looks like a section of code that could now use optional for
>> cleaner code I think. I see above we return early if the control is not
>> present, and only call setTestPatternMode if it is set.
>>
>>
>> Again, I think this patch is just bringing in the std::optional - so it
>> shouldn't have to 'make everything use the best implementation' - but I
>> can see benefits it can bring.
>>
>> I think even though we know it's guaranteed to exist here, the use of
>> value_or() is fine with me, as it highlights that this code could
>> perhaps be simplified later.
>>
>
> The current ".value_or(...)" implementation is the closest to the old
> behaviour, which would return a default contructed object in case of
> failure. You certainly can change that behaviour if you arec ertain that
> a value exists.
>
>>
>>>
>>>         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 0fa294d4..63d57033 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)
>>> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
>>> +               libcamera::Span<const float, 2> colourGains =
>>> +                       controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
>>>                 /* The control wants linear gains in the order B, Gb, Gr, R. */
>>>                 ControlList ctrls(sensor_->controls());
>>>                 std::array<int32_t, 4> gains{
>>> @@ -2031,7 +2032,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{});
>>
>> I'm starting to wonder if a templated get_or would be useful as the type
>> would be defined there (doesn't have to be here, just an idea)
>>
>> It would reduce line length on null initialisers:
>>
>>    controls.get_or<Rectangle>(controls::ScalerCrop, {});
>>
>> And easily allow default parameters to be defined:
>>
>>    controls.get_or<Rectangle>(controls::ScalerCrop, {640,480});
>>
>> I suspect seeing how this all gets used will determine if it has value
>> though.
>
> This is probably dependent on the situation, but I don't think that
> initialising control values with the default is a good idea in every
> case. The biggest advantage of "std::optional" is that you can properly
> test for errors. In most cases, it is probably better to notify the user
> about missing controls etc. instead of silently replacing the requested
> values with the defaults.
>
>>
>>>
>>>                 if (!nativeCrop.width || !nativeCrop.height)
>>>                         nativeCrop = { 0, 0, 1, 1 };
>>> @@ -2069,7 +2070,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 2fb527d8..030432e3 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, 2> const &colourGains = metadata.get(controls::ColourGains);
>>> +               Span<const float, 2> const &colourGains =
>>> +                       metadata.get(controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
>>>                 if (colourGains[0] > eps && colourGains[1] > eps) {
>>>                         wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
>>>                         neutral[0] = 1.0 / colourGains[0]; /* red */
>>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>>>                 }
>>>         }
>>>         if (metadata.contains(controls::ColourCorrectionMatrix)) {
>>> -               Span<const float, 9> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
>>> +               Span<const float, 9> const &coeffs =
>>> +                       metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float, 9>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
>>>                 Matrix3d ccmSupplied(coeffs);
>>>                 if (ccmSupplied.determinant() > eps)
>>>                         ccm = ccmSupplied;
>>> @@ -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, 4> levels = metadata.get(controls::SensorBlackLevels);
>>> +               Span<const int32_t, 4> levels =
>>> +                       metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t, 4>({ 0, 0, 0, 0 }));
>>>
>>>                 /*
>>>                  * The black levels control is specified in R, Gr, Gb, B order.
>>> @@ -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);
>>>         }
>>>
>>> --
>>> 2.25.1
>>>

Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)
> Hi Kieran,
> 
> Is my patch series, including the std::optional change, something you
> would consider? I think it's a useful addition as it properly "types"
> the Span Controls and makes the handling of invalid return "get" values
> explicit.
> 

I think overall it sounded good - but I think Laurent mentioned he has
some concerens about std::optional in public API, as we may have some
limitations there, that are preventing us having a full C++17 usage.

I can't recall what is holding us to C++14 on public API - but I would
hope we can look at what is required to bring that up to '17.

> Best,
> Christian
> 
> 
> Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:
> > Hi Kieran,
> >
> > Am 08.04.22 um 13:06 schrieb Kieran Bingham:
> >> Hi Christian,
> >>
> >> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)
> >>> 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.
> >>
> >> This looks like a really good way to express the controls from a list. I
> >> really like the value_or() that it brings to allow the code to set a
> >> default.
> >>
> >>
> >> I expect this will have knock-on effects to other out of tree
> >> applications using the control framework so we might want to coordinate
> >> the merge of this.
> >>
> >> Though I notice there's fairly minimal changes to cam and qcam. Do you
> >> know if your build includes the v4l2 adaptation layer and gstreamer?
> >> Does this API change cause definate breakage to users?
> >>
> >> (It's ok if it does, that's preciesly why we are not ABI stable).
> >
> > This is definitely a breaking change as it changes the public API. But
> > the changes that have to be made are quite trivial. You only have to add
> > ".value()" or ".value_or(...)" to the old code.
> >
> > I don't know about the v4l2 wrapper and gstreamer. There might be some
> > code that is not compiled on my setup. But the "qcam" application still
> > works. And I think this one is using the v4l2 wrapper.
> >
> >>
> >>
> >>>
> >>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >>> ---
> >>>  include/libcamera/controls.h                      |  6 +++---
> >>>  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 +++++++++------
> >>>  6 files changed, 24 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>> index 665bcac1..57b777e9 100644
> >>> --- a/include/libcamera/controls.h
> >>> +++ b/include/libcamera/controls.h
> >>> @@ -167,7 +167,7 @@ public:
> >>>
> >>>                 using V = typename T::value_type;
> >>>                 const V *value = reinterpret_cast<const V *>(data().data());
> >>> -               return { value, numElements_ };
> >>> +               return T{ value, numElements_ };
> >>>         }
> >>>
> >>>  #ifndef __DOXYGEN__
> >>> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
> >>> --- a/src/cam/main.cpp
> >>> +++ b/src/cam/main.cpp
> >>> @@ -293,7 +293,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{})) {
> >>
> >> Is there a way to do this without the value_or() in conditions where the
> >> value has already been guaranteed to exist?
> >>
> >> Here we have just checked that the lists contains a
> >> properties::Lcoation, so we 'know' that it will never process the
> >> '_or()' part.
> >>
> >> Looking at https://en.cppreference.com/w/cpp/utility/optional
> >>
> >> I guess we can just use .value() in the locations where we already check
> >> for the presence. I suspect this could lead to a code refactor to just
> >> use the optional to determine the properties existance instead of
> >> .contains() - but that could certainly be done on top.
> >>
> >> Perhaps it might be better for consistency to use the value_or() variant
> >> on occasions though - even if we know it must already exist?
> >
> > ".value()" will throw an exception if the "std::optional" does not
> > contain a value. If you can guarantee that a ControlValue contains a
> > value, then you can skip the check via ".has_value()" or the fallback
> > via ".value_or(...)" and use ".value()" directly.
> >
> >>
> >>
> >>>                 case properties::CameraLocationFront:
> >>>                         addModel = false;
> >>>                         name = "Internal front camera ";
> >>> @@ -313,7 +313,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{}) + "' ";
> >>>         }
> >>>
> >>>         name += "(" + camera->id() + ")";
> >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >>> index 5a5cdf66..93b32e94 100644
> >>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >>>
> >>>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >>>  {
> >>> -       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> >>> +       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -1146,7 +1146,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()
> >>> @@ -1341,7 +1341,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))
> >>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> >>>         ev.op = ipa::ipu3::EventStatReady;
> >>>         ev.frame = info->id;
> >>>         ev.bufferId = info->statBuffer->cookie();
> >>> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
> >>> +       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
> >>>         ev.sensorControls = info->effectiveSensorControls;
> >>>         ipa_->processEvent(ev);
> >>>  }
> >>> @@ -1477,8 +1477,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{});
> >>
> >> This looks like a section of code that could now use optional for
> >> cleaner code I think. I see above we return early if the control is not
> >> present, and only call setTestPatternMode if it is set.
> >>
> >>
> >> Again, I think this patch is just bringing in the std::optional - so it
> >> shouldn't have to 'make everything use the best implementation' - but I
> >> can see benefits it can bring.
> >>
> >> I think even though we know it's guaranteed to exist here, the use of
> >> value_or() is fine with me, as it highlights that this code could
> >> perhaps be simplified later.
> >>
> >
> > The current ".value_or(...)" implementation is the closest to the old
> > behaviour, which would return a default contructed object in case of
> > failure. You certainly can change that behaviour if you arec ertain that
> > a value exists.
> >
> >>
> >>>
> >>>         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 0fa294d4..63d57033 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)
> >>> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
> >>> +               libcamera::Span<const float, 2> colourGains =
> >>> +                       controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
> >>>                 /* The control wants linear gains in the order B, Gb, Gr, R. */
> >>>                 ControlList ctrls(sensor_->controls());
> >>>                 std::array<int32_t, 4> gains{
> >>> @@ -2031,7 +2032,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{});
> >>
> >> I'm starting to wonder if a templated get_or would be useful as the type
> >> would be defined there (doesn't have to be here, just an idea)
> >>
> >> It would reduce line length on null initialisers:
> >>
> >>    controls.get_or<Rectangle>(controls::ScalerCrop, {});
> >>
> >> And easily allow default parameters to be defined:
> >>
> >>    controls.get_or<Rectangle>(controls::ScalerCrop, {640,480});
> >>
> >> I suspect seeing how this all gets used will determine if it has value
> >> though.
> >
> > This is probably dependent on the situation, but I don't think that
> > initialising control values with the default is a good idea in every
> > case. The biggest advantage of "std::optional" is that you can properly
> > test for errors. In most cases, it is probably better to notify the user
> > about missing controls etc. instead of silently replacing the requested
> > values with the defaults.
> >
> >>
> >>>
> >>>                 if (!nativeCrop.width || !nativeCrop.height)
> >>>                         nativeCrop = { 0, 0, 1, 1 };
> >>> @@ -2069,7 +2070,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 2fb527d8..030432e3 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, 2> const &colourGains = metadata.get(controls::ColourGains);
> >>> +               Span<const float, 2> const &colourGains =
> >>> +                       metadata.get(controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
> >>>                 if (colourGains[0] > eps && colourGains[1] > eps) {
> >>>                         wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
> >>>                         neutral[0] = 1.0 / colourGains[0]; /* red */
> >>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>>                 }
> >>>         }
> >>>         if (metadata.contains(controls::ColourCorrectionMatrix)) {
> >>> -               Span<const float, 9> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
> >>> +               Span<const float, 9> const &coeffs =
> >>> +                       metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float, 9>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
> >>>                 Matrix3d ccmSupplied(coeffs);
> >>>                 if (ccmSupplied.determinant() > eps)
> >>>                         ccm = ccmSupplied;
> >>> @@ -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, 4> levels = metadata.get(controls::SensorBlackLevels);
> >>> +               Span<const int32_t, 4> levels =
> >>> +                       metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t, 4>({ 0, 0, 0, 0 }));
> >>>
> >>>                 /*
> >>>                  * The black levels control is specified in R, Gr, Gb, B order.
> >>> @@ -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);
> >>>         }
> >>>
> >>> --
> >>> 2.25.1
> >>>

Hi Christian and Kieran,

On Tue, Apr 19, 2022 at 11:48:53AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)
> > Hi Kieran,
> > 
> > Is my patch series, including the std::optional change, something you
> > would consider? I think it's a useful addition as it properly "types"
> > the Span Controls and makes the handling of invalid return "get" values
> > explicit.
> 
> I think overall it sounded good - but I think Laurent mentioned he has
> some concerens about std::optional in public API, as we may have some
> limitations there, that are preventing us having a full C++17 usage.
> 
> I can't recall what is holding us to C++14 on public API - but I would
> hope we can look at what is required to bring that up to '17.

I like where this is headed, but my concern is indeed the dependency on
C++17. We've refrained from requiring C++17 due to Chromium being
compiled with C++14 (we've actually briefly switched to C++17 and then
reverted to C++14 when we noticed the compilation failures). Chromium
seems to now support C++17, but can we assume everything else (or at
least everything else that matters) does ? How about OpenCV for instance
?

I also have a feeling that we could combine the existing ControlValue
class with std::optional to achieve a similar result, but I haven't
looked into it in details.

> > Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:
> > > Am 08.04.22 um 13:06 schrieb Kieran Bingham:
> > >> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)
> > >>> 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.
> > >>
> > >> This looks like a really good way to express the controls from a list. I
> > >> really like the value_or() that it brings to allow the code to set a
> > >> default.
> > >>
> > >>
> > >> I expect this will have knock-on effects to other out of tree
> > >> applications using the control framework so we might want to coordinate
> > >> the merge of this.
> > >>
> > >> Though I notice there's fairly minimal changes to cam and qcam. Do you
> > >> know if your build includes the v4l2 adaptation layer and gstreamer?
> > >> Does this API change cause definate breakage to users?
> > >>
> > >> (It's ok if it does, that's preciesly why we are not ABI stable).
> > >
> > > This is definitely a breaking change as it changes the public API. But
> > > the changes that have to be made are quite trivial. You only have to add
> > > ".value()" or ".value_or(...)" to the old code.
> > >
> > > I don't know about the v4l2 wrapper and gstreamer. There might be some
> > > code that is not compiled on my setup. But the "qcam" application still
> > > works. And I think this one is using the v4l2 wrapper.
> > >
> > >>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > >>> ---
> > >>>  include/libcamera/controls.h                      |  6 +++---
> > >>>  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 +++++++++------
> > >>>  6 files changed, 24 insertions(+), 21 deletions(-)
> > >>>
> > >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > >>> index 665bcac1..57b777e9 100644
> > >>> --- a/include/libcamera/controls.h
> > >>> +++ b/include/libcamera/controls.h
> > >>> @@ -167,7 +167,7 @@ public:
> > >>>
> > >>>                 using V = typename T::value_type;
> > >>>                 const V *value = reinterpret_cast<const V *>(data().data());
> > >>> -               return { value, numElements_ };
> > >>> +               return T{ value, numElements_ };
> > >>>         }
> > >>>
> > >>>  #ifndef __DOXYGEN__
> > >>> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
> > >>> --- a/src/cam/main.cpp
> > >>> +++ b/src/cam/main.cpp
> > >>> @@ -293,7 +293,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{})) {
> > >>
> > >> Is there a way to do this without the value_or() in conditions where the
> > >> value has already been guaranteed to exist?
> > >>
> > >> Here we have just checked that the lists contains a
> > >> properties::Lcoation, so we 'know' that it will never process the
> > >> '_or()' part.
> > >>
> > >> Looking at https://en.cppreference.com/w/cpp/utility/optional
> > >>
> > >> I guess we can just use .value() in the locations where we already check
> > >> for the presence. I suspect this could lead to a code refactor to just
> > >> use the optional to determine the properties existance instead of
> > >> .contains() - but that could certainly be done on top.
> > >>
> > >> Perhaps it might be better for consistency to use the value_or() variant
> > >> on occasions though - even if we know it must already exist?
> > >
> > > ".value()" will throw an exception if the "std::optional" does not
> > > contain a value. If you can guarantee that a ControlValue contains a
> > > value, then you can skip the check via ".has_value()" or the fallback
> > > via ".value_or(...)" and use ".value()" directly.
> > >
> > >>>                 case properties::CameraLocationFront:
> > >>>                         addModel = false;
> > >>>                         name = "Internal front camera ";
> > >>> @@ -313,7 +313,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{}) + "' ";
> > >>>         }
> > >>>
> > >>>         name += "(" + camera->id() + ")";
> > >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > >>> index 5a5cdf66..93b32e94 100644
> > >>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > >>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > >>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > >>>
> > >>>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> > >>>  {
> > >>> -       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> > >>> +       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
> > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >>> @@ -1146,7 +1146,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()
> > >>> @@ -1341,7 +1341,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))
> > >>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > >>>         ev.op = ipa::ipu3::EventStatReady;
> > >>>         ev.frame = info->id;
> > >>>         ev.bufferId = info->statBuffer->cookie();
> > >>> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
> > >>> +       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
> > >>>         ev.sensorControls = info->effectiveSensorControls;
> > >>>         ipa_->processEvent(ev);
> > >>>  }
> > >>> @@ -1477,8 +1477,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{});
> > >>
> > >> This looks like a section of code that could now use optional for
> > >> cleaner code I think. I see above we return early if the control is not
> > >> present, and only call setTestPatternMode if it is set.
> > >>
> > >>
> > >> Again, I think this patch is just bringing in the std::optional - so it
> > >> shouldn't have to 'make everything use the best implementation' - but I
> > >> can see benefits it can bring.
> > >>
> > >> I think even though we know it's guaranteed to exist here, the use of
> > >> value_or() is fine with me, as it highlights that this code could
> > >> perhaps be simplified later.
> > >
> > > The current ".value_or(...)" implementation is the closest to the old
> > > behaviour, which would return a default contructed object in case of
> > > failure. You certainly can change that behaviour if you arec ertain that
> > > a value exists.
> > >
> > >>>
> > >>>         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 0fa294d4..63d57033 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)
> > >>> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
> > >>> +               libcamera::Span<const float, 2> colourGains =
> > >>> +                       controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
> > >>>                 /* The control wants linear gains in the order B, Gb, Gr, R. */
> > >>>                 ControlList ctrls(sensor_->controls());
> > >>>                 std::array<int32_t, 4> gains{
> > >>> @@ -2031,7 +2032,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{});
> > >>
> > >> I'm starting to wonder if a templated get_or would be useful as the type
> > >> would be defined there (doesn't have to be here, just an idea)
> > >>
> > >> It would reduce line length on null initialisers:
> > >>
> > >>    controls.get_or<Rectangle>(controls::ScalerCrop, {});
> > >>
> > >> And easily allow default parameters to be defined:
> > >>
> > >>    controls.get_or<Rectangle>(controls::ScalerCrop, {640,480});
> > >>
> > >> I suspect seeing how this all gets used will determine if it has value
> > >> though.
> > >
> > > This is probably dependent on the situation, but I don't think that
> > > initialising control values with the default is a good idea in every
> > > case. The biggest advantage of "std::optional" is that you can properly
> > > test for errors. In most cases, it is probably better to notify the user
> > > about missing controls etc. instead of silently replacing the requested
> > > values with the defaults.
> > >
> > >>>
> > >>>                 if (!nativeCrop.width || !nativeCrop.height)
> > >>>                         nativeCrop = { 0, 0, 1, 1 };
> > >>> @@ -2069,7 +2070,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 2fb527d8..030432e3 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, 2> const &colourGains = metadata.get(controls::ColourGains);
> > >>> +               Span<const float, 2> const &colourGains =
> > >>> +                       metadata.get(controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
> > >>>                 if (colourGains[0] > eps && colourGains[1] > eps) {
> > >>>                         wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
> > >>>                         neutral[0] = 1.0 / colourGains[0]; /* red */
> > >>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> > >>>                 }
> > >>>         }
> > >>>         if (metadata.contains(controls::ColourCorrectionMatrix)) {
> > >>> -               Span<const float, 9> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
> > >>> +               Span<const float, 9> const &coeffs =
> > >>> +                       metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float, 9>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
> > >>>                 Matrix3d ccmSupplied(coeffs);
> > >>>                 if (ccmSupplied.determinant() > eps)
> > >>>                         ccm = ccmSupplied;
> > >>> @@ -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, 4> levels = metadata.get(controls::SensorBlackLevels);
> > >>> +               Span<const int32_t, 4> levels =
> > >>> +                       metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t, 4>({ 0, 0, 0, 0 }));
> > >>>
> > >>>                 /*
> > >>>                  * The black levels control is specified in R, Gr, Gb, B order.
> > >>> @@ -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,

Thank you for the patch.

On Fri, Apr 08, 2022 at 02:42:31AM +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                      |  6 +++---
>  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 +++++++++------
>  6 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 665bcac1..57b777e9 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -167,7 +167,7 @@ public:
> 
>  		using V = typename T::value_type;
>  		const V *value = reinterpret_cast<const V *>(data().data());
> -		return { value, numElements_ };
> +		return T{ value, numElements_ };
>  	}
> 
>  #ifndef __DOXYGEN__
> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -293,7 +293,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{})) {
>  		case properties::CameraLocationFront:
>  			addModel = false;
>  			name = "Internal front camera ";
> @@ -313,7 +313,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{}) + "' ";
>  	}
> 
>  	name += "(" + camera->id() + ")";
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 5a5cdf66..93b32e94 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> 
>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>  {
> -	int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> +	int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1146,7 +1146,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()
> @@ -1341,7 +1341,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))
> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	ev.op = ipa::ipu3::EventStatReady;
>  	ev.frame = info->id;
>  	ev.bufferId = info->statBuffer->cookie();
> -	ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
> +	ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
>  	ev.sensorControls = info->effectiveSensorControls;
>  	ipa_->processEvent(ev);
>  }
> @@ -1477,8 +1477,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 0fa294d4..63d57033 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)
> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
> +		libcamera::Span<const float, 2> colourGains =
> +			controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));

This is too long, we have a strick limit at 120 characters.

Do we need the value_or() here, given that it duplicates the
controls.contains() check ? I would expect either

	if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {
		libcamera::Span<const float, 2> colourGains =
			controls.get(libcamera::controls::ColourGains).value();

or

	std::optional<...> colourGainsCtrl =
		controls.get(libcamera::controls::ColourGains);
	if (notifyGainsUnity_ && colourGainsCtrl) {
		libcamera::Span<const float, 2> colourGains = colourGainsCtrl.value();

Same in quite a few locations below.

>  		/* The control wants linear gains in the order B, Gb, Gr, R. */
>  		ControlList ctrls(sensor_->controls());
>  		std::array<int32_t, 4> gains{
> @@ -2031,7 +2032,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 };
> @@ -2069,7 +2070,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 2fb527d8..030432e3 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, 2> const &colourGains = metadata.get(controls::ColourGains);
> +		Span<const float, 2> const &colourGains =
> +			metadata.get(controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
>  		if (colourGains[0] > eps && colourGains[1] > eps) {
>  			wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
>  			neutral[0] = 1.0 / colourGains[0]; /* red */
> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>  		}
>  	}
>  	if (metadata.contains(controls::ColourCorrectionMatrix)) {
> -		Span<const float, 9> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
> +		Span<const float, 9> const &coeffs =
> +			metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float, 9>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
>  		Matrix3d ccmSupplied(coeffs);
>  		if (ccmSupplied.determinant() > eps)
>  			ccm = ccmSupplied;
> @@ -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, 4> levels = metadata.get(controls::SensorBlackLevels);
> +		Span<const int32_t, 4> levels =
> +			metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t, 4>({ 0, 0, 0, 0 }));
> 
>  		/*
>  		 * The black levels control is specified in R, Gr, Gb, B order.
> @@ -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 and Kieran,

How is libcamera related to Chromium and OpenCV? I don't see any
references to these projects in libcamera code. And do they require to
use the same C++ standard?

Best,
Christian


Am 19.04.22 um 21:46 schrieb Laurent Pinchart:
> Hi Christian and Kieran,
>
> On Tue, Apr 19, 2022 at 11:48:53AM +0100, Kieran Bingham via libcamera-devel wrote:
>> Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)
>>> Hi Kieran,
>>>
>>> Is my patch series, including the std::optional change, something you
>>> would consider? I think it's a useful addition as it properly "types"
>>> the Span Controls and makes the handling of invalid return "get" values
>>> explicit.
>>
>> I think overall it sounded good - but I think Laurent mentioned he has
>> some concerens about std::optional in public API, as we may have some
>> limitations there, that are preventing us having a full C++17 usage.
>>
>> I can't recall what is holding us to C++14 on public API - but I would
>> hope we can look at what is required to bring that up to '17.
>
> I like where this is headed, but my concern is indeed the dependency on
> C++17. We've refrained from requiring C++17 due to Chromium being
> compiled with C++14 (we've actually briefly switched to C++17 and then
> reverted to C++14 when we noticed the compilation failures). Chromium
> seems to now support C++17, but can we assume everything else (or at
> least everything else that matters) does ? How about OpenCV for instance
> ?
>
> I also have a feeling that we could combine the existing ControlValue
> class with std::optional to achieve a similar result, but I haven't
> looked into it in details.
>
>>> Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:
>>>> Am 08.04.22 um 13:06 schrieb Kieran Bingham:
>>>>> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)
>>>>>> 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.
>>>>>
>>>>> This looks like a really good way to express the controls from a list. I
>>>>> really like the value_or() that it brings to allow the code to set a
>>>>> default.
>>>>>
>>>>>
>>>>> I expect this will have knock-on effects to other out of tree
>>>>> applications using the control framework so we might want to coordinate
>>>>> the merge of this.
>>>>>
>>>>> Though I notice there's fairly minimal changes to cam and qcam. Do you
>>>>> know if your build includes the v4l2 adaptation layer and gstreamer?
>>>>> Does this API change cause definate breakage to users?
>>>>>
>>>>> (It's ok if it does, that's preciesly why we are not ABI stable).
>>>>
>>>> This is definitely a breaking change as it changes the public API. But
>>>> the changes that have to be made are quite trivial. You only have to add
>>>> ".value()" or ".value_or(...)" to the old code.
>>>>
>>>> I don't know about the v4l2 wrapper and gstreamer. There might be some
>>>> code that is not compiled on my setup. But the "qcam" application still
>>>> works. And I think this one is using the v4l2 wrapper.
>>>>
>>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>>>>> ---
>>>>>>  include/libcamera/controls.h                      |  6 +++---
>>>>>>  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 +++++++++------
>>>>>>  6 files changed, 24 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>>>>> index 665bcac1..57b777e9 100644
>>>>>> --- a/include/libcamera/controls.h
>>>>>> +++ b/include/libcamera/controls.h
>>>>>> @@ -167,7 +167,7 @@ public:
>>>>>>
>>>>>>                 using V = typename T::value_type;
>>>>>>                 const V *value = reinterpret_cast<const V *>(data().data());
>>>>>> -               return { value, numElements_ };
>>>>>> +               return T{ value, numElements_ };
>>>>>>         }
>>>>>>
>>>>>>  #ifndef __DOXYGEN__
>>>>>> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
>>>>>> --- a/src/cam/main.cpp
>>>>>> +++ b/src/cam/main.cpp
>>>>>> @@ -293,7 +293,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{})) {
>>>>>
>>>>> Is there a way to do this without the value_or() in conditions where the
>>>>> value has already been guaranteed to exist?
>>>>>
>>>>> Here we have just checked that the lists contains a
>>>>> properties::Lcoation, so we 'know' that it will never process the
>>>>> '_or()' part.
>>>>>
>>>>> Looking at https://en.cppreference.com/w/cpp/utility/optional
>>>>>
>>>>> I guess we can just use .value() in the locations where we already check
>>>>> for the presence. I suspect this could lead to a code refactor to just
>>>>> use the optional to determine the properties existance instead of
>>>>> .contains() - but that could certainly be done on top.
>>>>>
>>>>> Perhaps it might be better for consistency to use the value_or() variant
>>>>> on occasions though - even if we know it must already exist?
>>>>
>>>> ".value()" will throw an exception if the "std::optional" does not
>>>> contain a value. If you can guarantee that a ControlValue contains a
>>>> value, then you can skip the check via ".has_value()" or the fallback
>>>> via ".value_or(...)" and use ".value()" directly.
>>>>
>>>>>>                 case properties::CameraLocationFront:
>>>>>>                         addModel = false;
>>>>>>                         name = "Internal front camera ";
>>>>>> @@ -313,7 +313,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{}) + "' ";
>>>>>>         }
>>>>>>
>>>>>>         name += "(" + camera->id() + ")";
>>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>>>>> index 5a5cdf66..93b32e94 100644
>>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>>>>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>>>>>>
>>>>>>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>>>>>>  {
>>>>>> -       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
>>>>>> +       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
>>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>>> @@ -1146,7 +1146,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()
>>>>>> @@ -1341,7 +1341,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))
>>>>>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>>>>         ev.op = ipa::ipu3::EventStatReady;
>>>>>>         ev.frame = info->id;
>>>>>>         ev.bufferId = info->statBuffer->cookie();
>>>>>> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
>>>>>> +       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
>>>>>>         ev.sensorControls = info->effectiveSensorControls;
>>>>>>         ipa_->processEvent(ev);
>>>>>>  }
>>>>>> @@ -1477,8 +1477,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{});
>>>>>
>>>>> This looks like a section of code that could now use optional for
>>>>> cleaner code I think. I see above we return early if the control is not
>>>>> present, and only call setTestPatternMode if it is set.
>>>>>
>>>>>
>>>>> Again, I think this patch is just bringing in the std::optional - so it
>>>>> shouldn't have to 'make everything use the best implementation' - but I
>>>>> can see benefits it can bring.
>>>>>
>>>>> I think even though we know it's guaranteed to exist here, the use of
>>>>> value_or() is fine with me, as it highlights that this code could
>>>>> perhaps be simplified later.
>>>>
>>>> The current ".value_or(...)" implementation is the closest to the old
>>>> behaviour, which would return a default contructed object in case of
>>>> failure. You certainly can change that behaviour if you arec ertain that
>>>> a value exists.
>>>>
>>>>>>
>>>>>>         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 0fa294d4..63d57033 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)
>>>>>> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
>>>>>> +               libcamera::Span<const float, 2> colourGains =
>>>>>> +                       controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
>>>>>>                 /* The control wants linear gains in the order B, Gb, Gr, R. */
>>>>>>                 ControlList ctrls(sensor_->controls());
>>>>>>                 std::array<int32_t, 4> gains{
>>>>>> @@ -2031,7 +2032,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{});
>>>>>
>>>>> I'm starting to wonder if a templated get_or would be useful as the type
>>>>> would be defined there (doesn't have to be here, just an idea)
>>>>>
>>>>> It would reduce line length on null initialisers:
>>>>>
>>>>>    controls.get_or<Rectangle>(controls::ScalerCrop, {});
>>>>>
>>>>> And easily allow default parameters to be defined:
>>>>>
>>>>>    controls.get_or<Rectangle>(controls::ScalerCrop, {640,480});
>>>>>
>>>>> I suspect seeing how this all gets used will determine if it has value
>>>>> though.
>>>>
>>>> This is probably dependent on the situation, but I don't think that
>>>> initialising control values with the default is a good idea in every
>>>> case. The biggest advantage of "std::optional" is that you can properly
>>>> test for errors. In most cases, it is probably better to notify the user
>>>> about missing controls etc. instead of silently replacing the requested
>>>> values with the defaults.
>>>>
>>>>>>
>>>>>>                 if (!nativeCrop.width || !nativeCrop.height)
>>>>>>                         nativeCrop = { 0, 0, 1, 1 };
>>>>>> @@ -2069,7 +2070,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 2fb527d8..030432e3 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, 2> const &colourGains = metadata.get(controls::ColourGains);
>>>>>> +               Span<const float, 2> const &colourGains =
>>>>>> +                       metadata.get(controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
>>>>>>                 if (colourGains[0] > eps && colourGains[1] > eps) {
>>>>>>                         wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
>>>>>>                         neutral[0] = 1.0 / colourGains[0]; /* red */
>>>>>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>>>>>>                 }
>>>>>>         }
>>>>>>         if (metadata.contains(controls::ColourCorrectionMatrix)) {
>>>>>> -               Span<const float, 9> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
>>>>>> +               Span<const float, 9> const &coeffs =
>>>>>> +                       metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float, 9>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
>>>>>>                 Matrix3d ccmSupplied(coeffs);
>>>>>>                 if (ccmSupplied.determinant() > eps)
>>>>>>                         ccm = ccmSupplied;
>>>>>> @@ -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, 4> levels = metadata.get(controls::SensorBlackLevels);
>>>>>> +               Span<const int32_t, 4> levels =
>>>>>> +                       metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t, 4>({ 0, 0, 0, 0 }));
>>>>>>
>>>>>>                 /*
>>>>>>                  * The black levels control is specified in R, Gr, Gb, B order.
>>>>>> @@ -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);
>>>>>>         }
>>>>>>
>

Quoting Christian Rauch via libcamera-devel (2022-04-19 23:34:12)
> Hi Laurent and Kieran,
> 
> How is libcamera related to Chromium and OpenCV? I don't see any
> references to these projects in libcamera code. And do they require to
> use the same C++ standard?

There is some development work that integrates libcamera into chromium,
plus ChromeOS (chromium derived, but separate) builds against libcamera
and can use the libcamera android hal.

However, I don't think we should be worried about being tied to a
Chromium release, as I would expect that to use pipewire in the (near)
future, to talk to libcamera.

OpenCV is currently using C++11 as far as I can tell.... so perhaps that
puts a libcamera integration there at a point that it needs updating
anyway?



> 
> Best,
> Christian
> 
> 
> Am 19.04.22 um 21:46 schrieb Laurent Pinchart:
> > Hi Christian and Kieran,
> >
> > On Tue, Apr 19, 2022 at 11:48:53AM +0100, Kieran Bingham via libcamera-devel wrote:
> >> Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)
> >>> Hi Kieran,
> >>>
> >>> Is my patch series, including the std::optional change, something you
> >>> would consider? I think it's a useful addition as it properly "types"
> >>> the Span Controls and makes the handling of invalid return "get" values
> >>> explicit.
> >>
> >> I think overall it sounded good - but I think Laurent mentioned he has
> >> some concerens about std::optional in public API, as we may have some
> >> limitations there, that are preventing us having a full C++17 usage.
> >>
> >> I can't recall what is holding us to C++14 on public API - but I would
> >> hope we can look at what is required to bring that up to '17.
> >
> > I like where this is headed, but my concern is indeed the dependency on
> > C++17. We've refrained from requiring C++17 due to Chromium being
> > compiled with C++14 (we've actually briefly switched to C++17 and then
> > reverted to C++14 when we noticed the compilation failures). Chromium
> > seems to now support C++17, but can we assume everything else (or at
> > least everything else that matters) does ? How about OpenCV for instance
> > ?
> >
> > I also have a feeling that we could combine the existing ControlValue
> > class with std::optional to achieve a similar result, but I haven't
> > looked into it in details.
> >
> >>> Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:
> >>>> Am 08.04.22 um 13:06 schrieb Kieran Bingham:
> >>>>> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)
> >>>>>> 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.
> >>>>>
> >>>>> This looks like a really good way to express the controls from a list. I
> >>>>> really like the value_or() that it brings to allow the code to set a
> >>>>> default.
> >>>>>
> >>>>>
> >>>>> I expect this will have knock-on effects to other out of tree
> >>>>> applications using the control framework so we might want to coordinate
> >>>>> the merge of this.
> >>>>>
> >>>>> Though I notice there's fairly minimal changes to cam and qcam. Do you
> >>>>> know if your build includes the v4l2 adaptation layer and gstreamer?
> >>>>> Does this API change cause definate breakage to users?
> >>>>>
> >>>>> (It's ok if it does, that's preciesly why we are not ABI stable).
> >>>>
> >>>> This is definitely a breaking change as it changes the public API. But
> >>>> the changes that have to be made are quite trivial. You only have to add
> >>>> ".value()" or ".value_or(...)" to the old code.
> >>>>
> >>>> I don't know about the v4l2 wrapper and gstreamer. There might be some
> >>>> code that is not compiled on my setup. But the "qcam" application still
> >>>> works. And I think this one is using the v4l2 wrapper.
> >>>>
> >>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >>>>>> ---
> >>>>>>  include/libcamera/controls.h                      |  6 +++---
> >>>>>>  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 +++++++++------
> >>>>>>  6 files changed, 24 insertions(+), 21 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>>>>> index 665bcac1..57b777e9 100644
> >>>>>> --- a/include/libcamera/controls.h
> >>>>>> +++ b/include/libcamera/controls.h
> >>>>>> @@ -167,7 +167,7 @@ public:
> >>>>>>
> >>>>>>                 using V = typename T::value_type;
> >>>>>>                 const V *value = reinterpret_cast<const V *>(data().data());
> >>>>>> -               return { value, numElements_ };
> >>>>>> +               return T{ value, numElements_ };
> >>>>>>         }
> >>>>>>
> >>>>>>  #ifndef __DOXYGEN__
> >>>>>> @@ -373,11 +373,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 c7f664b9..853a78ed 100644
> >>>>>> --- a/src/cam/main.cpp
> >>>>>> +++ b/src/cam/main.cpp
> >>>>>> @@ -293,7 +293,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{})) {
> >>>>>
> >>>>> Is there a way to do this without the value_or() in conditions where the
> >>>>> value has already been guaranteed to exist?
> >>>>>
> >>>>> Here we have just checked that the lists contains a
> >>>>> properties::Lcoation, so we 'know' that it will never process the
> >>>>> '_or()' part.
> >>>>>
> >>>>> Looking at https://en.cppreference.com/w/cpp/utility/optional
> >>>>>
> >>>>> I guess we can just use .value() in the locations where we already check
> >>>>> for the presence. I suspect this could lead to a code refactor to just
> >>>>> use the optional to determine the properties existance instead of
> >>>>> .contains() - but that could certainly be done on top.
> >>>>>
> >>>>> Perhaps it might be better for consistency to use the value_or() variant
> >>>>> on occasions though - even if we know it must already exist?
> >>>>
> >>>> ".value()" will throw an exception if the "std::optional" does not
> >>>> contain a value. If you can guarantee that a ControlValue contains a
> >>>> value, then you can skip the check via ".has_value()" or the fallback
> >>>> via ".value_or(...)" and use ".value()" directly.
> >>>>
> >>>>>>                 case properties::CameraLocationFront:
> >>>>>>                         addModel = false;
> >>>>>>                         name = "Internal front camera ";
> >>>>>> @@ -313,7 +313,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{}) + "' ";
> >>>>>>         }
> >>>>>>
> >>>>>>         name += "(" + camera->id() + ")";
> >>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>>> index 5a5cdf66..93b32e94 100644
> >>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >>>>>>
> >>>>>>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >>>>>>  {
> >>>>>> -       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> >>>>>> +       int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(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 60e01917..394221cb 100644
> >>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>>>> @@ -1146,7 +1146,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()
> >>>>>> @@ -1341,7 +1341,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))
> >>>>>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> >>>>>>         ev.op = ipa::ipu3::EventStatReady;
> >>>>>>         ev.frame = info->id;
> >>>>>>         ev.bufferId = info->statBuffer->cookie();
> >>>>>> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
> >>>>>> +       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});
> >>>>>>         ev.sensorControls = info->effectiveSensorControls;
> >>>>>>         ipa_->processEvent(ev);
> >>>>>>  }
> >>>>>> @@ -1477,8 +1477,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{});
> >>>>>
> >>>>> This looks like a section of code that could now use optional for
> >>>>> cleaner code I think. I see above we return early if the control is not
> >>>>> present, and only call setTestPatternMode if it is set.
> >>>>>
> >>>>>
> >>>>> Again, I think this patch is just bringing in the std::optional - so it
> >>>>> shouldn't have to 'make everything use the best implementation' - but I
> >>>>> can see benefits it can bring.
> >>>>>
> >>>>> I think even though we know it's guaranteed to exist here, the use of
> >>>>> value_or() is fine with me, as it highlights that this code could
> >>>>> perhaps be simplified later.
> >>>>
> >>>> The current ".value_or(...)" implementation is the closest to the old
> >>>> behaviour, which would return a default contructed object in case of
> >>>> failure. You certainly can change that behaviour if you arec ertain that
> >>>> a value exists.
> >>>>
> >>>>>>
> >>>>>>         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 0fa294d4..63d57033 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)
> >>>>>> @@ -1696,7 +1696,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, 2> colourGains = controls.get(libcamera::controls::ColourGains);
> >>>>>> +               libcamera::Span<const float, 2> colourGains =
> >>>>>> +                       controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
> >>>>>>                 /* The control wants linear gains in the order B, Gb, Gr, R. */
> >>>>>>                 ControlList ctrls(sensor_->controls());
> >>>>>>                 std::array<int32_t, 4> gains{
> >>>>>> @@ -2031,7 +2032,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{});
> >>>>>
> >>>>> I'm starting to wonder if a templated get_or would be useful as the type
> >>>>> would be defined there (doesn't have to be here, just an idea)
> >>>>>
> >>>>> It would reduce line length on null initialisers:
> >>>>>
> >>>>>    controls.get_or<Rectangle>(controls::ScalerCrop, {});
> >>>>>
> >>>>> And easily allow default parameters to be defined:
> >>>>>
> >>>>>    controls.get_or<Rectangle>(controls::ScalerCrop, {640,480});
> >>>>>
> >>>>> I suspect seeing how this all gets used will determine if it has value
> >>>>> though.
> >>>>
> >>>> This is probably dependent on the situation, but I don't think that
> >>>> initialising control values with the default is a good idea in every
> >>>> case. The biggest advantage of "std::optional" is that you can properly
> >>>> test for errors. In most cases, it is probably better to notify the user
> >>>> about missing controls etc. instead of silently replacing the requested
> >>>> values with the defaults.
> >>>>
> >>>>>>
> >>>>>>                 if (!nativeCrop.width || !nativeCrop.height)
> >>>>>>                         nativeCrop = { 0, 0, 1, 1 };
> >>>>>> @@ -2069,7 +2070,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 2fb527d8..030432e3 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, 2> const &colourGains = metadata.get(controls::ColourGains);
> >>>>>> +               Span<const float, 2> const &colourGains =
> >>>>>> +                       metadata.get(controls::ColourGains).value_or(libcamera::Span<const float, 2>({ 0, 0 }));
> >>>>>>                 if (colourGains[0] > eps && colourGains[1] > eps) {
> >>>>>>                         wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
> >>>>>>                         neutral[0] = 1.0 / colourGains[0]; /* red */
> >>>>>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>>>>>                 }
> >>>>>>         }
> >>>>>>         if (metadata.contains(controls::ColourCorrectionMatrix)) {
> >>>>>> -               Span<const float, 9> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
> >>>>>> +               Span<const float, 9> const &coeffs =
> >>>>>> +                       metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float, 9>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
> >>>>>>                 Matrix3d ccmSupplied(coeffs);
> >>>>>>                 if (ccmSupplied.determinant() > eps)
> >>>>>>                         ccm = ccmSupplied;
> >>>>>> @@ -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, 4> levels = metadata.get(controls::SensorBlackLevels);
> >>>>>> +               Span<const int32_t, 4> levels =
> >>>>>> +                       metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t, 4>({ 0, 0, 0, 0 }));
> >>>>>>
> >>>>>>                 /*
> >>>>>>                  * The black levels control is specified in R, Gr, Gb, B order.
> >>>>>> @@ -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);
> >>>>>>         }
> >>>>>>
> >