[{"id":23314,"web_url":"https://patchwork.libcamera.org/comment/23314/","msgid":"<20220604073113.yfpyfsljbagho4eg@uno.localdomain>","date":"2022-06-04T07:31:13","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Christian\n\nOn Fri, Jun 03, 2022 at 11:07:08PM +0100, Christian Rauch via libcamera-devel wrote:\n> Previously, ControlList::get() would use default constructed objects to\n> indicate that a ControlList does not have the requested Control. This has\n> several disadvantages: 1) It requires types to be default constructible,\n> 2) it does not differentiate between a default constructed object and an\n> object that happens to have the same state as a default constructed object.\n>\n> std::optional additionally stores the information if the object is valid\n> or not, and therefore is more expressive than a default constructed object.\n>\n> Signed-off-by: Christian Rauch \n> ---\n> include/libcamera/controls.h | 5 +++--\n> src/android/camera_capabilities.cpp | 8 +++----\n> src/android/camera_device.cpp | 21 +++++++++----------\n> src/android/camera_hal_manager.cpp | 2 +-\n> src/cam/main.cpp | 4 ++--\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++----\n> .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++++----\n> src/qcam/dng_writer.cpp | 15 +++++++------\n> 9 files changed, 39 insertions(+), 36 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 665bcac1..192be784 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -8,6 +8,7 @@\n> #pragma once\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -373,11 +374,11 @@ public:\n> \tbool contains(unsigned int id) const;\n>\n> \ttemplate\n> -\tT get(const Control &ctrl) const\n> +\tstd::optional get(const Control &ctrl) const\n> \t{\n> \t\tconst ControlValue *val = find(ctrl.id());\n> \t\tif (!val)\n> -\t\t\treturn T{};\n> +\t\t\treturn std::nullopt;\n>\n> \t\treturn val->get();\n> \t}\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 6f197eb8..892bbc2b 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t/* Sensor static metadata. */\n> \tstd::array pixelArraySize;\n> \t{\n> -\t\tconst Size &size = properties.get(properties::PixelArraySize);\n> +\t\tconst Size &size = properties.get(properties::PixelArraySize).value_or(Size{});\n\nWe should probably wrap this and a few similar cases above with an\n\n if (properties.contains(...))\n\nNot for this patch though.\n\n> \t\tpixelArraySize[0] = size.width;\n> \t\tpixelArraySize[1] = size.height;\n> \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t}\n>\n> \tif (properties.contains(properties::UnitCellSize)) {\n> -\t\tconst Size &cellSize = properties.get(properties::UnitCellSize);\n> +\t\tconst Size &cellSize = *properties.get(properties::UnitCellSize);\n> \t\tstd::array physicalSize{\n> \t\t\tcellSize.width * pixelArraySize[0] / 1e6f,\n> \t\t\tcellSize.height * pixelArraySize[1] / 1e6f\n> @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()\n>\n> \t{\n> \t\tconst Span &rects =\n> -\t\t\tproperties.get(properties::PixelArrayActiveAreas);\n> +\t\t\tproperties.get(properties::PixelArrayActiveAreas).value_or(Span{});\n> \t\tstd::vector data{\n> \t\t\tstatic_cast(rects[0].x),\n> \t\t\tstatic_cast(rects[0].y),\n> @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()\n>\n> \t/* Report the color filter arrangement if the camera reports it. */\n> \tif (properties.contains(properties::draft::ColorFilterArrangement)) {\n> -\t\tuint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);\n> +\t\tuint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);\n> \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> \t\t\t\t\t filterArr);\n> \t}\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8e804d4d..ec117101 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> \tconst ControlList &properties = camera_->properties();\n>\n> \tif (properties.contains(properties::Location)) {\n> -\t\tint32_t location = properties.get(properties::Location);\n> +\t\tint32_t location = *properties.get(properties::Location);\n> \t\tswitch (location) {\n> \t\tcase properties::CameraLocationFront:\n> \t\t\tfacing_ = CAMERA_FACING_FRONT;\n> @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> \t * metadata.\n> \t */\n> \tif (properties.contains(properties::Rotation)) {\n> -\t\tint rotation = properties.get(properties::Rotation);\n> +\t\tint rotation = *properties.get(properties::Rotation);\n> \t\torientation_ = (360 - rotation) % 360;\n> \t\tif (cameraConfigData && cameraConfigData->rotation != -1 &&\n> \t\t orientation_ != cameraConfigData->rotation) {\n> @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)\n> \t * as soon as possible, earlier than request completion time.\n> \t */\n> \tuint64_t sensorTimestamp = static_cast(request->metadata()\n> -\t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n> +\t\t\t\t\t\t\t\t .get(controls::SensorTimestamp)\n> +\t\t\t\t\t\t\t\t .value_or(0));\n> \tnotifyShutter(descriptor->frameNumber_, sensorTimestamp);\n>\n> \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> \t\t\t\t rolling_shutter_skew);\n>\n> \t/* Add metadata tags reported by libcamera. */\n> -\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp);\n> +\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);\n> \tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);\n>\n> \tif (metadata.contains(controls::draft::PipelineDepth)) {\n> -\t\tuint8_t pipeline_depth =\n> -\t\t\tmetadata.get(controls::draft::PipelineDepth);\n> +\t\tuint8_t pipeline_depth = *metadata.get(controls::draft::PipelineDepth);\n> \t\tresultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,\n> \t\t\t\t\t pipeline_depth);\n> \t}\n>\n> \tif (metadata.contains(controls::ExposureTime)) {\n> -\t\tint64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;\n> +\t\tint64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;\n> \t\tresultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);\n> \t}\n>\n> \tif (metadata.contains(controls::FrameDuration)) {\n> -\t\tint64_t duration = metadata.get(controls::FrameDuration) * 1000;\n> +\t\tint64_t duration = *metadata.get(controls::FrameDuration) * 1000;\n> \t\tresultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,\n> \t\t\t\t\t duration);\n> \t}\n>\n> \tif (metadata.contains(controls::ScalerCrop)) {\n> -\t\tRectangle crop = metadata.get(controls::ScalerCrop);\n> +\t\tRectangle crop = *metadata.get(controls::ScalerCrop);\n> \t\tint32_t cropRect[] = {\n> \t\t\tcrop.x, crop.y, static_cast(crop.width),\n> \t\t\tstatic_cast(crop.height),\n> @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> \t}\n>\n> \tif (metadata.contains(controls::draft::TestPatternMode)) {\n> -\t\tconst int32_t testPatternMode =\n> -\t\t\tmetadata.get(controls::draft::TestPatternMode);\n> +\t\tconst int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);\n> \t\tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> \t\t\t\t\t testPatternMode);\n> \t}\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 5f7bfe26..0b23397a 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> \tif (!properties.contains(properties::Location))\n> \t\treturn -1;\n>\n> -\treturn properties.get(properties::Location);\n> +\treturn properties.get(properties::Location).value_or(0);\n\nNo need to value_or() as we return earlier if Location is not\navailable\n\n> }\n>\n> CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index 79875ed7..d8115cd8 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> \t * is only used if the location isn't present or is set to External.\n> \t */\n> \tif (props.contains(properties::Location)) {\n> -\t\tswitch (props.get(properties::Location)) {\n> +\t\tswitch (*props.get(properties::Location)) {\n> \t\tcase properties::CameraLocationFront:\n> \t\t\taddModel = false;\n> \t\t\tname = \"Internal front camera \";\n> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> \t\t * If the camera location is not availble use the camera model\n> \t\t * to build the camera name.\n> \t\t */\n> -\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n> +\t\tname = \"'\" + *props.get(properties::Model) + \"' \";\n> \t}\n>\n> \tname += \"(\" + camera->id() + \")\";\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 3b126bb5..f65a0680 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>\n> void IPARPi::prepareISP(const ISPConfig &data)\n> {\n> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n\nThis should in theory be guaranteed to be there, as it's the pipeline\nhandler that populates it. However I'm not sure what's best. Either\nASSERT() that the control is there or default it to 0. frameTimestamp\nis not used as a divider anywhere so there's no risk of undefined\nbehaviour.\n\n> \tRPiController::Metadata lastMetadata;\n> \tSpan embeddedBuffer;\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fd989e61..53332826 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Convert the sensor rotation to a transformation */\n> \t\tint32_t rotation = 0;\n> \t\tif (data->properties_.contains(properties::Rotation))\n> -\t\t\trotation = data->properties_.get(properties::Rotation);\n> +\t\t\trotation = *(data->properties_.get(properties::Rotation));\n> \t\telse\n> \t\t\tLOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> \t\t\t\t\t << cio2->sensor()->id()\n> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> \tif (request->controls().contains(controls::ScalerCrop))\n> -\t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n> +\t\tcropRegion_ = *(request->controls().get(controls::ScalerCrop));\n> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n>\n> \tif (frameInfos_.tryComplete(info))\n> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \t\treturn;\n> \t}\n>\n> -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),\n> \t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n\nAlso in this case we should be guaranteed the sensor timestamp is\nthere.\n\nI'm tempted to ASSERT here as well...\n\n> }\n>\n> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> \tif (!request->controls().contains(controls::draft::TestPatternMode))\n> \t\treturn;\n>\n> -\tconst int32_t testPatternMode = request->controls().get(\n> -\t\tcontrols::draft::TestPatternMode);\n> +\tconst int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(0);\n\nNo need as we return earlier if !TestPatternMode\n\n>\n> \tint ret = cio2_.sensor()->setTestPatternMode(\n> \t\tstatic_cast(testPatternMode));\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index adc397e8..a62afdd4 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t * error means the platform can never run. Let's just print a warning\n> \t * and continue regardless; the rotation is effectively set to zero.\n> \t */\n> -\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> +\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);\n> \tbool success;\n> \tTransform rotationTransform = transformFromRotation(rotation, &success);\n> \tif (!success)\n> @@ -1706,7 +1706,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> \t * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> \t */\n> \tif (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n> -\t\tlibcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n> +\t\tlibcamera::Span colourGains =\n> +\t\t\t*controls.get(libcamera::controls::ColourGains);\n> \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n> \t\tControlList ctrls(sensor_->controls());\n> \t\tstd::array gains{\n> @@ -2041,7 +2042,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> {\n> \tif (controls.contains(controls::ScalerCrop)) {\n> -\t\tRectangle nativeCrop = controls.get(controls::ScalerCrop);\n> +\t\tRectangle nativeCrop = *controls.get(controls::ScalerCrop);\n>\n> \t\tif (!nativeCrop.width || !nativeCrop.height)\n> \t\t\tnativeCrop = { 0, 0, 1, 1 };\n> @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> \t\t\t\t\tRequest *request)\n> {\n> \trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\tbufferControls.get(controls::SensorTimestamp));\n> +\t\t\t\tbufferControls.get(controls::SensorTimestamp).value_or(0));\n>\n> \trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> }\n> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> index 34c8df5a..780f58f7 100644\n> --- a/src/qcam/dng_writer.cpp\n> +++ b/src/qcam/dng_writer.cpp\n> @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> \tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n>\n> \tif (cameraProperties.contains(properties::Model)) {\n> -\t\tstd::string model = cameraProperties.get(properties::Model);\n> +\t\tstd::string model = *cameraProperties.get(properties::Model);\n> \t\tTIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n> \t\t/* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> \t}\n> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> \tconst double eps = 1e-2;\n>\n> \tif (metadata.contains(controls::ColourGains)) {\n> -\t\tSpan const &colourGains = metadata.get(controls::ColourGains);\n> +\t\tSpan const &colourGains =\n> +\t\t\t*metadata.get(controls::ColourGains);\n> \t\tif (colourGains[0] > eps && colourGains[1] > eps) {\n> \t\t\twbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n> \t\t\tneutral[0] = 1.0 / colourGains[0]; /* red */\n> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> \t\t}\n> \t}\n> \tif (metadata.contains(controls::ColourCorrectionMatrix)) {\n> -\t\tSpan const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n> +\t\tSpan const &coeffs =\n> +\t\t\t*metadata.get(controls::ColourCorrectionMatrix);\n> \t\tMatrix3d ccmSupplied(coeffs);\n> \t\tif (ccmSupplied.determinant() > eps)\n> \t\t\tccm = ccmSupplied;\n> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> \tuint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n>\n> \tif (metadata.contains(controls::SensorBlackLevels)) {\n> -\t\tSpan levels = metadata.get(controls::SensorBlackLevels);\n> +\t\tSpan levels =\n> +\t\t\t*metadata.get(controls::SensorBlackLevels);\n>\n> \t\t/*\n> \t\t * The black levels control is specified in R, Gr, Gb, B order.\n> @@ -593,13 +596,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> \tTIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n>\n> \tif (metadata.contains(controls::AnalogueGain)) {\n> -\t\tfloat gain = metadata.get(controls::AnalogueGain);\n> +\t\tfloat gain = *metadata.get(controls::AnalogueGain);\n> \t\tuint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n> \t\tTIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n> \t}\n>\n> \tif (metadata.contains(controls::ExposureTime)) {\n> -\t\tfloat exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n> +\t\tfloat exposureTime = *metadata.get(controls::ExposureTime) / 1e6;\n> \t\tTIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> \t}\n\nLet's see what's Laurent comment on the possible assertion, but the\npatch looks good to me and the few other nits can be changed when\napplying if you agree with them\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 58331BD160\n\tfor ;\n\tSat, 4 Jun 2022 07:31:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91F9B65635;\n\tSat, 4 Jun 2022 09:31:17 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C10C96559A\n\tfor ;\n\tSat, 4 Jun 2022 09:31:15 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 26BA6100005;\n\tSat, 4 Jun 2022 07:31:14 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654327877;\n\tbh=kasq9obnx7BQDkzAj33djOdY0KP0JEdLhHI2dxUU/HY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=P8jn5r/WX5C3N/G1lRNf0QQyivKEHOyCMvxSRrvjWIUdOUeTlmdB4J0o3/QSSamiA\n\tjK2ZDtJr0x6ev7aL9/V813+ykUydH2HWaME/HQ0JAraA6OgAWqp0LxJJzEmcO7rWe9\n\tieqLOvjveS3UsBOKckch9hM3lOwaWMG8qgYR3NBUx3JQ0xUv20o9DftYN5tBJqCvm1\n\tud8JzFwPwiOc/RrButOhXy4gxppxO7F+ERK4xaN05ssLebpme5YBz4XqlvuCGdjwm0\n\t+SH5WlU8p0bF52bFSPDUXQlCZY/yoWAxRGJzEpAq0z77IyFtYY2XiZlOvs0GBiPasL\n\to1scLbwzvkS3A==","Date":"Sat, 4 Jun 2022 09:31:13 +0200","To":"Christian Rauch ","Message-ID":"<20220604073113.yfpyfsljbagho4eg@uno.localdomain>","References":"<20220603220712.73673-1-Rauch.Christian@gmx.de>\n\t<20220603220712.73673-2-Rauch.Christian@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220603220712.73673-2-Rauch.Christian@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23316,"web_url":"https://patchwork.libcamera.org/comment/23316/","msgid":"<20220604082957.ft4nlsugu4zpn36o@uno.localdomain>","date":"2022-06-04T08:29:57","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Christian,\n spoke too soon\n\nOn Sat, Jun 04, 2022 at 09:31:13AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Hi Christian\n>\n> On Fri, Jun 03, 2022 at 11:07:08PM +0100, Christian Rauch via libcamera-devel wrote:\n> > Previously, ControlList::get() would use default constructed objects to\n> > indicate that a ControlList does not have the requested Control. This has\n> > several disadvantages: 1) It requires types to be default constructible,\n> > 2) it does not differentiate between a default constructed object and an\n> > object that happens to have the same state as a default constructed object.\n> >\n> > std::optional additionally stores the information if the object is valid\n> > or not, and therefore is more expressive than a default constructed object.\n> >\n> > Signed-off-by: Christian Rauch \n> > ---\n> > include/libcamera/controls.h | 5 +++--\n> > src/android/camera_capabilities.cpp | 8 +++----\n> > src/android/camera_device.cpp | 21 +++++++++----------\n> > src/android/camera_hal_manager.cpp | 2 +-\n> > src/cam/main.cpp | 4 ++--\n> > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++----\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++++----\n> > src/qcam/dng_writer.cpp | 15 +++++++------\n> > 9 files changed, 39 insertions(+), 36 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 665bcac1..192be784 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -8,6 +8,7 @@\n> > #pragma once\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -373,11 +374,11 @@ public:\n> > \tbool contains(unsigned int id) const;\n> >\n> > \ttemplate\n> > -\tT get(const Control &ctrl) const\n> > +\tstd::optional get(const Control &ctrl) const\n> > \t{\n> > \t\tconst ControlValue *val = find(ctrl.id());\n> > \t\tif (!val)\n> > -\t\t\treturn T{};\n> > +\t\t\treturn std::nullopt;\n> >\n> > \t\treturn val->get();\n> > \t}\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 6f197eb8..892bbc2b 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t/* Sensor static metadata. */\n> > \tstd::array pixelArraySize;\n> > \t{\n> > -\t\tconst Size &size = properties.get(properties::PixelArraySize);\n> > +\t\tconst Size &size = properties.get(properties::PixelArraySize).value_or(Size{});\n>\n> We should probably wrap this and a few similar cases above with an\n>\n> if (properties.contains(...))\n>\n> Not for this patch though.\n>\n> > \t\tpixelArraySize[0] = size.width;\n> > \t\tpixelArraySize[1] = size.height;\n> > \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t}\n> >\n> > \tif (properties.contains(properties::UnitCellSize)) {\n> > -\t\tconst Size &cellSize = properties.get(properties::UnitCellSize);\n> > +\t\tconst Size &cellSize = *properties.get(properties::UnitCellSize);\n\nHere you could remove the template argument if I'm not\nmistaken. I know it was already there...\n\nBut, most important, I get the following with clang 14.0.0\n\n(Showing with both and without)\n\nsrc/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]\n const Size &cellSize = *properties.get(properties::UnitCellSize);\n\nsrc/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]\n const Size &cellSize = *properties.get(properties::UnitCellSize);\n\nI guess the issue might be due to taking a reference to a temporary\nvalue as:\nconst Size cellSize = *properties.get(properties::UnitCellSize);\n\nas well as:\nconst Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;\n\ncompile fine.\n\nWhen it comes to the usage of value_or(), its documentation says:\n[1] https://en.cppreference.com/w/cpp/utility/optional/value_or\n-T must meet the requirements of CopyConstructible in order to use overload (1).\n-T must meet the requirements of MoveConstructible in order to use overload (2).\n\nwhich means the value is copied or moved back while operator*\n[2] https://en.cppreference.com/w/cpp/utility/optional/operator*\n1) Returns a pointer to the contained value.\n2) Returns a reference to the contained value.\n\nI guess the issue is due to trying to reference the content of a\ntemporary std::optional instance, created to wrap the return value\nof ControlValue::get\n\n ControlList:\n\ttemplate\n\tstd::optional get(const Control &ctrl) const\n\t{\n\t\tconst ControlValue *val = find(ctrl.id());\n\t\tif (!val)\n\t\t\treturn std::nullopt;\n\n\t\treturn val->get();\n\t}\n\nThis should be fine, as the object returned by ControlValue::get<>()\nrefers to valid memory, and as we could take references to it before\nthis series, we should be able to do so now, but the compiler worries\nthat dereferencing the temporary std::optional<> would leave us with a\npointer to invalid memory. Is my interpretation of the issue correct ?\n\n> > \t\t\tcellSize.width * pixelArraySize[0] / 1e6f,\n> > \t\t\tcellSize.height * pixelArraySize[1] / 1e6f\n> > @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> >\n> > \t{\n> > \t\tconst Span &rects =\n> > -\t\t\tproperties.get(properties::PixelArrayActiveAreas);\n> > +\t\t\tproperties.get(properties::PixelArrayActiveAreas).value_or(Span{});\n> > \t\tstd::vector data{\n> > \t\t\tstatic_cast(rects[0].x),\n> > \t\t\tstatic_cast(rects[0].y),\n> > @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> >\n> > \t/* Report the color filter arrangement if the camera reports it. */\n> > \tif (properties.contains(properties::draft::ColorFilterArrangement)) {\n> > -\t\tuint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);\n> > +\t\tuint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);\n> > \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> > \t\t\t\t\t filterArr);\n> > \t}\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 8e804d4d..ec117101 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> > \tconst ControlList &properties = camera_->properties();\n> >\n> > \tif (properties.contains(properties::Location)) {\n> > -\t\tint32_t location = properties.get(properties::Location);\n> > +\t\tint32_t location = *properties.get(properties::Location);\n> > \t\tswitch (location) {\n> > \t\tcase properties::CameraLocationFront:\n> > \t\t\tfacing_ = CAMERA_FACING_FRONT;\n> > @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> > \t * metadata.\n> > \t */\n> > \tif (properties.contains(properties::Rotation)) {\n> > -\t\tint rotation = properties.get(properties::Rotation);\n> > +\t\tint rotation = *properties.get(properties::Rotation);\n> > \t\torientation_ = (360 - rotation) % 360;\n> > \t\tif (cameraConfigData && cameraConfigData->rotation != -1 &&\n> > \t\t orientation_ != cameraConfigData->rotation) {\n> > @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)\n> > \t * as soon as possible, earlier than request completion time.\n> > \t */\n> > \tuint64_t sensorTimestamp = static_cast(request->metadata()\n> > -\t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n> > +\t\t\t\t\t\t\t\t .get(controls::SensorTimestamp)\n> > +\t\t\t\t\t\t\t\t .value_or(0));\n> > \tnotifyShutter(descriptor->frameNumber_, sensorTimestamp);\n> >\n> > \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> > @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> > \t\t\t\t rolling_shutter_skew);\n> >\n> > \t/* Add metadata tags reported by libcamera. */\n> > -\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp);\n> > +\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);\n> > \tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);\n> >\n> > \tif (metadata.contains(controls::draft::PipelineDepth)) {\n> > -\t\tuint8_t pipeline_depth =\n> > -\t\t\tmetadata.get(controls::draft::PipelineDepth);\n> > +\t\tuint8_t pipeline_depth = *metadata.get(controls::draft::PipelineDepth);\n> > \t\tresultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,\n> > \t\t\t\t\t pipeline_depth);\n> > \t}\n> >\n> > \tif (metadata.contains(controls::ExposureTime)) {\n> > -\t\tint64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;\n> > +\t\tint64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;\n> > \t\tresultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);\n> > \t}\n> >\n> > \tif (metadata.contains(controls::FrameDuration)) {\n> > -\t\tint64_t duration = metadata.get(controls::FrameDuration) * 1000;\n> > +\t\tint64_t duration = *metadata.get(controls::FrameDuration) * 1000;\n> > \t\tresultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,\n> > \t\t\t\t\t duration);\n> > \t}\n> >\n> > \tif (metadata.contains(controls::ScalerCrop)) {\n> > -\t\tRectangle crop = metadata.get(controls::ScalerCrop);\n> > +\t\tRectangle crop = *metadata.get(controls::ScalerCrop);\n> > \t\tint32_t cropRect[] = {\n> > \t\t\tcrop.x, crop.y, static_cast(crop.width),\n> > \t\t\tstatic_cast(crop.height),\n> > @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> > \t}\n> >\n> > \tif (metadata.contains(controls::draft::TestPatternMode)) {\n> > -\t\tconst int32_t testPatternMode =\n> > -\t\t\tmetadata.get(controls::draft::TestPatternMode);\n> > +\t\tconst int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);\n> > \t\tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> > \t\t\t\t\t testPatternMode);\n> > \t}\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > index 5f7bfe26..0b23397a 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> > \tif (!properties.contains(properties::Location))\n> > \t\treturn -1;\n> >\n> > -\treturn properties.get(properties::Location);\n> > +\treturn properties.get(properties::Location).value_or(0);\n>\n> No need to value_or() as we return earlier if Location is not\n> available\n>\n> > }\n> >\n> > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index 79875ed7..d8115cd8 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> > \t * is only used if the location isn't present or is set to External.\n> > \t */\n> > \tif (props.contains(properties::Location)) {\n> > -\t\tswitch (props.get(properties::Location)) {\n> > +\t\tswitch (*props.get(properties::Location)) {\n> > \t\tcase properties::CameraLocationFront:\n> > \t\t\taddModel = false;\n> > \t\t\tname = \"Internal front camera \";\n> > @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> > \t\t * If the camera location is not availble use the camera model\n> > \t\t * to build the camera name.\n> > \t\t */\n> > -\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n> > +\t\tname = \"'\" + *props.get(properties::Model) + \"' \";\n> > \t}\n> >\n> > \tname += \"(\" + camera->id() + \")\";\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 3b126bb5..f65a0680 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> >\n> > void IPARPi::prepareISP(const ISPConfig &data)\n> > {\n> > -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> > +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n>\n> This should in theory be guaranteed to be there, as it's the pipeline\n> handler that populates it. However I'm not sure what's best. Either\n> ASSERT() that the control is there or default it to 0. frameTimestamp\n> is not used as a divider anywhere so there's no risk of undefined\n> behaviour.\n>\n> > \tRPiController::Metadata lastMetadata;\n> > \tSpan embeddedBuffer;\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index fd989e61..53332826 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t/* Convert the sensor rotation to a transformation */\n> > \t\tint32_t rotation = 0;\n> > \t\tif (data->properties_.contains(properties::Rotation))\n> > -\t\t\trotation = data->properties_.get(properties::Rotation);\n> > +\t\t\trotation = *(data->properties_.get(properties::Rotation));\n> > \t\telse\n> > \t\t\tLOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> > \t\t\t\t\t << cio2->sensor()->id()\n> > @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> > \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> > \tif (request->controls().contains(controls::ScalerCrop))\n> > -\t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n> > +\t\tcropRegion_ = *(request->controls().get(controls::ScalerCrop));\n> > \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n> >\n> > \tif (frameInfos_.tryComplete(info))\n> > @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > \t\treturn;\n> > \t}\n> >\n> > -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> > +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),\n> > \t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n>\n> Also in this case we should be guaranteed the sensor timestamp is\n> there.\n>\n> I'm tempted to ASSERT here as well...\n>\n> > }\n> >\n> > @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> > \tif (!request->controls().contains(controls::draft::TestPatternMode))\n> > \t\treturn;\n> >\n> > -\tconst int32_t testPatternMode = request->controls().get(\n> > -\t\tcontrols::draft::TestPatternMode);\n> > +\tconst int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(0);\n>\n> No need as we return earlier if !TestPatternMode\n>\n> >\n> > \tint ret = cio2_.sensor()->setTestPatternMode(\n> > \t\tstatic_cast(testPatternMode));\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index adc397e8..a62afdd4 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > \t * error means the platform can never run. Let's just print a warning\n> > \t * and continue regardless; the rotation is effectively set to zero.\n> > \t */\n> > -\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> > +\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);\n> > \tbool success;\n> > \tTransform rotationTransform = transformFromRotation(rotation, &success);\n> > \tif (!success)\n> > @@ -1706,7 +1706,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> > \t * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> > \t */\n> > \tif (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n> > -\t\tlibcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n> > +\t\tlibcamera::Span colourGains =\n> > +\t\t\t*controls.get(libcamera::controls::ColourGains);\n> > \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n> > \t\tControlList ctrls(sensor_->controls());\n> > \t\tstd::array gains{\n> > @@ -2041,7 +2042,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> > void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > {\n> > \tif (controls.contains(controls::ScalerCrop)) {\n> > -\t\tRectangle nativeCrop = controls.get(controls::ScalerCrop);\n> > +\t\tRectangle nativeCrop = *controls.get(controls::ScalerCrop);\n> >\n> > \t\tif (!nativeCrop.width || !nativeCrop.height)\n> > \t\t\tnativeCrop = { 0, 0, 1, 1 };\n> > @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> > \t\t\t\t\tRequest *request)\n> > {\n> > \trequest->metadata().set(controls::SensorTimestamp,\n> > -\t\t\t\tbufferControls.get(controls::SensorTimestamp));\n> > +\t\t\t\tbufferControls.get(controls::SensorTimestamp).value_or(0));\n> >\n> > \trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > }\n> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> > index 34c8df5a..780f58f7 100644\n> > --- a/src/qcam/dng_writer.cpp\n> > +++ b/src/qcam/dng_writer.cpp\n> > @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > \tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> >\n> > \tif (cameraProperties.contains(properties::Model)) {\n> > -\t\tstd::string model = cameraProperties.get(properties::Model);\n> > +\t\tstd::string model = *cameraProperties.get(properties::Model);\n> > \t\tTIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n> > \t\t/* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> > \t}\n> > @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > \tconst double eps = 1e-2;\n> >\n> > \tif (metadata.contains(controls::ColourGains)) {\n> > -\t\tSpan const &colourGains = metadata.get(controls::ColourGains);\n> > +\t\tSpan const &colourGains =\n> > +\t\t\t*metadata.get(controls::ColourGains);\n> > \t\tif (colourGains[0] > eps && colourGains[1] > eps) {\n> > \t\t\twbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n> > \t\t\tneutral[0] = 1.0 / colourGains[0]; /* red */\n> > @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > \t\t}\n> > \t}\n> > \tif (metadata.contains(controls::ColourCorrectionMatrix)) {\n> > -\t\tSpan const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n> > +\t\tSpan const &coeffs =\n> > +\t\t\t*metadata.get(controls::ColourCorrectionMatrix);\n> > \t\tMatrix3d ccmSupplied(coeffs);\n> > \t\tif (ccmSupplied.determinant() > eps)\n> > \t\t\tccm = ccmSupplied;\n> > @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > \tuint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n> >\n> > \tif (metadata.contains(controls::SensorBlackLevels)) {\n> > -\t\tSpan levels = metadata.get(controls::SensorBlackLevels);\n> > +\t\tSpan levels =\n> > +\t\t\t*metadata.get(controls::SensorBlackLevels);\n> >\n> > \t\t/*\n> > \t\t * The black levels control is specified in R, Gr, Gb, B order.\n> > @@ -593,13 +596,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > \tTIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n> >\n> > \tif (metadata.contains(controls::AnalogueGain)) {\n> > -\t\tfloat gain = metadata.get(controls::AnalogueGain);\n> > +\t\tfloat gain = *metadata.get(controls::AnalogueGain);\n> > \t\tuint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n> > \t\tTIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n> > \t}\n> >\n> > \tif (metadata.contains(controls::ExposureTime)) {\n> > -\t\tfloat exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n> > +\t\tfloat exposureTime = *metadata.get(controls::ExposureTime) / 1e6;\n> > \t\tTIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> > \t}\n>\n> Let's see what's Laurent comment on the possible assertion, but the\n> patch looks good to me and the few other nits can be changed when\n> applying if you agree with them\n>\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> >\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6771BBD161\n\tfor ;\n\tSat, 4 Jun 2022 08:30:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C558A65634;\n\tSat, 4 Jun 2022 10:30:01 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 863876559A\n\tfor ;\n\tSat, 4 Jun 2022 10:29:59 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id EE12A20003;\n\tSat, 4 Jun 2022 08:29:58 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654331401;\n\tbh=1Ugw44d42IUN8iPDJt9YnWylgGsgGoHdRh7ifuwhMJM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=c4XzyQhpnsV6TJ4dsmnEBRVuupAa35F3EK2P/1PBASuchAWwkbvGzdDtMQjtmKDQ3\n\tztweyd1eKSh+MgEZcS0F3igeMqhcvQUWs6PTHZvrm35c82NNBaZ623xuwC9KnW8ClJ\n\tzlK4O6bOsnTD1zqwMYfb6aIhsqIV6QzdDD/J99ZH+VudnezRBShm+LPSbvT2CRDsSh\n\tXYpsxiVKYROlqYPai9M25O5ZHrkN4ha9RfoqDYGpAr2F+bPhXDJQs3cvp5rlOAIPLv\n\tLPauBhkHjdOP1ne+T7av3M+2/burAVl2oxUoYXOVTlg7NOF6+4bU8UO+e7ocW0qpJv\n\t2i9RNJDyUG4LQ==","Date":"Sat, 4 Jun 2022 10:29:57 +0200","To":"Christian Rauch ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20220604082957.ft4nlsugu4zpn36o@uno.localdomain>","References":"<20220603220712.73673-1-Rauch.Christian@gmx.de>\n\t<20220603220712.73673-2-Rauch.Christian@gmx.de>\n\t<20220604073113.yfpyfsljbagho4eg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220604073113.yfpyfsljbagho4eg@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23318,"web_url":"https://patchwork.libcamera.org/comment/23318/","msgid":"<72d66ed0-1e73-5539-b661-96c63702482b@gmx.de>","date":"2022-06-04T20:50:27","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Jacopo,\n\nAm 04.06.22 um 09:29 schrieb Jacopo Mondi:\n> Hi Christian,\n> spoke too soon\n>\n> On Sat, Jun 04, 2022 at 09:31:13AM +0200, Jacopo Mondi via libcamera-devel wrote:\n>> Hi Christian\n>>\n>> On Fri, Jun 03, 2022 at 11:07:08PM +0100, Christian Rauch via libcamera-devel wrote:\n>>> Previously, ControlList::get() would use default constructed objects to\n>>> indicate that a ControlList does not have the requested Control. This has\n>>> several disadvantages: 1) It requires types to be default constructible,\n>>> 2) it does not differentiate between a default constructed object and an\n>>> object that happens to have the same state as a default constructed object.\n>>>\n>>> std::optional additionally stores the information if the object is valid\n>>> or not, and therefore is more expressive than a default constructed object.\n>>>\n>>> Signed-off-by: Christian Rauch \n>>> ---\n>>> include/libcamera/controls.h | 5 +++--\n>>> src/android/camera_capabilities.cpp | 8 +++----\n>>> src/android/camera_device.cpp | 21 +++++++++----------\n>>> src/android/camera_hal_manager.cpp | 2 +-\n>>> src/cam/main.cpp | 4 ++--\n>>> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++----\n>>> .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++++----\n>>> src/qcam/dng_writer.cpp | 15 +++++++------\n>>> 9 files changed, 39 insertions(+), 36 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>> index 665bcac1..192be784 100644\n>>> --- a/include/libcamera/controls.h\n>>> +++ b/include/libcamera/controls.h\n>>> @@ -8,6 +8,7 @@\n>>> #pragma once\n>>>\n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -373,11 +374,11 @@ public:\n>>> \tbool contains(unsigned int id) const;\n>>>\n>>> \ttemplate\n>>> -\tT get(const Control &ctrl) const\n>>> +\tstd::optional get(const Control &ctrl) const\n>>> \t{\n>>> \t\tconst ControlValue *val = find(ctrl.id());\n>>> \t\tif (!val)\n>>> -\t\t\treturn T{};\n>>> +\t\t\treturn std::nullopt;\n>>>\n>>> \t\treturn val->get();\n>>> \t}\n>>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n>>> index 6f197eb8..892bbc2b 100644\n>>> --- a/src/android/camera_capabilities.cpp\n>>> +++ b/src/android/camera_capabilities.cpp\n>>> @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()\n>>> \t/* Sensor static metadata. */\n>>> \tstd::array pixelArraySize;\n>>> \t{\n>>> -\t\tconst Size &size = properties.get(properties::PixelArraySize);\n>>> +\t\tconst Size &size = properties.get(properties::PixelArraySize).value_or(Size{});\n>>\n>> We should probably wrap this and a few similar cases above with an\n>>\n>> if (properties.contains(...))\n>>\n>> Not for this patch though.\n>>\n>>> \t\tpixelArraySize[0] = size.width;\n>>> \t\tpixelArraySize[1] = size.height;\n>>> \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n>>> @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()\n>>> \t}\n>>>\n>>> \tif (properties.contains(properties::UnitCellSize)) {\n>>> -\t\tconst Size &cellSize = properties.get(properties::UnitCellSize);\n>>> +\t\tconst Size &cellSize = *properties.get(properties::UnitCellSize);\n>\n> Here you could remove the template argument if I'm not\n> mistaken. I know it was already there...\n>\n> But, most important, I get the following with clang 14.0.0\n>\n> (Showing with both and without)\n>\n> src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]\n> const Size &cellSize = *properties.get(properties::UnitCellSize);\n>\n> src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]\n> const Size &cellSize = *properties.get(properties::UnitCellSize);\n>\n> I guess the issue might be due to taking a reference to a temporary\n> value as:\n> const Size cellSize = *properties.get(properties::UnitCellSize);\n>\n> as well as:\n> const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;\n>\n> compile fine.\n\nThanks for pointing this out. I could reproduce this with clang\n(\"CC=clang CXX=clang++ meson build -Dandroid=enabled\") but not with gcc.\nEven \"-Dwarning_level=3\" did not show these errors.\n\nDoes libcamera run a build server somewhere? This would be helpful to\ndiscover errors in non-standard configurations (e.g. with clang in this\ncase or with \"android\" enabled).\n\n>\n> When it comes to the usage of value_or(), its documentation says:\n> [1] https://en.cppreference.com/w/cpp/utility/optional/value_or\n> -T must meet the requirements of CopyConstructible in order to use overload (1).\n> -T must meet the requirements of MoveConstructible in order to use overload (2).\n>\n> which means the value is copied or moved back while operator*\n> [2] https://en.cppreference.com/w/cpp/utility/optional/operator*\n> 1) Returns a pointer to the contained value.\n> 2) Returns a reference to the contained value.\n>\n> I guess the issue is due to trying to reference the content of a\n> temporary std::optional instance, created to wrap the return value\n> of ControlValue::get\n>\n> ControlList:\n> \ttemplate\n> \tstd::optional get(const Control &ctrl) const\n> \t{\n> \t\tconst ControlValue *val = find(ctrl.id());\n> \t\tif (!val)\n> \t\t\treturn std::nullopt;\n>\n> \t\treturn val->get();\n> \t}\n>\n> This should be fine, as the object returned by ControlValue::get<>()\n> refers to valid memory, and as we could take references to it before\n> this series, we should be able to do so now, but the compiler worries\n> that dereferencing the temporary std::optional<> would leave us with a\n> pointer to invalid memory. Is my interpretation of the issue correct ?\n>\n\nYes, I think what is happening here is that the optional returns the\npointer to a value that can become \"invalid\" later. This could happen\nwhen the \"std::optional::reset()\" is called or when \"std::nullopt\" is\nassigned.\n\nTaking the reference of this would be dangerous. We know that the\nreference will \"cellSize\" be destroyed shortly after, but generally,\nthere is no guarantee. For the compiler, it makes sense to complain\nabout this.\n\nI only see two ways to solve this:\n\n1. copy the content:\n\n const Size cellSize = *properties.get(properties::UnitCellSize);\n\n2. use the \"std::optional\" directly:\n\n const auto cellSize = properties.get(properties::UnitCellSize);\n\n and then access it via the \"->\" operator:\n\n cellSize->width\n\nI personally prefer the second option since it avoids copying the entire\ncontent, which could be expensive for other control types.\n\n\n>>> \t\t\tcellSize.width * pixelArraySize[0] / 1e6f,\n>>> \t\t\tcellSize.height * pixelArraySize[1] / 1e6f\n>>> @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()\n>>>\n>>> \t{\n>>> \t\tconst Span &rects =\n>>> -\t\t\tproperties.get(properties::PixelArrayActiveAreas);\n>>> +\t\t\tproperties.get(properties::PixelArrayActiveAreas).value_or(Span{});\n>>> \t\tstd::vector data{\n>>> \t\t\tstatic_cast(rects[0].x),\n>>> \t\t\tstatic_cast(rects[0].y),\n>>> @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()\n>>>\n>>> \t/* Report the color filter arrangement if the camera reports it. */\n>>> \tif (properties.contains(properties::draft::ColorFilterArrangement)) {\n>>> -\t\tuint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);\n>>> +\t\tuint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);\n>>> \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n>>> \t\t\t\t\t filterArr);\n>>> \t}\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index 8e804d4d..ec117101 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n>>> \tconst ControlList &properties = camera_->properties();\n>>>\n>>> \tif (properties.contains(properties::Location)) {\n>>> -\t\tint32_t location = properties.get(properties::Location);\n>>> +\t\tint32_t location = *properties.get(properties::Location);\n>>> \t\tswitch (location) {\n>>> \t\tcase properties::CameraLocationFront:\n>>> \t\t\tfacing_ = CAMERA_FACING_FRONT;\n>>> @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n>>> \t * metadata.\n>>> \t */\n>>> \tif (properties.contains(properties::Rotation)) {\n>>> -\t\tint rotation = properties.get(properties::Rotation);\n>>> +\t\tint rotation = *properties.get(properties::Rotation);\n>>> \t\torientation_ = (360 - rotation) % 360;\n>>> \t\tif (cameraConfigData && cameraConfigData->rotation != -1 &&\n>>> \t\t orientation_ != cameraConfigData->rotation) {\n>>> @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)\n>>> \t * as soon as possible, earlier than request completion time.\n>>> \t */\n>>> \tuint64_t sensorTimestamp = static_cast(request->metadata()\n>>> -\t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n>>> +\t\t\t\t\t\t\t\t .get(controls::SensorTimestamp)\n>>> +\t\t\t\t\t\t\t\t .value_or(0));\n>>> \tnotifyShutter(descriptor->frameNumber_, sensorTimestamp);\n>>>\n>>> \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n>>> @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n>>> \t\t\t\t rolling_shutter_skew);\n>>>\n>>> \t/* Add metadata tags reported by libcamera. */\n>>> -\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp);\n>>> +\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);\n>>> \tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);\n>>>\n>>> \tif (metadata.contains(controls::draft::PipelineDepth)) {\n>>> -\t\tuint8_t pipeline_depth =\n>>> -\t\t\tmetadata.get(controls::draft::PipelineDepth);\n>>> +\t\tuint8_t pipeline_depth = *metadata.get(controls::draft::PipelineDepth);\n>>> \t\tresultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,\n>>> \t\t\t\t\t pipeline_depth);\n>>> \t}\n>>>\n>>> \tif (metadata.contains(controls::ExposureTime)) {\n>>> -\t\tint64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;\n>>> +\t\tint64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;\n>>> \t\tresultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);\n>>> \t}\n>>>\n>>> \tif (metadata.contains(controls::FrameDuration)) {\n>>> -\t\tint64_t duration = metadata.get(controls::FrameDuration) * 1000;\n>>> +\t\tint64_t duration = *metadata.get(controls::FrameDuration) * 1000;\n>>> \t\tresultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,\n>>> \t\t\t\t\t duration);\n>>> \t}\n>>>\n>>> \tif (metadata.contains(controls::ScalerCrop)) {\n>>> -\t\tRectangle crop = metadata.get(controls::ScalerCrop);\n>>> +\t\tRectangle crop = *metadata.get(controls::ScalerCrop);\n>>> \t\tint32_t cropRect[] = {\n>>> \t\t\tcrop.x, crop.y, static_cast(crop.width),\n>>> \t\t\tstatic_cast(crop.height),\n>>> @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n>>> \t}\n>>>\n>>> \tif (metadata.contains(controls::draft::TestPatternMode)) {\n>>> -\t\tconst int32_t testPatternMode =\n>>> -\t\t\tmetadata.get(controls::draft::TestPatternMode);\n>>> +\t\tconst int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);\n>>> \t\tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n>>> \t\t\t\t\t testPatternMode);\n>>> \t}\n>>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n>>> index 5f7bfe26..0b23397a 100644\n>>> --- a/src/android/camera_hal_manager.cpp\n>>> +++ b/src/android/camera_hal_manager.cpp\n>>> @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n>>> \tif (!properties.contains(properties::Location))\n>>> \t\treturn -1;\n>>>\n>>> -\treturn properties.get(properties::Location);\n>>> +\treturn properties.get(properties::Location).value_or(0);\n>>\n>> No need to value_or() as we return earlier if Location is not\n>> available\n>>\n>>> }\n>>>\n>>> CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n>>> index 79875ed7..d8115cd8 100644\n>>> --- a/src/cam/main.cpp\n>>> +++ b/src/cam/main.cpp\n>>> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>> \t * is only used if the location isn't present or is set to External.\n>>> \t */\n>>> \tif (props.contains(properties::Location)) {\n>>> -\t\tswitch (props.get(properties::Location)) {\n>>> +\t\tswitch (*props.get(properties::Location)) {\n>>> \t\tcase properties::CameraLocationFront:\n>>> \t\t\taddModel = false;\n>>> \t\t\tname = \"Internal front camera \";\n>>> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>> \t\t * If the camera location is not availble use the camera model\n>>> \t\t * to build the camera name.\n>>> \t\t */\n>>> -\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n>>> +\t\tname = \"'\" + *props.get(properties::Model) + \"' \";\n>>> \t}\n>>>\n>>> \tname += \"(\" + camera->id() + \")\";\n>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> index 3b126bb5..f65a0680 100644\n>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>>>\n>>> void IPARPi::prepareISP(const ISPConfig &data)\n>>> {\n>>> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n>>> +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n>>\n>> This should in theory be guaranteed to be there, as it's the pipeline\n>> handler that populates it. However I'm not sure what's best. Either\n>> ASSERT() that the control is there or default it to 0. frameTimestamp\n>> is not used as a divider anywhere so there's no risk of undefined\n>> behaviour.\n>>\n>>> \tRPiController::Metadata lastMetadata;\n>>> \tSpan embeddedBuffer;\n>>>\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index fd989e61..53332826 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()\n>>> \t\t/* Convert the sensor rotation to a transformation */\n>>> \t\tint32_t rotation = 0;\n>>> \t\tif (data->properties_.contains(properties::Rotation))\n>>> -\t\t\trotation = data->properties_.get(properties::Rotation);\n>>> +\t\t\trotation = *(data->properties_.get(properties::Rotation));\n>>> \t\telse\n>>> \t\t\tLOG(IPU3, Warning) << \"Rotation control not exposed by \"\n>>> \t\t\t\t\t << cio2->sensor()->id()\n>>> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n>>> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n>>> \tif (request->controls().contains(controls::ScalerCrop))\n>>> -\t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n>>> +\t\tcropRegion_ = *(request->controls().get(controls::ScalerCrop));\n>>> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n>>>\n>>> \tif (frameInfos_.tryComplete(info))\n>>> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>> \t\treturn;\n>>> \t}\n>>>\n>>> -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n>>> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),\n>>> \t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n>>\n>> Also in this case we should be guaranteed the sensor timestamp is\n>> there.\n>>\n>> I'm tempted to ASSERT here as well...\n>>\n>>> }\n>>>\n>>> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n>>> \tif (!request->controls().contains(controls::draft::TestPatternMode))\n>>> \t\treturn;\n>>>\n>>> -\tconst int32_t testPatternMode = request->controls().get(\n>>> -\t\tcontrols::draft::TestPatternMode);\n>>> +\tconst int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(0);\n>>\n>> No need as we return earlier if !TestPatternMode\n>>\n>>>\n>>> \tint ret = cio2_.sensor()->setTestPatternMode(\n>>> \t\tstatic_cast(testPatternMode));\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> index adc397e8..a62afdd4 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n>>> \t * error means the platform can never run. Let's just print a warning\n>>> \t * and continue regardless; the rotation is effectively set to zero.\n>>> \t */\n>>> -\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n>>> +\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);\n>>> \tbool success;\n>>> \tTransform rotationTransform = transformFromRotation(rotation, &success);\n>>> \tif (!success)\n>>> @@ -1706,7 +1706,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n>>> \t * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n>>> \t */\n>>> \tif (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n>>> -\t\tlibcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n>>> +\t\tlibcamera::Span colourGains =\n>>> +\t\t\t*controls.get(libcamera::controls::ColourGains);\n>>> \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n>>> \t\tControlList ctrls(sensor_->controls());\n>>> \t\tstd::array gains{\n>>> @@ -2041,7 +2042,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n>>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n>>> {\n>>> \tif (controls.contains(controls::ScalerCrop)) {\n>>> -\t\tRectangle nativeCrop = controls.get(controls::ScalerCrop);\n>>> +\t\tRectangle nativeCrop = *controls.get(controls::ScalerCrop);\n>>>\n>>> \t\tif (!nativeCrop.width || !nativeCrop.height)\n>>> \t\t\tnativeCrop = { 0, 0, 1, 1 };\n>>> @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n>>> \t\t\t\t\tRequest *request)\n>>> {\n>>> \trequest->metadata().set(controls::SensorTimestamp,\n>>> -\t\t\t\tbufferControls.get(controls::SensorTimestamp));\n>>> +\t\t\t\tbufferControls.get(controls::SensorTimestamp).value_or(0));\n>>>\n>>> \trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n>>> }\n>>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n>>> index 34c8df5a..780f58f7 100644\n>>> --- a/src/qcam/dng_writer.cpp\n>>> +++ b/src/qcam/dng_writer.cpp\n>>> @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> \tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n>>>\n>>> \tif (cameraProperties.contains(properties::Model)) {\n>>> -\t\tstd::string model = cameraProperties.get(properties::Model);\n>>> +\t\tstd::string model = *cameraProperties.get(properties::Model);\n>>> \t\tTIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n>>> \t\t/* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n>>> \t}\n>>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> \tconst double eps = 1e-2;\n>>>\n>>> \tif (metadata.contains(controls::ColourGains)) {\n>>> -\t\tSpan const &colourGains = metadata.get(controls::ColourGains);\n>>> +\t\tSpan const &colourGains =\n>>> +\t\t\t*metadata.get(controls::ColourGains);\n>>> \t\tif (colourGains[0] > eps && colourGains[1] > eps) {\n>>> \t\t\twbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n>>> \t\t\tneutral[0] = 1.0 / colourGains[0]; /* red */\n>>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> \t\t}\n>>> \t}\n>>> \tif (metadata.contains(controls::ColourCorrectionMatrix)) {\n>>> -\t\tSpan const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n>>> +\t\tSpan const &coeffs =\n>>> +\t\t\t*metadata.get(controls::ColourCorrectionMatrix);\n>>> \t\tMatrix3d ccmSupplied(coeffs);\n>>> \t\tif (ccmSupplied.determinant() > eps)\n>>> \t\t\tccm = ccmSupplied;\n>>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> \tuint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n>>>\n>>> \tif (metadata.contains(controls::SensorBlackLevels)) {\n>>> -\t\tSpan levels = metadata.get(controls::SensorBlackLevels);\n>>> +\t\tSpan levels =\n>>> +\t\t\t*metadata.get(controls::SensorBlackLevels);\n>>>\n>>> \t\t/*\n>>> \t\t * The black levels control is specified in R, Gr, Gb, B order.\n>>> @@ -593,13 +596,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> \tTIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n>>>\n>>> \tif (metadata.contains(controls::AnalogueGain)) {\n>>> -\t\tfloat gain = metadata.get(controls::AnalogueGain);\n>>> +\t\tfloat gain = *metadata.get(controls::AnalogueGain);\n>>> \t\tuint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n>>> \t\tTIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n>>> \t}\n>>>\n>>> \tif (metadata.contains(controls::ExposureTime)) {\n>>> -\t\tfloat exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n>>> +\t\tfloat exposureTime = *metadata.get(controls::ExposureTime) / 1e6;\n>>> \t\tTIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n>>> \t}\n>>\n>> Let's see what's Laurent comment on the possible assertion, but the\n>> patch looks good to me and the few other nits can be changed when\n>> applying if you agree with them\n>>\n>> Reviewed-by: Jacopo Mondi \n>>\n>> Thanks\n>> j\n>>\n>>>\n>>> --\n>>> 2.34.1\n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 299F2BD161\n\tfor ;\n\tSat, 4 Jun 2022 20:50:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6ED2465635;\n\tSat, 4 Jun 2022 22:50:29 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.15])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57B22633A6\n\tfor ;\n\tSat, 4 Jun 2022 22:50:28 +0200 (CEST)","from [192.168.1.209] ([92.18.80.244]) by mail.gmx.net (mrgmx004\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id 1Mv2xO-1nfmnD2sjI-00qxlC\n\tfor\n\t; Sat, 04 Jun 2022 22:50:27 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654375829;\n\tbh=mvMHVlg+fKFP179lR9vnBobhNE5jq3Z+4EOXQ/oeWoE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=P9JjrlzMW7ozkj9nu3T8dZNBz98TeX7donQSPtwRP1OhwHUec78j7xlKI04rP4BTv\n\tzMq1YlnFZ+hIwd7KHEzZzJHgnHM2n0QBukv05E4VzO3PRDDHpLZKGVbfg+t0J5lbz2\n\tjDdBWEt+iULbn2tTVQW7AkIdFC1bJ5+f86ndzFlA5g6tt1T6nC9BVS6dD0HMjGHFTj\n\txPU72EJ8hwmguOPyHND8V9w2YhA5bfZpLEry2YOVlK/UV+osODhY6qbkjzDDrf0r8R\n\tWjFbQfm14sgXQ5FBNQfqIFj0XdQpFvWH4zDvEirMaEROnD2debVIu9hyrOAwS42key\n\t+4fdmyPMGV0bg==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1654375828;\n\tbh=mvMHVlg+fKFP179lR9vnBobhNE5jq3Z+4EOXQ/oeWoE=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=Xk7sklIsEt1EAuaRxWN7lRKwtptXqacPbtmdsfizHWGsetvmBO7bkEe5qmiPPVW1J\n\tJXzE7ve7zQ93bKhzQJsFlz08tercq6ecmK3C5v+TbvbbYgyTgCz/h54uE41TNzaC2x\n\tfQjMmiJVMaywQz0nDIUh7jNl2aTCdwhtnx2+OWvE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"Xk7sklIs\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<72d66ed0-1e73-5539-b661-96c63702482b@gmx.de>","Date":"Sat, 4 Jun 2022 21:50:27 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220603220712.73673-1-Rauch.Christian@gmx.de>\n\t<20220603220712.73673-2-Rauch.Christian@gmx.de>\n\t<20220604073113.yfpyfsljbagho4eg@uno.localdomain>\n\t<20220604082957.ft4nlsugu4zpn36o@uno.localdomain>","In-Reply-To":"<20220604082957.ft4nlsugu4zpn36o@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:zQzikcAp+9tCvy4dNpDW4QtT7cpA3QBELZdEzw9KjozEkQqK3vG\n\tjny6gDoStk4B7fMtYWqFkugx06zriA/lm3oWVm+6CdNrjYWbh94cAOufpiQ8hYkhCBopFEj\n\tL5+wno/+6TVh22F/NjFHnMoEkeB5svcaCI7k69qs2z6J9Up+GLOpb12uGqJkxAhKfdscncp\n\tAmGbHwXOPStQ0Hv2OOEVg==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:rHqa5FMgi/c=:IyE+RokdmMrOaPGO7hG3Zd\n\tTzw82t283bfCgNCUykjM7Dirb09oV4NBbAfxe2M3WZ+YJDfg2xcVB+czZMDeA0eZ0WNHvVJZ5\n\tHMaOB+d8ASQbhKnM0lBhCsv4Vg+ZhhtIzSyTqgjLEWhuij/09Q+aeSRp0j3BaJIAEXen46Nzx\n\tVHMIkGh1kfbOaRk+UsEiWxuJjEdXwywTFej0ABBv5wQ+K85kxs5rjy9msYdsjvyvAtTxtpuic\n\tVjhT6NbVLBQ8V3SafLgkM4Csm3uYSekuHH3vFuUX6Eb9PgIwaYTJo9AQ3yfzeZYm9eg8qa19E\n\tZq/4lVyFkayiyUNYUZ2zgUYw5ZuZTxhXMNK5kiYRLuNTO+2oDsykPjbx2hUHSIEowNjbR/GMr\n\twEq3gSuqs3pFpCK/dUzfRh3lRIzLN+Ntu9xZLxZtaT35yDKagcF/P009/CNDcDM7y68yJItvP\n\tFm+LS5EfoEf9I1cnMRRAC6h+x6uay7AR3uAURg77/7kkLAQvVJu+dZ/vnlj3QlxK03yE1nef5\n\tef1QOneNtapYjlUQP+UTXLVDY+Mhc8p/xT/OeF3qMqiqj2Z27sqaCkveJNj4Gx/coFCUCmVr7\n\tgbN/wp8DQQ43W30bihGIxTkfSGPgU4vitD8HdwG5DiTfC9zUJ2pGfF3+nX75JR4XRjexWIzs9\n\t/S7LsEMIQOy0gXHQc4yAsl+2tYYN5uQ0kLf1ooY/G/rl3yz4WhD1cKjRHKW5wGaRpLrGW2e0M\n\tQW+NyseAHaDsiOCZuO3nrhacQIk387VdJmdGEhglFWH/c01KheZ/XkLCTx1LQTfkLUBJUbpS1\n\t6rTMxx6YeDprvMTpCDvCstHKGYXdg5++vmjYVxXVt4XSXZM6Cf308N/5FkweIKQQspVFyKkpM\n\tM9JUKgQJhqZPkBTD3FQrNsFHtQ00qWsxD9PiBPcUrj1JZBTjdS1+DPocJxRwthVdwDODB0hoE\n\toy5BYWtjVtEbtaf2MRQsVAMujlqd3qvqJA5GLeMqqEDq4w5IyzEUA1J073cICVCsiqYb8lt9e\n\tQJgqS8qZibP8JyNHN+moze/XIYb/IXFeOobZdjZgOC1wu6IuFb61WE1zeXMa6CGAXujAecgcO\n\t4iLlUThePKn2UTU6oWhsu2dt3nOCGi9nfB4Ic7tL3wzavmyAVabNSzGFA==","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23322,"web_url":"https://patchwork.libcamera.org/comment/23322/","msgid":"<20220605091332.zptqbfwxac2ttcl4@uno.localdomain>","date":"2022-06-05T09:13:32","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Christian\n\nOn Sat, Jun 04, 2022 at 09:50:27PM +0100, Christian Rauch via libcamera-devel wrote:\n> Hi Jacopo,\n>\n> Am 04.06.22 um 09:29 schrieb Jacopo Mondi:\n> > Hi Christian,\n> > spoke too soon\n> >\n> > On Sat, Jun 04, 2022 at 09:31:13AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> >> Hi Christian\n> >>\n> >> On Fri, Jun 03, 2022 at 11:07:08PM +0100, Christian Rauch via libcamera-devel wrote:\n> >>> Previously, ControlList::get() would use default constructed objects to\n> >>> indicate that a ControlList does not have the requested Control. This has\n> >>> several disadvantages: 1) It requires types to be default constructible,\n> >>> 2) it does not differentiate between a default constructed object and an\n> >>> object that happens to have the same state as a default constructed object.\n> >>>\n> >>> std::optional additionally stores the information if the object is valid\n> >>> or not, and therefore is more expressive than a default constructed object.\n> >>>\n> >>> Signed-off-by: Christian Rauch \n> >>> ---\n> >>> include/libcamera/controls.h | 5 +++--\n> >>> src/android/camera_capabilities.cpp | 8 +++----\n> >>> src/android/camera_device.cpp | 21 +++++++++----------\n> >>> src/android/camera_hal_manager.cpp | 2 +-\n> >>> src/cam/main.cpp | 4 ++--\n> >>> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++----\n> >>> .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++++----\n> >>> src/qcam/dng_writer.cpp | 15 +++++++------\n> >>> 9 files changed, 39 insertions(+), 36 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>> index 665bcac1..192be784 100644\n> >>> --- a/include/libcamera/controls.h\n> >>> +++ b/include/libcamera/controls.h\n> >>> @@ -8,6 +8,7 @@\n> >>> #pragma once\n> >>>\n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -373,11 +374,11 @@ public:\n> >>> \tbool contains(unsigned int id) const;\n> >>>\n> >>> \ttemplate\n> >>> -\tT get(const Control &ctrl) const\n> >>> +\tstd::optional get(const Control &ctrl) const\n> >>> \t{\n> >>> \t\tconst ControlValue *val = find(ctrl.id());\n> >>> \t\tif (!val)\n> >>> -\t\t\treturn T{};\n> >>> +\t\t\treturn std::nullopt;\n> >>>\n> >>> \t\treturn val->get();\n> >>> \t}\n> >>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> >>> index 6f197eb8..892bbc2b 100644\n> >>> --- a/src/android/camera_capabilities.cpp\n> >>> +++ b/src/android/camera_capabilities.cpp\n> >>> @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> >>> \t/* Sensor static metadata. */\n> >>> \tstd::array pixelArraySize;\n> >>> \t{\n> >>> -\t\tconst Size &size = properties.get(properties::PixelArraySize);\n> >>> +\t\tconst Size &size = properties.get(properties::PixelArraySize).value_or(Size{});\n> >>\n> >> We should probably wrap this and a few similar cases above with an\n> >>\n> >> if (properties.contains(...))\n> >>\n> >> Not for this patch though.\n> >>\n> >>> \t\tpixelArraySize[0] = size.width;\n> >>> \t\tpixelArraySize[1] = size.height;\n> >>> \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> >>> @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> >>> \t}\n> >>>\n> >>> \tif (properties.contains(properties::UnitCellSize)) {\n> >>> -\t\tconst Size &cellSize = properties.get(properties::UnitCellSize);\n> >>> +\t\tconst Size &cellSize = *properties.get(properties::UnitCellSize);\n> >\n> > Here you could remove the template argument if I'm not\n> > mistaken. I know it was already there...\n> >\n> > But, most important, I get the following with clang 14.0.0\n> >\n> > (Showing with both and without)\n> >\n> > src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]\n> > const Size &cellSize = *properties.get(properties::UnitCellSize);\n> >\n> > src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]\n> > const Size &cellSize = *properties.get(properties::UnitCellSize);\n> >\n> > I guess the issue might be due to taking a reference to a temporary\n> > value as:\n> > const Size cellSize = *properties.get(properties::UnitCellSize);\n> >\n> > as well as:\n> > const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;\n> >\n> > compile fine.\n>\n> Thanks for pointing this out. I could reproduce this with clang\n> (\"CC=clang CXX=clang++ meson build -Dandroid=enabled\") but not with gcc.\n> Even \"-Dwarning_level=3\" did not show these errors.\n>\n> Does libcamera run a build server somewhere? This would be helpful to\n> discover errors in non-standard configurations (e.g. with clang in this\n> case or with \"android\" enabled).\n>\n\nUnfortunately not for public use. I run a buildbot instance as a best\neffort service for internal (aka people who can push to master)\npre-integration testing. It's not stable or maintained well enough\nfor public usage yet.\n\nWe do also have the linux-media Jenkins instance, but that only runs\non master afaict\n\n> >\n> > When it comes to the usage of value_or(), its documentation says:\n> > [1] https://en.cppreference.com/w/cpp/utility/optional/value_or\n> > -T must meet the requirements of CopyConstructible in order to use overload (1).\n> > -T must meet the requirements of MoveConstructible in order to use overload (2).\n> >\n> > which means the value is copied or moved back while operator*\n> > [2] https://en.cppreference.com/w/cpp/utility/optional/operator*\n> > 1) Returns a pointer to the contained value.\n> > 2) Returns a reference to the contained value.\n> >\n> > I guess the issue is due to trying to reference the content of a\n> > temporary std::optional instance, created to wrap the return value\n> > of ControlValue::get\n> >\n> > ControlList:\n> > \ttemplate\n> > \tstd::optional get(const Control &ctrl) const\n> > \t{\n> > \t\tconst ControlValue *val = find(ctrl.id());\n> > \t\tif (!val)\n> > \t\t\treturn std::nullopt;\n> >\n> > \t\treturn val->get();\n> > \t}\n> >\n> > This should be fine, as the object returned by ControlValue::get<>()\n> > refers to valid memory, and as we could take references to it before\n> > this series, we should be able to do so now, but the compiler worries\n> > that dereferencing the temporary std::optional<> would leave us with a\n> > pointer to invalid memory. Is my interpretation of the issue correct ?\n> >\n>\n> Yes, I think what is happening here is that the optional returns the\n> pointer to a value that can become \"invalid\" later. This could happen\n> when the \"std::optional::reset()\" is called or when \"std::nullopt\" is\n> assigned.\n>\n> Taking the reference of this would be dangerous. We know that the\n> reference will \"cellSize\" be destroyed shortly after, but generally,\n> there is no guarantee. For the compiler, it makes sense to complain\n> about this.\n>\n> I only see two ways to solve this:\n>\n> 1. copy the content:\n>\n> const Size cellSize = *properties.get(properties::UnitCellSize);\n>\n> 2. use the \"std::optional\" directly:\n>\n> const auto cellSize = properties.get(properties::UnitCellSize);\n>\n> and then access it via the \"->\" operator:\n>\n> cellSize->width\n>\n> I personally prefer the second option since it avoids copying the entire\n> content, which could be expensive for other control types.\n>\n\nI agree copies should be avoided as they can become expensive for\nlarger objects. We also have the issue that using value_or() might\nintroduce a copy operation that might get unnoticed..\n\nUsing std::optional<> directly might get a bit heavy for controls of\ntype Span<>\n\n\t\tconst auto &rects = properties.get(properties::PixelArrayActiveAreas);\n\t\tstd::vector data{\n\t\t\tstatic_cast((*rects)[0].x),\n\t\t\tstatic_cast((*rects)[0].y),\n\t\t\tstatic_cast((*rects)[0].width),\n\t\t\tstatic_cast((*rects)[0].height),\n\t\t};\n\nSpecifically because of the need to dereference the std::optiona<>\nbefore accessing its content with operator[]\n\nIt's not something huge, and I still think the series has ways more\npro than cons, but I wonder if we shouln't wrap std::optional<> in\nsome libcamera::controls specific type and expose that in the public\nAPI instead of the raw std::optional<>\n\nNot asking you to do so for this series of course.\n\nThanks\n j\n\n>\n> >>> \t\t\tcellSize.width * pixelArraySize[0] / 1e6f,\n> >>> \t\t\tcellSize.height * pixelArraySize[1] / 1e6f\n> >>> @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> >>>\n> >>> \t{\n> >>> \t\tconst Span &rects =\n> >>> -\t\t\tproperties.get(properties::PixelArrayActiveAreas);\n> >>> +\t\t\tproperties.get(properties::PixelArrayActiveAreas).value_or(Span{});\n> >>> \t\tstd::vector data{\n> >>> \t\t\tstatic_cast(rects[0].x),\n> >>> \t\t\tstatic_cast(rects[0].y),\n> >>> @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> >>>\n> >>> \t/* Report the color filter arrangement if the camera reports it. */\n> >>> \tif (properties.contains(properties::draft::ColorFilterArrangement)) {\n> >>> -\t\tuint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);\n> >>> +\t\tuint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);\n> >>> \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> >>> \t\t\t\t\t filterArr);\n> >>> \t}\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index 8e804d4d..ec117101 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> >>> \tconst ControlList &properties = camera_->properties();\n> >>>\n> >>> \tif (properties.contains(properties::Location)) {\n> >>> -\t\tint32_t location = properties.get(properties::Location);\n> >>> +\t\tint32_t location = *properties.get(properties::Location);\n> >>> \t\tswitch (location) {\n> >>> \t\tcase properties::CameraLocationFront:\n> >>> \t\t\tfacing_ = CAMERA_FACING_FRONT;\n> >>> @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> >>> \t * metadata.\n> >>> \t */\n> >>> \tif (properties.contains(properties::Rotation)) {\n> >>> -\t\tint rotation = properties.get(properties::Rotation);\n> >>> +\t\tint rotation = *properties.get(properties::Rotation);\n> >>> \t\torientation_ = (360 - rotation) % 360;\n> >>> \t\tif (cameraConfigData && cameraConfigData->rotation != -1 &&\n> >>> \t\t orientation_ != cameraConfigData->rotation) {\n> >>> @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)\n> >>> \t * as soon as possible, earlier than request completion time.\n> >>> \t */\n> >>> \tuint64_t sensorTimestamp = static_cast(request->metadata()\n> >>> -\t\t\t\t\t\t\t .get(controls::SensorTimestamp));\n> >>> +\t\t\t\t\t\t\t\t .get(controls::SensorTimestamp)\n> >>> +\t\t\t\t\t\t\t\t .value_or(0));\n> >>> \tnotifyShutter(descriptor->frameNumber_, sensorTimestamp);\n> >>>\n> >>> \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> >>> @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> >>> \t\t\t\t rolling_shutter_skew);\n> >>>\n> >>> \t/* Add metadata tags reported by libcamera. */\n> >>> -\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp);\n> >>> +\tconst int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0);\n> >>> \tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp);\n> >>>\n> >>> \tif (metadata.contains(controls::draft::PipelineDepth)) {\n> >>> -\t\tuint8_t pipeline_depth =\n> >>> -\t\t\tmetadata.get(controls::draft::PipelineDepth);\n> >>> +\t\tuint8_t pipeline_depth = *metadata.get(controls::draft::PipelineDepth);\n> >>> \t\tresultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,\n> >>> \t\t\t\t\t pipeline_depth);\n> >>> \t}\n> >>>\n> >>> \tif (metadata.contains(controls::ExposureTime)) {\n> >>> -\t\tint64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;\n> >>> +\t\tint64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;\n> >>> \t\tresultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);\n> >>> \t}\n> >>>\n> >>> \tif (metadata.contains(controls::FrameDuration)) {\n> >>> -\t\tint64_t duration = metadata.get(controls::FrameDuration) * 1000;\n> >>> +\t\tint64_t duration = *metadata.get(controls::FrameDuration) * 1000;\n> >>> \t\tresultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,\n> >>> \t\t\t\t\t duration);\n> >>> \t}\n> >>>\n> >>> \tif (metadata.contains(controls::ScalerCrop)) {\n> >>> -\t\tRectangle crop = metadata.get(controls::ScalerCrop);\n> >>> +\t\tRectangle crop = *metadata.get(controls::ScalerCrop);\n> >>> \t\tint32_t cropRect[] = {\n> >>> \t\t\tcrop.x, crop.y, static_cast(crop.width),\n> >>> \t\t\tstatic_cast(crop.height),\n> >>> @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> >>> \t}\n> >>>\n> >>> \tif (metadata.contains(controls::draft::TestPatternMode)) {\n> >>> -\t\tconst int32_t testPatternMode =\n> >>> -\t\t\tmetadata.get(controls::draft::TestPatternMode);\n> >>> +\t\tconst int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);\n> >>> \t\tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> >>> \t\t\t\t\t testPatternMode);\n> >>> \t}\n> >>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> >>> index 5f7bfe26..0b23397a 100644\n> >>> --- a/src/android/camera_hal_manager.cpp\n> >>> +++ b/src/android/camera_hal_manager.cpp\n> >>> @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> >>> \tif (!properties.contains(properties::Location))\n> >>> \t\treturn -1;\n> >>>\n> >>> -\treturn properties.get(properties::Location);\n> >>> +\treturn properties.get(properties::Location).value_or(0);\n> >>\n> >> No need to value_or() as we return earlier if Location is not\n> >> available\n> >>\n> >>> }\n> >>>\n> >>> CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n> >>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> >>> index 79875ed7..d8115cd8 100644\n> >>> --- a/src/cam/main.cpp\n> >>> +++ b/src/cam/main.cpp\n> >>> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >>> \t * is only used if the location isn't present or is set to External.\n> >>> \t */\n> >>> \tif (props.contains(properties::Location)) {\n> >>> -\t\tswitch (props.get(properties::Location)) {\n> >>> +\t\tswitch (*props.get(properties::Location)) {\n> >>> \t\tcase properties::CameraLocationFront:\n> >>> \t\t\taddModel = false;\n> >>> \t\t\tname = \"Internal front camera \";\n> >>> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >>> \t\t * If the camera location is not availble use the camera model\n> >>> \t\t * to build the camera name.\n> >>> \t\t */\n> >>> -\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n> >>> +\t\tname = \"'\" + *props.get(properties::Model) + \"' \";\n> >>> \t}\n> >>>\n> >>> \tname += \"(\" + camera->id() + \")\";\n> >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>> index 3b126bb5..f65a0680 100644\n> >>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> >>>\n> >>> void IPARPi::prepareISP(const ISPConfig &data)\n> >>> {\n> >>> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> >>> +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n> >>\n> >> This should in theory be guaranteed to be there, as it's the pipeline\n> >> handler that populates it. However I'm not sure what's best. Either\n> >> ASSERT() that the control is there or default it to 0. frameTimestamp\n> >> is not used as a divider anywhere so there's no risk of undefined\n> >> behaviour.\n> >>\n> >>> \tRPiController::Metadata lastMetadata;\n> >>> \tSpan embeddedBuffer;\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index fd989e61..53332826 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()\n> >>> \t\t/* Convert the sensor rotation to a transformation */\n> >>> \t\tint32_t rotation = 0;\n> >>> \t\tif (data->properties_.contains(properties::Rotation))\n> >>> -\t\t\trotation = data->properties_.get(properties::Rotation);\n> >>> +\t\t\trotation = *(data->properties_.get(properties::Rotation));\n> >>> \t\telse\n> >>> \t\t\tLOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> >>> \t\t\t\t\t << cio2->sensor()->id()\n> >>> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> >>> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> >>> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> >>> \tif (request->controls().contains(controls::ScalerCrop))\n> >>> -\t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n> >>> +\t\tcropRegion_ = *(request->controls().get(controls::ScalerCrop));\n> >>> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n> >>>\n> >>> \tif (frameInfos_.tryComplete(info))\n> >>> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>> \t\treturn;\n> >>> \t}\n> >>>\n> >>> -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> >>> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),\n> >>> \t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n> >>\n> >> Also in this case we should be guaranteed the sensor timestamp is\n> >> there.\n> >>\n> >> I'm tempted to ASSERT here as well...\n> >>\n> >>> }\n> >>>\n> >>> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> >>> \tif (!request->controls().contains(controls::draft::TestPatternMode))\n> >>> \t\treturn;\n> >>>\n> >>> -\tconst int32_t testPatternMode = request->controls().get(\n> >>> -\t\tcontrols::draft::TestPatternMode);\n> >>> +\tconst int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(0);\n> >>\n> >> No need as we return earlier if !TestPatternMode\n> >>\n> >>>\n> >>> \tint ret = cio2_.sensor()->setTestPatternMode(\n> >>> \t\tstatic_cast(testPatternMode));\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> index adc397e8..a62afdd4 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> >>> \t * error means the platform can never run. Let's just print a warning\n> >>> \t * and continue regardless; the rotation is effectively set to zero.\n> >>> \t */\n> >>> -\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> >>> +\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);\n> >>> \tbool success;\n> >>> \tTransform rotationTransform = transformFromRotation(rotation, &success);\n> >>> \tif (!success)\n> >>> @@ -1706,7 +1706,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> >>> \t * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> >>> \t */\n> >>> \tif (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n> >>> -\t\tlibcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n> >>> +\t\tlibcamera::Span colourGains =\n> >>> +\t\t\t*controls.get(libcamera::controls::ColourGains);\n> >>> \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n> >>> \t\tControlList ctrls(sensor_->controls());\n> >>> \t\tstd::array gains{\n> >>> @@ -2041,7 +2042,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> >>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> >>> {\n> >>> \tif (controls.contains(controls::ScalerCrop)) {\n> >>> -\t\tRectangle nativeCrop = controls.get(controls::ScalerCrop);\n> >>> +\t\tRectangle nativeCrop = *controls.get(controls::ScalerCrop);\n> >>>\n> >>> \t\tif (!nativeCrop.width || !nativeCrop.height)\n> >>> \t\t\tnativeCrop = { 0, 0, 1, 1 };\n> >>> @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> >>> \t\t\t\t\tRequest *request)\n> >>> {\n> >>> \trequest->metadata().set(controls::SensorTimestamp,\n> >>> -\t\t\t\tbufferControls.get(controls::SensorTimestamp));\n> >>> +\t\t\t\tbufferControls.get(controls::SensorTimestamp).value_or(0));\n> >>>\n> >>> \trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> >>> }\n> >>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> >>> index 34c8df5a..780f58f7 100644\n> >>> --- a/src/qcam/dng_writer.cpp\n> >>> +++ b/src/qcam/dng_writer.cpp\n> >>> @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> \tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> >>>\n> >>> \tif (cameraProperties.contains(properties::Model)) {\n> >>> -\t\tstd::string model = cameraProperties.get(properties::Model);\n> >>> +\t\tstd::string model = *cameraProperties.get(properties::Model);\n> >>> \t\tTIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n> >>> \t\t/* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> >>> \t}\n> >>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> \tconst double eps = 1e-2;\n> >>>\n> >>> \tif (metadata.contains(controls::ColourGains)) {\n> >>> -\t\tSpan const &colourGains = metadata.get(controls::ColourGains);\n> >>> +\t\tSpan const &colourGains =\n> >>> +\t\t\t*metadata.get(controls::ColourGains);\n> >>> \t\tif (colourGains[0] > eps && colourGains[1] > eps) {\n> >>> \t\t\twbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n> >>> \t\t\tneutral[0] = 1.0 / colourGains[0]; /* red */\n> >>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> \t\t}\n> >>> \t}\n> >>> \tif (metadata.contains(controls::ColourCorrectionMatrix)) {\n> >>> -\t\tSpan const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n> >>> +\t\tSpan const &coeffs =\n> >>> +\t\t\t*metadata.get(controls::ColourCorrectionMatrix);\n> >>> \t\tMatrix3d ccmSupplied(coeffs);\n> >>> \t\tif (ccmSupplied.determinant() > eps)\n> >>> \t\t\tccm = ccmSupplied;\n> >>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> \tuint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n> >>>\n> >>> \tif (metadata.contains(controls::SensorBlackLevels)) {\n> >>> -\t\tSpan levels = metadata.get(controls::SensorBlackLevels);\n> >>> +\t\tSpan levels =\n> >>> +\t\t\t*metadata.get(controls::SensorBlackLevels);\n> >>>\n> >>> \t\t/*\n> >>> \t\t * The black levels control is specified in R, Gr, Gb, B order.\n> >>> @@ -593,13 +596,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> \tTIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n> >>>\n> >>> \tif (metadata.contains(controls::AnalogueGain)) {\n> >>> -\t\tfloat gain = metadata.get(controls::AnalogueGain);\n> >>> +\t\tfloat gain = *metadata.get(controls::AnalogueGain);\n> >>> \t\tuint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n> >>> \t\tTIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n> >>> \t}\n> >>>\n> >>> \tif (metadata.contains(controls::ExposureTime)) {\n> >>> -\t\tfloat exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n> >>> +\t\tfloat exposureTime = *metadata.get(controls::ExposureTime) / 1e6;\n> >>> \t\tTIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> >>> \t}\n> >>\n> >> Let's see what's Laurent comment on the possible assertion, but the\n> >> patch looks good to me and the few other nits can be changed when\n> >> applying if you agree with them\n> >>\n> >> Reviewed-by: Jacopo Mondi \n> >>\n> >> Thanks\n> >> j\n> >>\n> >>>\n> >>> --\n> >>> 2.34.1\n> >>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A3E77BD160\n\tfor ;\n\tSun, 5 Jun 2022 09:13:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E97565635;\n\tSun, 5 Jun 2022 11:13:36 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E56CA633A5\n\tfor ;\n\tSun, 5 Jun 2022 11:13:34 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4108CFF802;\n\tSun, 5 Jun 2022 09:13:33 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654420416;\n\tbh=5vXz4kk5nTsUfm3O8opSW59GI4iyQHsKjga0TryWDjw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=LzSvGej4rDzkRD3JJpXwh/Gf8qTTOUOJ17iTWOP+V7UFF3Oa0dF07j+iEQl0be8GV\n\tY7qBtWtDp37A4yXgiYa5Oo09WPXdFLSztgzggPfb2N7Jl7XUttDD8ll+5eHuUgwMO7\n\t2mLMkduoD/f0njb4Ju+ejNrJcPIJvLotTHm1j9TQ/PGhQJy2/53PwMgRtz9Qs2fbhW\n\t/arePOkkquFHd5SJS/xbyOO0JBC6YtS3PymQdVxwRm8ofmvxVzrTV3G7np4mG+EC5s\n\tomSTSQE2NX7iX2CNWZZIn6o8qnxxasQYQoByh7kMRZ/Rwok0dztC3rNZ4sXdl2wwvv\n\trU2JFKUVhsM4A==","Date":"Sun, 5 Jun 2022 11:13:32 +0200","To":"Christian Rauch ","Message-ID":"<20220605091332.zptqbfwxac2ttcl4@uno.localdomain>","References":"<20220603220712.73673-1-Rauch.Christian@gmx.de>\n\t<20220603220712.73673-2-Rauch.Christian@gmx.de>\n\t<20220604073113.yfpyfsljbagho4eg@uno.localdomain>\n\t<20220604082957.ft4nlsugu4zpn36o@uno.localdomain>\n\t<72d66ed0-1e73-5539-b661-96c63702482b@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<72d66ed0-1e73-5539-b661-96c63702482b@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23328,"web_url":"https://patchwork.libcamera.org/comment/23328/","msgid":"<165445565345.3726444.5487039785708085574@Monstersaurus>","date":"2022-06-05T19:00:53","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi All,\n\nQuoting Jacopo Mondi via libcamera-devel (2022-06-05 10:13:32)\n> > > I guess the issue might be due to taking a reference to a temporary\n> > > value as:\n> > > const Size cellSize = *properties.get(properties::UnitCellSize);\n> > >\n> > > as well as:\n> > > const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;\n> > >\n> > > compile fine.\n> >\n> > Thanks for pointing this out. I could reproduce this with clang\n> > (\"CC=clang CXX=clang++ meson build -Dandroid=enabled\") but not with gcc.\n> > Even \"-Dwarning_level=3\" did not show these errors.\n> >\n> > Does libcamera run a build server somewhere? This would be helpful to\n> > discover errors in non-standard configurations (e.g. with clang in this\n> > case or with \"android\" enabled).\n> >\n> \n> Unfortunately not for public use. I run a buildbot instance as a best\n> effort service for internal (aka people who can push to master)\n> pre-integration testing. It's not stable or maintained well enough\n> for public usage yet.\n> \n> We do also have the linux-media Jenkins instance, but that only runs\n> on master afaict\n\nAnd I have my build machine too.\n\nI think one possible solution would be to get a gitlab instance on\nfreedesktop.org and add a .gitlab-ci.yml - this could then trigger build\njobs for others to push to a gitlab instance.\n\nI tried setting up and maintaining my own gitlab instance, but that\nlasted a week before it fell over :S ... so I don't want to run one\nmyself. If we can use an external such as F.D.O that will help.\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 971AEBD161\n\tfor ;\n\tSun, 5 Jun 2022 19:00:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E70FC65634;\n\tSun, 5 Jun 2022 21:00:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BCE160104\n\tfor ;\n\tSun, 5 Jun 2022 21:00:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0C400496;\n\tSun, 5 Jun 2022 21:00:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654455657;\n\tbh=ZMGVgszBuQIM61fQOXLmZJTBUwqrg8+AJIrIJ5XNjLw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=bJ6rVhhye2xMCnxPEhMJ0f33wInKfYfh/mpUnMDh4aD1byYhidkMis/8v3HdUUmK4\n\tpV1V8FH9xk9BPFlvVk436umWOzBjOobIJDtnRI6HYpXp0Vs+QIj1j/gtjch3sL+rcM\n\tsKBImeT/tsEnB/LS/y9sSycrFTrinNURL0ugQ5yIXQbWr4CPLLrDwO1LVZ2Q8/XySA\n\ta/JJ525+pi2HkZyAHYBrP1ArgmN9m/0vcstLhEuS7uz8XfsBHr7hp4OxHnZRHFC/a+\n\tfYCh58qMz2kfXu15rEqYwkfKajFa+vPGZV9fJweS2640bc/pMDLWPvWVdmM94TI2EL\n\tKT/ZSNTmz+18w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654455656;\n\tbh=ZMGVgszBuQIM61fQOXLmZJTBUwqrg8+AJIrIJ5XNjLw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=feXFI+XlHnw9jywvOawRienHldSyXkziU0P+ex0NBMSL8csHfHtcJCS/WPB6BVpBn\n\t3n96NlQKZTMKez8RMgKwlygUzZtFnz9kSCR/sPt+huTlmQ/ITjGnOGvt8mV8KMiX1V\n\tYWJPwF+Tzjk58Kh42jpqZbkSIsALmA5hPNOSLjj8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"feXFI+Xl\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220605091332.zptqbfwxac2ttcl4@uno.localdomain>","References":"<20220603220712.73673-1-Rauch.Christian@gmx.de>\n\t<20220603220712.73673-2-Rauch.Christian@gmx.de>\n\t<20220604073113.yfpyfsljbagho4eg@uno.localdomain>\n\t<20220604082957.ft4nlsugu4zpn36o@uno.localdomain>\n\t<72d66ed0-1e73-5539-b661-96c63702482b@gmx.de>\n\t<20220605091332.zptqbfwxac2ttcl4@uno.localdomain>","To":"Christian Rauch ,\n\tJacopo Mondi , Jacopo Mondi via libcamera-devel\n\t","Date":"Sun, 05 Jun 2022 20:00:53 +0100","Message-ID":"<165445565345.3726444.5487039785708085574@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]