[libcamera-devel] [PATCH v3 4/4] use std::optional to handle invalid control values
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 19 22:46:25 CEST 2022
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 at 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);
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list