[libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use std::optional to handle invalid control values
Christian Rauch
Rauch.Christian at gmx.de
Fri Jun 3 01:48:15 CEST 2022
Hi Laurent,
Thanks for looking at this.
Am 02.06.22 um 10:58 schrieb Laurent Pinchart:
> Hi Christian,
>
> Thank you for the patch.
>
> On Thu, Jun 02, 2022 at 12:17:59AM +0100, Christian Rauch via libcamera-devel wrote:
>> Previously, ControlList::get<T>() would use default constructed objects to
>> indicate that a ControlList does not have the requested Control. This has
>> several disadvantages: 1) It requires types to be default constructible,
>> 2) it does not differentiate between a default constructed object and an
>> object that happens to have the same state as a default constructed object.
>>
>> std::optional<T> additionally stores the information if the object is valid
>> or not, and therefore is more expressive than a default constructed object.
>
> I think I like this :-)
>
>> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
>> ---
>> include/libcamera/controls.h | 7 ++++---
>> src/cam/main.cpp | 4 ++--
>> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++-----
>> .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++----
>> src/qcam/dng_writer.cpp | 15 +++++++++------
>> 6 files changed, 26 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 665bcac1..363e7809 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -13,6 +13,7 @@
>> #include <string>
>> #include <unordered_map>
>> #include <vector>
>> +#include <optional>
>
> Please keep headers alphabetically sorted. Are you using the
> checkstyle.py style checker ? You can copy utils/hooks/post-commit (or
> pre-commit if preferred) git hook to .git/hooks/ to automate this.
I stopped using the automated checker in my IDE because I got a lot of
"false positives" where it adapted the style according to the
clang-format file, that were later rejected on the mailing list. But I
will note down that this header order should be included.
>
>>
>> #include <libcamera/base/class.h>
>> #include <libcamera/base/span.h>
>> @@ -167,7 +168,7 @@ public:
>>
>> using V = typename T::value_type;
>> const V *value = reinterpret_cast<const V *>(data().data());
>> - return { value, numElements_ };
>> + return T{ value, numElements_ };
>
> This seems unrelated. It makes the code more explicit though so I think
> it's good, but I'd at least mention it in the commit message as a "While
> at it, ..." item.
This might have been a left-over from an earlier version of this patch
set. I am removing this in a future version as it is not necessary.
>
>> }
>>
>> #ifndef __DOXYGEN__
>> @@ -373,11 +374,11 @@ public:
>> bool contains(unsigned int id) const;
>>
>> template<typename T>
>> - T get(const Control<T> &ctrl) const
>> + std::optional<T> get(const Control<T> &ctrl) const
>> {
>> const ControlValue *val = find(ctrl.id());
>> if (!val)
>> - return T{};
>> + return std::nullopt;
>>
>> return val->get<T>();
>> }
>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
>> index 79875ed7..9b773931 100644
>> --- a/src/cam/main.cpp
>> +++ b/src/cam/main.cpp
>> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)
>> * is only used if the location isn't present or is set to External.
>> */
>> if (props.contains(properties::Location)) {
>> - switch (props.get(properties::Location)) {
>> + switch (props.get(properties::Location).value_or(int32_t{})) {
>
> Given that the props.contains() check guarantees that the value is
> present, we could use
>
> switch (*props.get(properties::Location)) {
>
> here. Alternatively, the code could be changed to
>
> const auto location = props.get(properties::Location);
> if (location) {
> switch (*location) {
>
> which avoids the double lookup. I think I like this one better. Any
> opinion from anyone ?
I initially replicated the old behaviour by returning a default
constructed object if the value is not valid. But now, I like your idea
better since "if (location)" makes this much more clear.
>
> The same comment applies to several locations below.
>
>> case properties::CameraLocationFront:
>> addModel = false;
>> name = "Internal front camera ";
>> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)
>> * If the camera location is not availble use the camera model
>> * to build the camera name.
>> */
>> - name = "'" + props.get(properties::Model) + "' ";
>> + name = "'" + props.get(properties::Model).value_or(std::string{}) + "' ";
>
> This brings the question of what we should do for properties (and
> controls below) that are mandatory. One option would be to simply write
>
> name = "'" + *props.get(properties::Model) + "' ";
>
> This leads to undefined behaviour if the property isn't present, which
> isn't very nice. On the other hand, the runtime check is in theory not
> necessary, as the property is mandatory (which should be ensured through
> compliance testing).
>
> If we want to keep the check, I'd like to shorten the line a bit with
>
> name = "'" + props.get(properties::Model).value_or("") + "' ";
I would not use "*props.get()" unless it was checked before that
"props.has_value()" is true. If the compliance test fails to recognise
an undefined "Model" in some situation,s then this will dereference a
NULL pointer, which is hard to debug.
So unless it is checked before that "props" is valid, I would always
fall back to the default value like:
"props.get(<propery>).value_or(<default>)"
>> }
>>
>> name += "(" + camera->id() + ")";
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 3b126bb5..00600a2e 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>>
>> void IPARPi::prepareISP(const ISPConfig &data)
>> {
>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});
>
> Same comment here, we could drop the value_or(), or call value_or(0).
> Same in other locations below where applicable.
Do you want to drop the "value_or" because it is somewhere else
guaranteed that this control exists? Or do you just explicitely set "0"
instead of the default constructed int64_t?
>
>> RPiController::Metadata lastMetadata;
>> Span<uint8_t> embeddedBuffer;
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index fd989e61..1e9e5081 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()
>> /* Convert the sensor rotation to a transformation */
>> int32_t rotation = 0;
>> if (data->properties_.contains(properties::Rotation))
>> - rotation = data->properties_.get(properties::Rotation);
>> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});
>> else
>> LOG(IPU3, Warning) << "Rotation control not exposed by "
>> << cio2->sensor()->id()
>> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>> request->metadata().set(controls::draft::PipelineDepth, 3);
>> /* \todo Actually apply the scaler crop region to the ImgU. */
>> if (request->controls().contains(controls::ScalerCrop))
>> - cropRegion_ = request->controls().get(controls::ScalerCrop);
>> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});
>> request->metadata().set(controls::ScalerCrop, cropRegion_);
>>
>> if (frameInfos_.tryComplete(info))
>> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>> return;
>> }
>>
>> - ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),
>> + ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(int64_t{}),
>> info->statBuffer->cookie(), info->effectiveSensorControls);
>> }
>>
>> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)
>> if (!request->controls().contains(controls::draft::TestPatternMode))
>> return;
>>
>> - const int32_t testPatternMode = request->controls().get(
>> - controls::draft::TestPatternMode);
>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});
>>
>> int ret = cio2_.sensor()->setTestPatternMode(
>> static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index adc397e8..556af882 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>> * error means the platform can never run. Let's just print a warning
>> * and continue regardless; the rotation is effectively set to zero.
>> */
>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);
>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});
>> bool success;
>> Transform rotationTransform = transformFromRotation(rotation, &success);
>> if (!success)
>> @@ -1706,7 +1706,9 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
>> */
>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {
>> - libcamera::Span<const float> colourGains = controls.get(libcamera::controls::ColourGains);
>> + libcamera::Span<const float> colourGains =
>> + controls.get(libcamera::controls::ColourGains).\
>> + value_or(libcamera::Span<const float>({ 0, 0 }));
>> /* The control wants linear gains in the order B, Gb, Gr, R. */
>> ControlList ctrls(sensor_->controls());
>> std::array<int32_t, 4> gains{
>> @@ -2041,7 +2043,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const
>> void RPiCameraData::applyScalerCrop(const ControlList &controls)
>> {
>> if (controls.contains(controls::ScalerCrop)) {
>> - Rectangle nativeCrop = controls.get<Rectangle>(controls::ScalerCrop);
>> + Rectangle nativeCrop = controls.get<Rectangle>(controls::ScalerCrop).value_or(Rectangle{});
>>
>> if (!nativeCrop.width || !nativeCrop.height)
>> nativeCrop = { 0, 0, 1, 1 };
>> @@ -2079,7 +2081,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
>> Request *request)
>> {
>> request->metadata().set(controls::SensorTimestamp,
>> - bufferControls.get(controls::SensorTimestamp));
>> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));
>>
>> request->metadata().set(controls::ScalerCrop, scalerCrop_);
>> }
>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
>> index 34c8df5a..e119ca52 100644
>> --- a/src/qcam/dng_writer.cpp
>> +++ b/src/qcam/dng_writer.cpp
>> @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> TIFFSetField(tif, TIFFTAG_MAKE, "libcamera");
>>
>> if (cameraProperties.contains(properties::Model)) {
>> - std::string model = cameraProperties.get(properties::Model);
>> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});
>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());
>> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */
>> }
>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> const double eps = 1e-2;
>>
>> if (metadata.contains(controls::ColourGains)) {
>> - Span<const float> const &colourGains = metadata.get(controls::ColourGains);
>> + Span<const float> const &colourGains =
>> + metadata.get(controls::ColourGains).value_or(libcamera::Span<const float>({ 0, 0 }));
>> if (colourGains[0] > eps && colourGains[1] > eps) {
>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
>> neutral[0] = 1.0 / colourGains[0]; /* red */
>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> }
>> }
>> if (metadata.contains(controls::ColourCorrectionMatrix)) {
>> - Span<const float> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
>> + Span<const float> const &coeffs =
>> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span<const float>({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));
>> Matrix3d ccmSupplied(coeffs);
>> if (ccmSupplied.determinant() > eps)
>> ccm = ccmSupplied;
>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;
>>
>> if (metadata.contains(controls::SensorBlackLevels)) {
>> - Span<const int32_t> levels = metadata.get(controls::SensorBlackLevels);
>> + Span<const int32_t> levels =
>> + metadata.get(controls::SensorBlackLevels).value_or(Span<const int32_t>({ 0, 0, 0, 0 }));
>>
>> /*
>> * The black levels control is specified in R, Gr, Gb, B order.
>> @@ -593,13 +596,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);
>>
>> if (metadata.contains(controls::AnalogueGain)) {
>> - float gain = metadata.get(controls::AnalogueGain);
>> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});
>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);
>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);
>> }
>>
>> if (metadata.contains(controls::ExposureTime)) {
>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;
>> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;
>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
>> }
>>
>
More information about the libcamera-devel
mailing list