[libcamera-devel] [PATCH v3 4/4] use std::optional to handle invalid control values
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 18 01:53:36 CEST 2022
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 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{})) {
> >>>>>
> >>>>> 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);
> >>>>>> }
> >>>>>>
> >
More information about the libcamera-devel
mailing list