[libcamera-devel] [PATCH v8 1/4] libcamera: controls: Use std::optional to handle invalid control values
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 4 00:05:46 CEST 2022
On Mon, Jul 04, 2022 at 12:45:19AM +0300, Laurent Pinchart wrote:
> Hi Christian,
>
> Thank you for the patch.
>
> On Fri, Jun 10, 2022 at 01:03:35PM +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>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/controls.h | 5 +++--
> > src/android/camera_capabilities.cpp | 12 +++++-----
> > src/android/camera_device.cpp | 21 +++++++++---------
> > src/android/camera_hal_manager.cpp | 2 +-
> > src/cam/main.cpp | 4 ++--
> > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++----
> > .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++++----
> > src/qcam/dng_writer.cpp | 22 +++++++++----------
> > 9 files changed, 43 insertions(+), 43 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 665bcac1..192be784 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -8,6 +8,7 @@
> > #pragma once
> >
> > #include <assert.h>
> > +#include <optional>
> > #include <set>
> > #include <stdint.h>
> > #include <string>
> > @@ -373,11 +374,11 @@ public:
> > bool contains(unsigned int id) const;
> >
> > template<typename T>
> > - T get(const Control<T> &ctrl) const
> > + std::optional<T> get(const Control<T> &ctrl) const
> > {
> > const ControlValue *val = find(ctrl.id());
> > if (!val)
> > - return T{};
> > + return std::nullopt;
> >
> > return val->get<T>();
>
> I think one of the perks of using std::optional here is that we could
> return a reference to the value instead of copying it. To avoid a v9,
> I'll send a patch on top.
>
> > }
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index 6f197eb8..5304b2da 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()
> > /* Sensor static metadata. */
> > std::array<int32_t, 2> pixelArraySize;
> > {
> > - const Size &size = properties.get(properties::PixelArraySize);
> > + const Size &size = properties.get(properties::PixelArraySize).value_or(Size{});
> > pixelArraySize[0] = size.width;
> > pixelArraySize[1] = size.height;
> > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > @@ -1050,10 +1050,10 @@ int CameraCapabilities::initializeStaticMetadata()
> > }
> >
> > if (properties.contains(properties::UnitCellSize)) {
> > - const Size &cellSize = properties.get<Size>(properties::UnitCellSize);
> > + const auto &cellSize = properties.get<Size>(properties::UnitCellSize);
>
> We can avoid the double-lookup here and in several locations below. I'll
> send a patch on top too.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Turns out I spoke a bit too fast :-( When compiling with gcc 10.3.1, I'm
getting
[27/218] Compiling C++ object src/android/libcamera-hal.so.p/camera_capabilities.cpp.o
FAILED: src/android/libcamera-hal.so.p/camera_capabilities.cpp.o
g++-10.3.1 -Isrc/android/libcamera-hal.so.p -Isrc/android -I../../src/android -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude -I../../include -I../../subprojects/libyuv/include -Isubprojects/libyuv/__CMake_build -I../../subprojects/libyuv/__CMake_build -Isubprojects/libyuv -I../../subprojects/libyuv -Iinclude/libcamera/ipa -Iinclude/libcamera -I/usr/include/libexif -fdiagnostics-color=always -fsanitize=address -fno-omit-frame-pointer -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -O3 -Wshadow -include config.h -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/android/libcamera-hal.so.p/camera_capabilities.cpp.o -MF src/android/libcamera-hal.so.p/camera_capabilities.cpp.o.d -o src/android/libcamera-hal.so.p/camera_capabilities.cpp.o -c ../../src/android/camera_capabilities.cpp
In file included from ../../include/libcamera/camera.h:20,
from ../../src/android/camera_capabilities.h:17,
from ../../src/android/camera_capabilities.cpp:8:
../../include/libcamera/controls.h: In member function ‘int CameraCapabilities::initializeStaticMetadata()’:
../../include/libcamera/controls.h:381:16: error: ‘<anonymous>’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
381 | return std::nullopt;
| ^~~~~~~
../../include/libcamera/controls.h:381:16: error: ‘*((void*)&<anonymous> +4)’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
The following change fixes it:
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index 5304b2da1f93..429054ae3633 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -1049,8 +1049,8 @@ int CameraCapabilities::initializeStaticMetadata()
pixelArraySize);
}
- if (properties.contains(properties::UnitCellSize)) {
- const auto &cellSize = properties.get<Size>(properties::UnitCellSize);
+ const auto &cellSize = properties.get<Size>(properties::UnitCellSize);
+ if (cellSize) {
std::array<float, 2> physicalSize{
cellSize->width * pixelArraySize[0] / 1e6f,
cellSize->height * pixelArraySize[1] / 1e6f
If we do that, I'd like to apply the same change through this patch. I
can post a v9 of this patch with this tomorrow if you don't beat me to
it.
> > std::array<float, 2> physicalSize{
> > - cellSize.width * pixelArraySize[0] / 1e6f,
> > - cellSize.height * pixelArraySize[1] / 1e6f
> > + cellSize->width * pixelArraySize[0] / 1e6f,
> > + cellSize->height * pixelArraySize[1] / 1e6f
> > };
> > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> > physicalSize);
> > @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >
> > {
> > const Span<const Rectangle> &rects =
> > - properties.get(properties::PixelArrayActiveAreas);
> > + properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
> > std::vector<int32_t> data{
> > static_cast<int32_t>(rects[0].x),
> > static_cast<int32_t>(rects[0].y),
> > @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >
> > /* Report the color filter arrangement if the camera reports it. */
> > if (properties.contains(properties::draft::ColorFilterArrangement)) {
> > - uint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);
> > + uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);
> > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> > filterArr);
> > }
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 8e804d4d..ec117101 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > const ControlList &properties = camera_->properties();
> >
> > if (properties.contains(properties::Location)) {
> > - int32_t location = properties.get(properties::Location);
> > + int32_t location = *properties.get(properties::Location);
> > switch (location) {
> > case properties::CameraLocationFront:
> > facing_ = CAMERA_FACING_FRONT;
> > @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > * metadata.
> > */
> > if (properties.contains(properties::Rotation)) {
> > - int rotation = properties.get(properties::Rotation);
> > + int rotation = *properties.get(properties::Rotation);
> > orientation_ = (360 - rotation) % 360;
> > if (cameraConfigData && cameraConfigData->rotation != -1 &&
> > orientation_ != cameraConfigData->rotation) {
> > @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)
> > * as soon as possible, earlier than request completion time.
> > */
> > uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> > - .get(controls::SensorTimestamp));
> > + .get(controls::SensorTimestamp)
> > + .value_or(0));
> > notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >
> > LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> > @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> > rolling_shutter_skew);
> >
> > /* Add metadata tags reported by libcamera. */
> > - const int64_t timestamp = metadata.get(controls::SensorTimestamp);
> > + const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);
> > resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);
> >
> > if (metadata.contains(controls::draft::PipelineDepth)) {
> > - uint8_t pipeline_depth =
> > - metadata.get<int32_t>(controls::draft::PipelineDepth);
> > + uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth);
> > resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,
> > pipeline_depth);
> > }
> >
> > if (metadata.contains(controls::ExposureTime)) {
> > - int64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;
> > + int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;
> > resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);
> > }
> >
> > if (metadata.contains(controls::FrameDuration)) {
> > - int64_t duration = metadata.get(controls::FrameDuration) * 1000;
> > + int64_t duration = *metadata.get(controls::FrameDuration) * 1000;
> > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,
> > duration);
> > }
> >
> > if (metadata.contains(controls::ScalerCrop)) {
> > - Rectangle crop = metadata.get(controls::ScalerCrop);
> > + Rectangle crop = *metadata.get(controls::ScalerCrop);
> > int32_t cropRect[] = {
> > crop.x, crop.y, static_cast<int32_t>(crop.width),
> > static_cast<int32_t>(crop.height),
> > @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> > }
> >
> > if (metadata.contains(controls::draft::TestPatternMode)) {
> > - const int32_t testPatternMode =
> > - metadata.get(controls::draft::TestPatternMode);
> > + const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);
> > resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,
> > testPatternMode);
> > }
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > index 5f7bfe26..0bffe96f 100644
> > --- a/src/android/camera_hal_manager.cpp
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)
> > if (!properties.contains(properties::Location))
> > return -1;
> >
> > - return properties.get(properties::Location);
> > + return *properties.get(properties::Location);
> > }
> >
> > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 79875ed7..d8115cd8 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)
> > * is only used if the location isn't present or is set to External.
> > */
> > if (props.contains(properties::Location)) {
> > - switch (props.get(properties::Location)) {
> > + switch (*props.get(properties::Location)) {
> > case properties::CameraLocationFront:
> > addModel = false;
> > name = "Internal front camera ";
> > @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)
> > * If the camera location is not availble use the camera model
> > * to build the camera name.
> > */
> > - name = "'" + props.get(properties::Model) + "' ";
> > + name = "'" + *props.get(properties::Model) + "' ";
> > }
> >
> > name += "(" + camera->id() + ")";
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 3b126bb5..f65a0680 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >
> > void IPARPi::prepareISP(const ISPConfig &data)
> > {
> > - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> > + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> > RPiController::Metadata lastMetadata;
> > Span<uint8_t> embeddedBuffer;
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index b7dda282..43db7b68 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()
> > /* Convert the sensor rotation to a transformation */
> > int32_t rotation = 0;
> > if (data->properties_.contains(properties::Rotation))
> > - rotation = data->properties_.get(properties::Rotation);
> > + rotation = *(data->properties_.get(properties::Rotation));
> > else
> > LOG(IPU3, Warning) << "Rotation control not exposed by "
> > << cio2->sensor()->id()
> > @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> > request->metadata().set(controls::draft::PipelineDepth, 3);
> > /* \todo Actually apply the scaler crop region to the ImgU. */
> > if (request->controls().contains(controls::ScalerCrop))
> > - cropRegion_ = request->controls().get(controls::ScalerCrop);
> > + cropRegion_ = *(request->controls().get(controls::ScalerCrop));
> > request->metadata().set(controls::ScalerCrop, cropRegion_);
> >
> > if (frameInfos_.tryComplete(info))
> > @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > return;
> > }
> >
> > - ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),
> > + ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),
> > info->statBuffer->cookie(), info->effectiveSensorControls);
> > }
> >
> > @@ -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));
> >
> > int ret = cio2_.sensor()->setTestPatternMode(
> > static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index adc397e8..a62afdd4 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > * error means the platform can never run. Let's just print a warning
> > * and continue regardless; the rotation is effectively set to zero.
> > */
> > - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);
> > + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);
> > bool success;
> > Transform rotationTransform = transformFromRotation(rotation, &success);
> > if (!success)
> > @@ -1706,7 +1706,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
> > */
> > if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {
> > - libcamera::Span<const float> colourGains = controls.get(libcamera::controls::ColourGains);
> > + libcamera::Span<const float> colourGains =
> > + *controls.get(libcamera::controls::ColourGains);
> > /* The control wants linear gains in the order B, Gb, Gr, R. */
> > ControlList ctrls(sensor_->controls());
> > std::array<int32_t, 4> gains{
> > @@ -2041,7 +2042,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const
> > void RPiCameraData::applyScalerCrop(const ControlList &controls)
> > {
> > if (controls.contains(controls::ScalerCrop)) {
> > - Rectangle nativeCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > + Rectangle nativeCrop = *controls.get<Rectangle>(controls::ScalerCrop);
> >
> > if (!nativeCrop.width || !nativeCrop.height)
> > nativeCrop = { 0, 0, 1, 1 };
> > @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
> > Request *request)
> > {
> > request->metadata().set(controls::SensorTimestamp,
> > - bufferControls.get(controls::SensorTimestamp));
> > + bufferControls.get(controls::SensorTimestamp).value_or(0));
> >
> > request->metadata().set(controls::ScalerCrop, scalerCrop_);
> > }
> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> > index 34c8df5a..4b5d8276 100644
> > --- a/src/qcam/dng_writer.cpp
> > +++ b/src/qcam/dng_writer.cpp
> > @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> > TIFFSetField(tif, TIFFTAG_MAKE, "libcamera");
> >
> > if (cameraProperties.contains(properties::Model)) {
> > - std::string model = cameraProperties.get(properties::Model);
> > + std::string model = *cameraProperties.get(properties::Model);
> > TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());
> > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */
> > }
> > @@ -438,16 +438,15 @@ 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);
> > - if (colourGains[0] > eps && colourGains[1] > eps) {
> > - wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
> > - neutral[0] = 1.0 / colourGains[0]; /* red */
> > - neutral[2] = 1.0 / colourGains[1]; /* blue */
> > + const auto &colourGains = metadata.get(controls::ColourGains);
> > + if ((*colourGains)[0] > eps && (*colourGains)[1] > eps) {
> > + wbGain = Matrix3d::diag((*colourGains)[0], 1, (*colourGains)[1]);
> > + neutral[0] = 1.0 / (*colourGains)[0]; /* red */
> > + neutral[2] = 1.0 / (*colourGains)[1]; /* blue */
> > }
> > }
> > if (metadata.contains(controls::ColourCorrectionMatrix)) {
> > - Span<const float> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
> > - Matrix3d ccmSupplied(coeffs);
> > + Matrix3d ccmSupplied(*metadata.get(controls::ColourCorrectionMatrix));
> > if (ccmSupplied.determinant() > eps)
> > ccm = ccmSupplied;
> > }
> > @@ -515,7 +514,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> > uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;
> >
> > if (metadata.contains(controls::SensorBlackLevels)) {
> > - Span<const int32_t> levels = metadata.get(controls::SensorBlackLevels);
> > + Span<const int32_t> levels =
> > + *metadata.get(controls::SensorBlackLevels);
> >
> > /*
> > * The black levels control is specified in R, Gr, Gb, B order.
> > @@ -593,13 +593,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> > TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);
> >
> > if (metadata.contains(controls::AnalogueGain)) {
> > - float gain = metadata.get(controls::AnalogueGain);
> > + float gain = *metadata.get(controls::AnalogueGain);
> > uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);
> > TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);
> > }
> >
> > if (metadata.contains(controls::ExposureTime)) {
> > - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;
> > + float exposureTime = *metadata.get(controls::ExposureTime) / 1e6;
> > TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list