[{"id":22672,"web_url":"https://patchwork.libcamera.org/comment/22672/","msgid":"<164941960610.22830.16120063966032672613@Monstersaurus>","date":"2022-04-08T12:06:46","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Christian,\n\nQuoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)\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\nThis looks like a really good way to express the controls from a list. I\nreally like the value_or() that it brings to allow the code to set a\ndefault.\n\n\nI expect this will have knock-on effects to other out of tree\napplications using the control framework so we might want to coordinate\nthe merge of this.\n\nThough I notice there's fairly minimal changes to cam and qcam. Do you\nknow if your build includes the v4l2 adaptation layer and gstreamer?\nDoes this API change cause definate breakage to users?\n\n(It's ok if it does, that's preciesly why we are not ABI stable).\n\n\n> \n> Signed-off-by: Christian Rauch \n> ---\n> include/libcamera/controls.h | 6 +++---\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> 6 files changed, 24 insertions(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 665bcac1..57b777e9 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -167,7 +167,7 @@ public:\n> \n> using V = typename T::value_type;\n> const V *value = reinterpret_cast(data().data());\n> - return { value, numElements_ };\n> + return T{ value, numElements_ };\n> }\n> \n> #ifndef __DOXYGEN__\n> @@ -373,11 +373,11 @@ public:\n> bool contains(unsigned int id) const;\n> \n> template\n> - T get(const Control &ctrl) const\n> + std::optional get(const Control &ctrl) const\n> {\n> const ControlValue *val = find(ctrl.id());\n> if (!val)\n> - return T{};\n> + return std::nullopt;\n> \n> return val->get();\n> }\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index c7f664b9..853a78ed 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -293,7 +293,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> * is only used if the location isn't present or is set to External.\n> */\n> if (props.contains(properties::Location)) {\n> - switch (props.get(properties::Location)) {\n> + switch (props.get(properties::Location).value_or(int32_t{})) {\n\nIs there a way to do this without the value_or() in conditions where the\nvalue has already been guaranteed to exist?\n\nHere we have just checked that the lists contains a\nproperties::Lcoation, so we 'know' that it will never process the\n'_or()' part.\n\nLooking at https://en.cppreference.com/w/cpp/utility/optional\n\nI guess we can just use .value() in the locations where we already check\nfor the presence. I suspect this could lead to a code refactor to just\nuse the optional to determine the properties existance instead of\n.contains() - but that could certainly be done on top.\n\nPerhaps it might be better for consistency to use the value_or() variant\non occasions though - even if we know it must already exist?\n\n\n> case properties::CameraLocationFront:\n> addModel = false;\n> name = \"Internal front camera \";\n> @@ -313,7 +313,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> * If the camera location is not availble use the camera model\n> * to build the camera name.\n> */\n> - name = \"'\" + props.get(properties::Model) + \"' \";\n> + name = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n> }\n> \n> name += \"(\" + camera->id() + \")\";\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5a5cdf66..93b32e94 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> \n> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> {\n> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n> RPiController::Metadata lastMetadata;\n> Span embeddedBuffer;\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 60e01917..394221cb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1146,7 +1146,7 @@ int PipelineHandlerIPU3::registerCameras()\n> /* Convert the sensor rotation to a transformation */\n> int32_t rotation = 0;\n> if (data->properties_.contains(properties::Rotation))\n> - rotation = data->properties_.get(properties::Rotation);\n> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});\n> else\n> LOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> << cio2->sensor()->id()\n> @@ -1341,7 +1341,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> request->metadata().set(controls::draft::PipelineDepth, 3);\n> /* \\todo Actually apply the scaler crop region to the ImgU. */\n> if (request->controls().contains(controls::ScalerCrop))\n> - cropRegion_ = request->controls().get(controls::ScalerCrop);\n> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n> request->metadata().set(controls::ScalerCrop, cropRegion_);\n> \n> if (frameInfos_.tryComplete(info))\n> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> ev.op = ipa::ipu3::EventStatReady;\n> ev.frame = info->id;\n> ev.bufferId = info->statBuffer->cookie();\n> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n> ev.sensorControls = info->effectiveSensorControls;\n> ipa_->processEvent(ev);\n> }\n> @@ -1477,8 +1477,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> if (!request->controls().contains(controls::draft::TestPatternMode))\n> return;\n> \n> - const int32_t testPatternMode = request->controls().get(\n> - controls::draft::TestPatternMode);\n> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});\n\nThis looks like a section of code that could now use optional for\ncleaner code I think. I see above we return early if the control is not\npresent, and only call setTestPatternMode if it is set.\n\n\nAgain, I think this patch is just bringing in the std::optional - so it\nshouldn't have to 'make everything use the best implementation' - but I\ncan see benefits it can bring.\n\nI think even though we know it's guaranteed to exist here, the use of\nvalue_or() is fine with me, as it highlights that this code could\nperhaps be simplified later.\n\n\n> \n> int ret = cio2_.sensor()->setTestPatternMode(\n> static_cast(testPatternMode));\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0fa294d4..63d57033 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> * error means the platform can never run. Let's just print a warning\n> * and continue regardless; the rotation is effectively set to zero.\n> */\n> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});\n> bool success;\n> Transform rotationTransform = transformFromRotation(rotation, &success);\n> if (!success)\n> @@ -1696,7 +1696,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> */\n> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n> - libcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n> + libcamera::Span colourGains =\n> + controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> /* The control wants linear gains in the order B, Gb, Gr, R. */\n> ControlList ctrls(sensor_->controls());\n> std::array gains{\n> @@ -2031,7 +2032,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> {\n> if (controls.contains(controls::ScalerCrop)) {\n> - Rectangle nativeCrop = controls.get(controls::ScalerCrop);\n> + Rectangle nativeCrop = controls.get(controls::ScalerCrop).value_or(Rectangle{});\n\nI'm starting to wonder if a templated get_or would be useful as the type\nwould be defined there (doesn't have to be here, just an idea)\n\nIt would reduce line length on null initialisers:\n\n controls.get_or(controls::ScalerCrop, {});\n\nAnd easily allow default parameters to be defined:\n\n controls.get_or(controls::ScalerCrop, {640,480});\n\nI suspect seeing how this all gets used will determine if it has value\nthough.\n\n> \n> if (!nativeCrop.width || !nativeCrop.height)\n> nativeCrop = { 0, 0, 1, 1 };\n> @@ -2069,7 +2070,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> Request *request)\n> {\n> request->metadata().set(controls::SensorTimestamp,\n> - bufferControls.get(controls::SensorTimestamp));\n> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));\n> \n> request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> }\n> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> index 2fb527d8..030432e3 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> TIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> \n> if (cameraProperties.contains(properties::Model)) {\n> - std::string model = cameraProperties.get(properties::Model);\n> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});\n> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> }\n> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> const double eps = 1e-2;\n> \n> if (metadata.contains(controls::ColourGains)) {\n> - Span const &colourGains = metadata.get(controls::ColourGains);\n> + Span const &colourGains =\n> + metadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> if (colourGains[0] > eps && colourGains[1] > eps) {\n> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n> neutral[0] = 1.0 / colourGains[0]; /* red */\n> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> }\n> }\n> if (metadata.contains(controls::ColourCorrectionMatrix)) {\n> - Span const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n> + Span const &coeffs =\n> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\n> Matrix3d ccmSupplied(coeffs);\n> if (ccmSupplied.determinant() > eps)\n> ccm = ccmSupplied;\n> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n> \n> if (metadata.contains(controls::SensorBlackLevels)) {\n> - Span levels = metadata.get(controls::SensorBlackLevels);\n> + Span levels =\n> + metadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\n> \n> /*\n> * 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> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n> \n> if (metadata.contains(controls::AnalogueGain)) {\n> - float gain = metadata.get(controls::AnalogueGain);\n> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});\n> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n> }\n> \n> if (metadata.contains(controls::ExposureTime)) {\n> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;\n> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> }\n> \n> --\n> 2.25.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 476AAC0F1B\n\tfor ;\n\tFri, 8 Apr 2022 12:06:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8547D65644;\n\tFri, 8 Apr 2022 14:06:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 95F6A604BA\n\tfor ;\n\tFri, 8 Apr 2022 14:06:49 +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 19A89499;\n\tFri, 8 Apr 2022 14:06:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649419610;\n\tbh=J8NM6qQ/6KkIsZiE5WDwMor0ofcKH82Dvk035/VWZBI=;\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:\n\tFrom;\n\tb=AEg2ssrLekSnkGTBfQa9f19ULv6My8dJusebQPhF0Q0WIqJNQAm9fZZ5kEw3cVfEF\n\tNjue8AoLkffhEw0DKWGHHQUuyS5GUX6GsYRtTgJbhSUkF7zTR3PceMpp39hOR08Ewq\n\tQ+j36+bd1yvVHRuFQjexwYJcgkafYZZkWUtubmdqGt5zUyZm2cM1KMxj9oOXHxWll3\n\tNUfYUIy3PVf8QAHACcLPlSJnnhvTnBqmrB0vVYl1AesfXhoaZGayadwoJbq5jMFgQw\n\tGQVB52pFBpaEp+VUeV4lY4C0Yf0U9q4vOdP6+jzsU1oehp2yNJEZMVQ3wllBYn4X8k\n\tfOIHEpL2uhY4Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649419609;\n\tbh=J8NM6qQ/6KkIsZiE5WDwMor0ofcKH82Dvk035/VWZBI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=o4lUdTAfrwkEZi8dXoQdqEDsPtXjhhURh0gAXlVm9W+62Qx9bqCKIUwTgI6+56gKj\n\tjCPzYnyLIsP/9gZrbhsm1W6TB1rh6vBAEj7IPgKA88nzZce8YDfU9mlzKwaH4L+JRY\n\tDGZ/gP6XcNhqXYih6y49HbrXdSpagLtTYxgJZiE4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"o4lUdTAf\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220408014231.231083-5-Rauch.Christian@gmx.de>","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>","To":"Christian Rauch ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 08 Apr 2022 13:06:46 +0100","Message-ID":"<164941960610.22830.16120063966032672613@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22674,"web_url":"https://patchwork.libcamera.org/comment/22674/","msgid":"<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>","date":"2022-04-08T22:29:51","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Kieran,\n\nAm 08.04.22 um 13:06 schrieb Kieran Bingham:\n> Hi Christian,\n>\n> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)\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> This looks like a really good way to express the controls from a list. I\n> really like the value_or() that it brings to allow the code to set a\n> default.\n>\n>\n> I expect this will have knock-on effects to other out of tree\n> applications using the control framework so we might want to coordinate\n> the merge of this.\n>\n> Though I notice there's fairly minimal changes to cam and qcam. Do you\n> know if your build includes the v4l2 adaptation layer and gstreamer?\n> Does this API change cause definate breakage to users?\n>\n> (It's ok if it does, that's preciesly why we are not ABI stable).\n\nThis is definitely a breaking change as it changes the public API. But\nthe changes that have to be made are quite trivial. You only have to add\n\".value()\" or \".value_or(...)\" to the old code.\n\nI don't know about the v4l2 wrapper and gstreamer. There might be some\ncode that is not compiled on my setup. But the \"qcam\" application still\nworks. And I think this one is using the v4l2 wrapper.\n\n>\n>\n>>\n>> Signed-off-by: Christian Rauch \n>> ---\n>> include/libcamera/controls.h | 6 +++---\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>> 6 files changed, 24 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 665bcac1..57b777e9 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -167,7 +167,7 @@ public:\n>>\n>> using V = typename T::value_type;\n>> const V *value = reinterpret_cast(data().data());\n>> - return { value, numElements_ };\n>> + return T{ value, numElements_ };\n>> }\n>>\n>> #ifndef __DOXYGEN__\n>> @@ -373,11 +373,11 @@ public:\n>> bool contains(unsigned int id) const;\n>>\n>> template\n>> - T get(const Control &ctrl) const\n>> + std::optional get(const Control &ctrl) const\n>> {\n>> const ControlValue *val = find(ctrl.id());\n>> if (!val)\n>> - return T{};\n>> + return std::nullopt;\n>>\n>> return val->get();\n>> }\n>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n>> index c7f664b9..853a78ed 100644\n>> --- a/src/cam/main.cpp\n>> +++ b/src/cam/main.cpp\n>> @@ -293,7 +293,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>> * is only used if the location isn't present or is set to External.\n>> */\n>> if (props.contains(properties::Location)) {\n>> - switch (props.get(properties::Location)) {\n>> + switch (props.get(properties::Location).value_or(int32_t{})) {\n>\n> Is there a way to do this without the value_or() in conditions where the\n> value has already been guaranteed to exist?\n>\n> Here we have just checked that the lists contains a\n> properties::Lcoation, so we 'know' that it will never process the\n> '_or()' part.\n>\n> Looking at https://en.cppreference.com/w/cpp/utility/optional\n>\n> I guess we can just use .value() in the locations where we already check\n> for the presence. I suspect this could lead to a code refactor to just\n> use the optional to determine the properties existance instead of\n> .contains() - but that could certainly be done on top.\n>\n> Perhaps it might be better for consistency to use the value_or() variant\n> on occasions though - even if we know it must already exist?\n\n\".value()\" will throw an exception if the \"std::optional\" does not\ncontain a value. If you can guarantee that a ControlValue contains a\nvalue, then you can skip the check via \".has_value()\" or the fallback\nvia \".value_or(...)\" and use \".value()\" directly.\n\n>\n>\n>> case properties::CameraLocationFront:\n>> addModel = false;\n>> name = \"Internal front camera \";\n>> @@ -313,7 +313,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>> * If the camera location is not availble use the camera model\n>> * to build the camera name.\n>> */\n>> - name = \"'\" + props.get(properties::Model) + \"' \";\n>> + name = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n>> }\n>>\n>> name += \"(\" + camera->id() + \")\";\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 5a5cdf66..93b32e94 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>>\n>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>> {\n>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n>> RPiController::Metadata lastMetadata;\n>> Span embeddedBuffer;\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 60e01917..394221cb 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -1146,7 +1146,7 @@ int PipelineHandlerIPU3::registerCameras()\n>> /* Convert the sensor rotation to a transformation */\n>> int32_t rotation = 0;\n>> if (data->properties_.contains(properties::Rotation))\n>> - rotation = data->properties_.get(properties::Rotation);\n>> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});\n>> else\n>> LOG(IPU3, Warning) << \"Rotation control not exposed by \"\n>> << cio2->sensor()->id()\n>> @@ -1341,7 +1341,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>> request->metadata().set(controls::draft::PipelineDepth, 3);\n>> /* \\todo Actually apply the scaler crop region to the ImgU. */\n>> if (request->controls().contains(controls::ScalerCrop))\n>> - cropRegion_ = request->controls().get(controls::ScalerCrop);\n>> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n>> request->metadata().set(controls::ScalerCrop, cropRegion_);\n>>\n>> if (frameInfos_.tryComplete(info))\n>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>> ev.op = ipa::ipu3::EventStatReady;\n>> ev.frame = info->id;\n>> ev.bufferId = info->statBuffer->cookie();\n>> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n>> ev.sensorControls = info->effectiveSensorControls;\n>> ipa_->processEvent(ev);\n>> }\n>> @@ -1477,8 +1477,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n>> if (!request->controls().contains(controls::draft::TestPatternMode))\n>> return;\n>>\n>> - const int32_t testPatternMode = request->controls().get(\n>> - controls::draft::TestPatternMode);\n>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});\n>\n> This looks like a section of code that could now use optional for\n> cleaner code I think. I see above we return early if the control is not\n> present, and only call setTestPatternMode if it is set.\n>\n>\n> Again, I think this patch is just bringing in the std::optional - so it\n> shouldn't have to 'make everything use the best implementation' - but I\n> can see benefits it can bring.\n>\n> I think even though we know it's guaranteed to exist here, the use of\n> value_or() is fine with me, as it highlights that this code could\n> perhaps be simplified later.\n>\n\nThe current \".value_or(...)\" implementation is the closest to the old\nbehaviour, which would return a default contructed object in case of\nfailure. You certainly can change that behaviour if you arec ertain that\na value exists.\n\n>\n>>\n>> int ret = cio2_.sensor()->setTestPatternMode(\n>> static_cast(testPatternMode));\n>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> index 0fa294d4..63d57033 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>> * error means the platform can never run. Let's just print a warning\n>> * and continue regardless; the rotation is effectively set to zero.\n>> */\n>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});\n>> bool success;\n>> Transform rotationTransform = transformFromRotation(rotation, &success);\n>> if (!success)\n>> @@ -1696,7 +1696,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n>> */\n>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n>> - libcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n>> + libcamera::Span colourGains =\n>> + controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n>> /* The control wants linear gains in the order B, Gb, Gr, R. */\n>> ControlList ctrls(sensor_->controls());\n>> std::array gains{\n>> @@ -2031,7 +2032,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n>> {\n>> if (controls.contains(controls::ScalerCrop)) {\n>> - Rectangle nativeCrop = controls.get(controls::ScalerCrop);\n>> + Rectangle nativeCrop = controls.get(controls::ScalerCrop).value_or(Rectangle{});\n>\n> I'm starting to wonder if a templated get_or would be useful as the type\n> would be defined there (doesn't have to be here, just an idea)\n>\n> It would reduce line length on null initialisers:\n>\n> controls.get_or(controls::ScalerCrop, {});\n>\n> And easily allow default parameters to be defined:\n>\n> controls.get_or(controls::ScalerCrop, {640,480});\n>\n> I suspect seeing how this all gets used will determine if it has value\n> though.\n\nThis is probably dependent on the situation, but I don't think that\ninitialising control values with the default is a good idea in every\ncase. The biggest advantage of \"std::optional\" is that you can properly\ntest for errors. In most cases, it is probably better to notify the user\nabout missing controls etc. instead of silently replacing the requested\nvalues with the defaults.\n\n>\n>>\n>> if (!nativeCrop.width || !nativeCrop.height)\n>> nativeCrop = { 0, 0, 1, 1 };\n>> @@ -2069,7 +2070,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n>> Request *request)\n>> {\n>> request->metadata().set(controls::SensorTimestamp,\n>> - bufferControls.get(controls::SensorTimestamp));\n>> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));\n>>\n>> request->metadata().set(controls::ScalerCrop, scalerCrop_);\n>> }\n>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n>> index 2fb527d8..030432e3 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>> TIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n>>\n>> if (cameraProperties.contains(properties::Model)) {\n>> - std::string model = cameraProperties.get(properties::Model);\n>> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});\n>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n>> }\n>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>> const double eps = 1e-2;\n>>\n>> if (metadata.contains(controls::ColourGains)) {\n>> - Span const &colourGains = metadata.get(controls::ColourGains);\n>> + Span const &colourGains =\n>> + metadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n>> if (colourGains[0] > eps && colourGains[1] > eps) {\n>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n>> neutral[0] = 1.0 / colourGains[0]; /* red */\n>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>> }\n>> }\n>> if (metadata.contains(controls::ColourCorrectionMatrix)) {\n>> - Span const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n>> + Span const &coeffs =\n>> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\n>> Matrix3d ccmSupplied(coeffs);\n>> if (ccmSupplied.determinant() > eps)\n>> ccm = ccmSupplied;\n>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n>>\n>> if (metadata.contains(controls::SensorBlackLevels)) {\n>> - Span levels = metadata.get(controls::SensorBlackLevels);\n>> + Span levels =\n>> + metadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\n>>\n>> /*\n>> * 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>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n>>\n>> if (metadata.contains(controls::AnalogueGain)) {\n>> - float gain = metadata.get(controls::AnalogueGain);\n>> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});\n>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n>> }\n>>\n>> if (metadata.contains(controls::ExposureTime)) {\n>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n>> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;\n>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n>> }\n>>\n>> --\n>> 2.25.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 99009C0F1B\n\tfor ;\n\tFri, 8 Apr 2022 22:29:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDEE265644;\n\tSat, 9 Apr 2022 00:29:53 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.19])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 400FD6563F\n\tfor ;\n\tSat, 9 Apr 2022 00:29:52 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx005\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id 1My32L-1nuBwO26SF-00zY3E\n\tfor\n\t; Sat, 09 Apr 2022 00:29:51 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649456994;\n\tbh=mhUhDUG44DUCao4bvE+0XlNdYD2sAdybC+XQO+Ud2Z4=;\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=C/jwoJumPQhxBONCsJwBWX+Ubmj9OFzuw1HbqCJM1AEdJZg538ip7U+qgLznVe92c\n\tEWtxfiwKxgsS+dwlpSpzKvt76Sbr2sucd6bfBgz40xay9LwyH+d+cLIdAIAvZR2JGX\n\tkulJanjYwC3pg/81jsBAqxpWjf6Ui3m6pcbXr3s/omAneQ48Xc5Gj4dUNoRVt1FNp8\n\trfdjlLAOYl7vzoNxPczeI9Wt93HeBbI0RDAefk6q7PQyewv+ygEZ/i3rHuNhPlcLTv\n\tLokWRnD5VDgLrtG1vJPMPTmhdFVp57q3zwah/0ZCtgMuOGhfaMkAe0KF8daJFkwZMU\n\t4GKmv3YTy8C4A==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1649456991;\n\tbh=mhUhDUG44DUCao4bvE+0XlNdYD2sAdybC+XQO+Ud2Z4=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=JI8MfGOrGQqJAVNV+9cZgOvmU0w/apw0ZTaE668rqz3+unNzl7NST/NaGWSQx1mYU\n\tw0jZTPtLe+V2a07Fs307okD0bHrQk9Ft5T4A6GeBHWtA3t6ZSToVxfz6CHw8kQ3OkV\n\tQj+uzul8ouhVRFl3+WgKNI19Cb1E/xYTm4kQHpXY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"JI8MfGOr\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>","Date":"Fri, 8 Apr 2022 23:29:51 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>\n\t<164941960610.22830.16120063966032672613@Monstersaurus>","In-Reply-To":"<164941960610.22830.16120063966032672613@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:XQC6WfScylNBQLqpxbMxkzCD65FutnRefVMV3IJ4OGRhx8YHvan\n\tfLMlHbJYoFZIIuaPpgvl88RlAKFusgGSjuwsT4qMPQPaUIvDTXOqfER/IwYe0qXD9KVAlsg\n\tR42vyAXFmSRc8D7wa6YUs2mIqLnCyAdMBGfNYAZdOEX6wkkGWdtDZBv7kq7Z4exB31Qxjax\n\tB2D8dz9RrxPMUrIAU+HHA==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:oXshNafB3MA=:tjFf/4ZMxqp8vVI/56kVJe\n\tkYo8M+E8luMpR3mP5+Ai4rHTMxa1ZyeojX0kLBL6+0PrxkOcw76S5GdtbD6Fp3rJ+uhJeLAMU\n\tBSCb+1/SH3SHALGvJhHoLqyBt6vm5ISENWLcxZx81rUqDEbTD8UwvZU0oWK6FeoR2b6FHOyNI\n\tIRAW/aGnw/3FVuXIym1Q+LofuhIUufXkCrL6MoRdKblmkEwkQmISjs4qoTWR896HFIqGrnwOE\n\th81nF+RDJY7a7YQgd8JxGgIBDz7WxenzGtOJR9vI4G7JLdeUciBKg+cymHNJbRsQkJHrfUEOV\n\tgzfIrwSinAOjuKKYgRD3hcXuX+y0TBH+vW2Wn4QuzO1OVZseXfjApoEqxtCkcfjocuwzNo6Dd\n\tYK/Jp/i4pA6U/98+fg+3IuH8nvtaTZxrSOARVLbpx1QaU1WeIl7KQ1RoBn6/lASFEOabwwStx\n\tp+K0l9Xo7CeyBgAg14HEbTxtP+Tj1qk96Xm/GunMnE/fJZQkBr5ahK44kvgMt0HMRonJsKGKP\n\t67PhI0+6KWKiTKAu2OJUEd0Klb/HPKk3qavg9Ab93e10VOD64pJK3yRuoM15wXoxGFMnvW19C\n\tEeDb9zPOkMOuxDq5GmhX23Bi00r02fXDY7qwBOvSplDuletZ03Z0X7uTNcyUe9DgMwsNClVe6\n\tAhOwuXRKwmrjbOw5mYKevkQeKCniAPavbkEPgf1QB4KsHaJCuxtw3eoI+mBOcbjOGWHR3hKWt\n\tjIsbp/14eSyGm/MJ1th2Av+WczkDE6kbXRIl8Q4r5RU3LwfIQhdZVF2a3UYSNe6If/LA2uAlC\n\t1q843gRkoHtc76RcLG8ZhFCVPTAxO1oNIlj/zezS+1YlAtQhlBi0WEP8zh4Tz2orFbo01PI3C\n\t89LaPv04DuvztCq8pLWtRK1OW5PvYQpl7o0agRQ8/+1n5X9mGhJVenvRFE+VD5m+f7tB/d4yF\n\tRMN9JqsGh61/Ne2LiZFnQeQyTTLbDuF0Fzd3vaSXCt3L1jiU8LHrwC3A/gfHoMhCt+u/iDz75\n\t296aLoCRMCJjpUu9dD2LyiYud1ej52PNlSV1Hxjrek7NHA+l5gjc5j+brs6LsW0C6qbB6WYzs\n\tDmmxNVuKWLCz60=","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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":22730,"web_url":"https://patchwork.libcamera.org/comment/22730/","msgid":"","date":"2022-04-16T19:41:21","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Kieran,\n\nIs my patch series, including the std::optional change, something you\nwould consider? I think it's a useful addition as it properly \"types\"\nthe Span Controls and makes the handling of invalid return \"get\" values\nexplicit.\n\nBest,\nChristian\n\n\nAm 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:\n> Hi Kieran,\n>\n> Am 08.04.22 um 13:06 schrieb Kieran Bingham:\n>> Hi Christian,\n>>\n>> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)\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>> This looks like a really good way to express the controls from a list. I\n>> really like the value_or() that it brings to allow the code to set a\n>> default.\n>>\n>>\n>> I expect this will have knock-on effects to other out of tree\n>> applications using the control framework so we might want to coordinate\n>> the merge of this.\n>>\n>> Though I notice there's fairly minimal changes to cam and qcam. Do you\n>> know if your build includes the v4l2 adaptation layer and gstreamer?\n>> Does this API change cause definate breakage to users?\n>>\n>> (It's ok if it does, that's preciesly why we are not ABI stable).\n>\n> This is definitely a breaking change as it changes the public API. But\n> the changes that have to be made are quite trivial. You only have to add\n> \".value()\" or \".value_or(...)\" to the old code.\n>\n> I don't know about the v4l2 wrapper and gstreamer. There might be some\n> code that is not compiled on my setup. But the \"qcam\" application still\n> works. And I think this one is using the v4l2 wrapper.\n>\n>>\n>>\n>>>\n>>> Signed-off-by: Christian Rauch \n>>> ---\n>>> include/libcamera/controls.h | 6 +++---\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>>> 6 files changed, 24 insertions(+), 21 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>> index 665bcac1..57b777e9 100644\n>>> --- a/include/libcamera/controls.h\n>>> +++ b/include/libcamera/controls.h\n>>> @@ -167,7 +167,7 @@ public:\n>>>\n>>> using V = typename T::value_type;\n>>> const V *value = reinterpret_cast(data().data());\n>>> - return { value, numElements_ };\n>>> + return T{ value, numElements_ };\n>>> }\n>>>\n>>> #ifndef __DOXYGEN__\n>>> @@ -373,11 +373,11 @@ public:\n>>> bool contains(unsigned int id) const;\n>>>\n>>> template\n>>> - T get(const Control &ctrl) const\n>>> + std::optional get(const Control &ctrl) const\n>>> {\n>>> const ControlValue *val = find(ctrl.id());\n>>> if (!val)\n>>> - return T{};\n>>> + return std::nullopt;\n>>>\n>>> return val->get();\n>>> }\n>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n>>> index c7f664b9..853a78ed 100644\n>>> --- a/src/cam/main.cpp\n>>> +++ b/src/cam/main.cpp\n>>> @@ -293,7 +293,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>> * is only used if the location isn't present or is set to External.\n>>> */\n>>> if (props.contains(properties::Location)) {\n>>> - switch (props.get(properties::Location)) {\n>>> + switch (props.get(properties::Location).value_or(int32_t{})) {\n>>\n>> Is there a way to do this without the value_or() in conditions where the\n>> value has already been guaranteed to exist?\n>>\n>> Here we have just checked that the lists contains a\n>> properties::Lcoation, so we 'know' that it will never process the\n>> '_or()' part.\n>>\n>> Looking at https://en.cppreference.com/w/cpp/utility/optional\n>>\n>> I guess we can just use .value() in the locations where we already check\n>> for the presence. I suspect this could lead to a code refactor to just\n>> use the optional to determine the properties existance instead of\n>> .contains() - but that could certainly be done on top.\n>>\n>> Perhaps it might be better for consistency to use the value_or() variant\n>> on occasions though - even if we know it must already exist?\n>\n> \".value()\" will throw an exception if the \"std::optional\" does not\n> contain a value. If you can guarantee that a ControlValue contains a\n> value, then you can skip the check via \".has_value()\" or the fallback\n> via \".value_or(...)\" and use \".value()\" directly.\n>\n>>\n>>\n>>> case properties::CameraLocationFront:\n>>> addModel = false;\n>>> name = \"Internal front camera \";\n>>> @@ -313,7 +313,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>> * If the camera location is not availble use the camera model\n>>> * to build the camera name.\n>>> */\n>>> - name = \"'\" + props.get(properties::Model) + \"' \";\n>>> + name = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n>>> }\n>>>\n>>> name += \"(\" + camera->id() + \")\";\n>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> index 5a5cdf66..93b32e94 100644\n>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>>>\n>>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>>> {\n>>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n>>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n>>> RPiController::Metadata lastMetadata;\n>>> Span embeddedBuffer;\n>>>\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 60e01917..394221cb 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -1146,7 +1146,7 @@ int PipelineHandlerIPU3::registerCameras()\n>>> /* Convert the sensor rotation to a transformation */\n>>> int32_t rotation = 0;\n>>> if (data->properties_.contains(properties::Rotation))\n>>> - rotation = data->properties_.get(properties::Rotation);\n>>> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});\n>>> else\n>>> LOG(IPU3, Warning) << \"Rotation control not exposed by \"\n>>> << cio2->sensor()->id()\n>>> @@ -1341,7 +1341,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>> request->metadata().set(controls::draft::PipelineDepth, 3);\n>>> /* \\todo Actually apply the scaler crop region to the ImgU. */\n>>> if (request->controls().contains(controls::ScalerCrop))\n>>> - cropRegion_ = request->controls().get(controls::ScalerCrop);\n>>> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n>>> request->metadata().set(controls::ScalerCrop, cropRegion_);\n>>>\n>>> if (frameInfos_.tryComplete(info))\n>>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>> ev.op = ipa::ipu3::EventStatReady;\n>>> ev.frame = info->id;\n>>> ev.bufferId = info->statBuffer->cookie();\n>>> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n>>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n>>> ev.sensorControls = info->effectiveSensorControls;\n>>> ipa_->processEvent(ev);\n>>> }\n>>> @@ -1477,8 +1477,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n>>> if (!request->controls().contains(controls::draft::TestPatternMode))\n>>> return;\n>>>\n>>> - const int32_t testPatternMode = request->controls().get(\n>>> - controls::draft::TestPatternMode);\n>>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});\n>>\n>> This looks like a section of code that could now use optional for\n>> cleaner code I think. I see above we return early if the control is not\n>> present, and only call setTestPatternMode if it is set.\n>>\n>>\n>> Again, I think this patch is just bringing in the std::optional - so it\n>> shouldn't have to 'make everything use the best implementation' - but I\n>> can see benefits it can bring.\n>>\n>> I think even though we know it's guaranteed to exist here, the use of\n>> value_or() is fine with me, as it highlights that this code could\n>> perhaps be simplified later.\n>>\n>\n> The current \".value_or(...)\" implementation is the closest to the old\n> behaviour, which would return a default contructed object in case of\n> failure. You certainly can change that behaviour if you arec ertain that\n> a value exists.\n>\n>>\n>>>\n>>> int ret = cio2_.sensor()->setTestPatternMode(\n>>> static_cast(testPatternMode));\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> index 0fa294d4..63d57033 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>>> * error means the platform can never run. Let's just print a warning\n>>> * and continue regardless; the rotation is effectively set to zero.\n>>> */\n>>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n>>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});\n>>> bool success;\n>>> Transform rotationTransform = transformFromRotation(rotation, &success);\n>>> if (!success)\n>>> @@ -1696,7 +1696,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n>>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n>>> */\n>>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n>>> - libcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n>>> + libcamera::Span colourGains =\n>>> + controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n>>> /* The control wants linear gains in the order B, Gb, Gr, R. */\n>>> ControlList ctrls(sensor_->controls());\n>>> std::array gains{\n>>> @@ -2031,7 +2032,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n>>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n>>> {\n>>> if (controls.contains(controls::ScalerCrop)) {\n>>> - Rectangle nativeCrop = controls.get(controls::ScalerCrop);\n>>> + Rectangle nativeCrop = controls.get(controls::ScalerCrop).value_or(Rectangle{});\n>>\n>> I'm starting to wonder if a templated get_or would be useful as the type\n>> would be defined there (doesn't have to be here, just an idea)\n>>\n>> It would reduce line length on null initialisers:\n>>\n>> controls.get_or(controls::ScalerCrop, {});\n>>\n>> And easily allow default parameters to be defined:\n>>\n>> controls.get_or(controls::ScalerCrop, {640,480});\n>>\n>> I suspect seeing how this all gets used will determine if it has value\n>> though.\n>\n> This is probably dependent on the situation, but I don't think that\n> initialising control values with the default is a good idea in every\n> case. The biggest advantage of \"std::optional\" is that you can properly\n> test for errors. In most cases, it is probably better to notify the user\n> about missing controls etc. instead of silently replacing the requested\n> values with the defaults.\n>\n>>\n>>>\n>>> if (!nativeCrop.width || !nativeCrop.height)\n>>> nativeCrop = { 0, 0, 1, 1 };\n>>> @@ -2069,7 +2070,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n>>> Request *request)\n>>> {\n>>> request->metadata().set(controls::SensorTimestamp,\n>>> - bufferControls.get(controls::SensorTimestamp));\n>>> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));\n>>>\n>>> request->metadata().set(controls::ScalerCrop, scalerCrop_);\n>>> }\n>>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n>>> index 2fb527d8..030432e3 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>>> TIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n>>>\n>>> if (cameraProperties.contains(properties::Model)) {\n>>> - std::string model = cameraProperties.get(properties::Model);\n>>> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});\n>>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n>>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n>>> }\n>>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> const double eps = 1e-2;\n>>>\n>>> if (metadata.contains(controls::ColourGains)) {\n>>> - Span const &colourGains = metadata.get(controls::ColourGains);\n>>> + Span const &colourGains =\n>>> + metadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n>>> if (colourGains[0] > eps && colourGains[1] > eps) {\n>>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n>>> neutral[0] = 1.0 / colourGains[0]; /* red */\n>>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> }\n>>> }\n>>> if (metadata.contains(controls::ColourCorrectionMatrix)) {\n>>> - Span const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n>>> + Span const &coeffs =\n>>> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\n>>> Matrix3d ccmSupplied(coeffs);\n>>> if (ccmSupplied.determinant() > eps)\n>>> ccm = ccmSupplied;\n>>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n>>>\n>>> if (metadata.contains(controls::SensorBlackLevels)) {\n>>> - Span levels = metadata.get(controls::SensorBlackLevels);\n>>> + Span levels =\n>>> + metadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\n>>>\n>>> /*\n>>> * 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>>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n>>>\n>>> if (metadata.contains(controls::AnalogueGain)) {\n>>> - float gain = metadata.get(controls::AnalogueGain);\n>>> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});\n>>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n>>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n>>> }\n>>>\n>>> if (metadata.contains(controls::ExposureTime)) {\n>>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n>>> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;\n>>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n>>> }\n>>>\n>>> --\n>>> 2.25.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 C53D5C3256\n\tfor ;\n\tSat, 16 Apr 2022 19:41:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1268665642;\n\tSat, 16 Apr 2022 21:41:23 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.19])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99C6F604B1\n\tfor ;\n\tSat, 16 Apr 2022 21:41:21 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx005\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id 1MZTqW-1nTFUF0JLK-00WZvy\n\tfor\n\t; Sat, 16 Apr 2022 21:41:21 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650138083;\n\tbh=Im1quiNBmiqZ2YfMdhI4PMyc+ttRt3fxSqnScGkwjF0=;\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=13t/xoAmYrrrI9oTRcYLGJNBA2sj9Lzv4bqcx9m6LsSItHZz/Q4LRt/07P34aJNrm\n\tN79DQ+TYLe7wcsX1MvrJ6kjkfQqesL3+TgmqkAsGy1RY9QpMmGEGyMO42StWT7YKol\n\tFE394H1lXMtD6UTWuVD6PK7FXbELBGpq5JcSL1lz9oOILKuDbf+8Al9WkInG1EnYvA\n\tI+Gg3EuzkyXgYAtsNB/deQ3GDRo7AH9h++39DveyyGbI+OtKrxFtle1kClkHE+Vql8\n\t7AP4rAFTyWdpwaXqaobvcIOHJVP9+It1vYRRZk5VAEqt66E5Ib1IOcQ63SmKg4vnsp\n\tvbf34NgNlns8Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1650138081;\n\tbh=Im1quiNBmiqZ2YfMdhI4PMyc+ttRt3fxSqnScGkwjF0=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=c2DLrVny7MR0gKWRVc4iHSMXgIHPMOlmLeFfmFtaYsIBgjz2r1QhPkfSHFHwT9O3U\n\tUqT5iGf/cSQQ7u2W0uXEYQ5SkFS5kPJNbavCIPd0TAbfdZ+y8GqeLA0c7/0ReP//xF\n\tkUjDvvEpZC1db6MLeObG3XeRLE2Pq9qrim21KrXI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"c2DLrVny\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Sat, 16 Apr 2022 20:41:21 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>\n\t<164941960610.22830.16120063966032672613@Monstersaurus>\n\t<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>","In-Reply-To":"<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:0a/Tk53ECtGb06xkpOn63ty0nqGnvAYmLj7oNTum2Pl3NBMbamQ\n\tOrvgYRj/qDzvTL5FicpbUBE384PAFADN2CRqd4YWOPu4FkGJjLq85T3oDYdMW9j1DO/4IiC\n\tnv3hC2mq6j1LSzMNj/FjpVQDTsLYPDsVH++hUM7GHRDfQ/pQ38v8YKz8YsD8znzVvWmdndy\n\t45twy8N6UJSvsxX6IfJ7g==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:LH5q0CkINMc=:Sjl9Zc4pXNykFln5CzoslE\n\tbY6F/4VMhLKvwKSj4++dvgISu4ccUNhV5MzAF9mz6V8+gAay32FLCGZZkBtptJHD0s/vVVMse\n\tsRqOrMg/aHkHcn7DZIY4Funu4u9aDjN8IRh0xr4qcpHgwRIg2TPcCsHb6s40rFTKTZbo+g51W\n\tDCWdEMdsXWnZc0gleDfz2fWn7Y3C1EoWSkOXRddDAHFUFWRZ9m6OZhKke7J24vkP7TVXAgdWg\n\tQ65ioNH+0TITin0Wv5HdAQN2b1ocKx4H1K2aXAxEDIiwPPyYtGtbZGJJkDUQ+lw11xTMS9WaY\n\twUdePXujwUM1DsLF4gSXlkVQnf3QhTA+Hk1enPfhTV/AdbvI3JYgOIk2dtkELQtisYAM2YIis\n\tdYj5Cm4GIYisIH5UKfGLkH92iBAyJj4w+FRl1NGwJeSlQ4qJ151m4VHO2ajmDiAHDvMFvdk/L\n\t5/eMFisTUPqCrcdRKsMDgdUpfOm228jH5TVy+GzWQLCXIvzyrWSaJLCJD9byjXnPmQCHaDaZ5\n\tCmyKAk8auFAboLRk3ydB1bYc8LEp4WR2JD0Uccu9Q5ecldjILKLoQw+sIS0+MBDhZdF/1Auxg\n\trrwFbYG7zI8UlkrRk40qJLMW0px0MZ6DGeNGK4WJEPuQxe+q4zNqWu7Dnn6ZBqdrFMygtyGKE\n\tNlPjzHUnicjIQvMhWfNrLMLqcqCsNNjz2jyXIWQGvCMV1BA37SXv1X1JXGW6N8cHMp2HX9inb\n\t+HsZoiqf+PpVj0dNI8M6gb3rbrGR1tqG2J3Bwa+fcajY9ELIqcyd4ybTFmH4GdoXoWRqLfX1/\n\tv3OtHwXKq6WecImdBIGIeQsaqlo9V4cQOAXW+TeSIFp49kDLSiv0ffIkkasDmTJIdE1RnQqbX\n\tHJK2KAhcQw8nf9bHfrLSNaiY6qFlgS1M0pSZpTF45Bz/bx2BIf+WB2peSsXQEutPN6wOOG2rq\n\t3GbayZF8MZnK1CKaIVfOphEIQq70ia3EXcqJbdRNgLNSS6ihFYmBXDGIswIRc11rTSxBHMBpO\n\tgeDmGo30pce6hlLdAyQUmtkCKG/6KWhYKqQOkJfPf66jPzIPr6QNjCBWXaNWlQy3S71O+fZiH\n\tGeZCwgvOxOx/aY=","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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":22733,"web_url":"https://patchwork.libcamera.org/comment/22733/","msgid":"<165036533305.2548121.7066095784140188670@Monstersaurus>","date":"2022-04-19T10:48:53","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)\n> Hi Kieran,\n> \n> Is my patch series, including the std::optional change, something you\n> would consider? I think it's a useful addition as it properly \"types\"\n> the Span Controls and makes the handling of invalid return \"get\" values\n> explicit.\n> \n\nI think overall it sounded good - but I think Laurent mentioned he has\nsome concerens about std::optional in public API, as we may have some\nlimitations there, that are preventing us having a full C++17 usage.\n\nI can't recall what is holding us to C++14 on public API - but I would\nhope we can look at what is required to bring that up to '17.\n\n> Best,\n> Christian\n> \n> \n> Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:\n> > Hi Kieran,\n> >\n> > Am 08.04.22 um 13:06 schrieb Kieran Bingham:\n> >> Hi Christian,\n> >>\n> >> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)\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> >> This looks like a really good way to express the controls from a list. I\n> >> really like the value_or() that it brings to allow the code to set a\n> >> default.\n> >>\n> >>\n> >> I expect this will have knock-on effects to other out of tree\n> >> applications using the control framework so we might want to coordinate\n> >> the merge of this.\n> >>\n> >> Though I notice there's fairly minimal changes to cam and qcam. Do you\n> >> know if your build includes the v4l2 adaptation layer and gstreamer?\n> >> Does this API change cause definate breakage to users?\n> >>\n> >> (It's ok if it does, that's preciesly why we are not ABI stable).\n> >\n> > This is definitely a breaking change as it changes the public API. But\n> > the changes that have to be made are quite trivial. You only have to add\n> > \".value()\" or \".value_or(...)\" to the old code.\n> >\n> > I don't know about the v4l2 wrapper and gstreamer. There might be some\n> > code that is not compiled on my setup. But the \"qcam\" application still\n> > works. And I think this one is using the v4l2 wrapper.\n> >\n> >>\n> >>\n> >>>\n> >>> Signed-off-by: Christian Rauch \n> >>> ---\n> >>> include/libcamera/controls.h | 6 +++---\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> >>> 6 files changed, 24 insertions(+), 21 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>> index 665bcac1..57b777e9 100644\n> >>> --- a/include/libcamera/controls.h\n> >>> +++ b/include/libcamera/controls.h\n> >>> @@ -167,7 +167,7 @@ public:\n> >>>\n> >>> using V = typename T::value_type;\n> >>> const V *value = reinterpret_cast(data().data());\n> >>> - return { value, numElements_ };\n> >>> + return T{ value, numElements_ };\n> >>> }\n> >>>\n> >>> #ifndef __DOXYGEN__\n> >>> @@ -373,11 +373,11 @@ public:\n> >>> bool contains(unsigned int id) const;\n> >>>\n> >>> template\n> >>> - T get(const Control &ctrl) const\n> >>> + std::optional get(const Control &ctrl) const\n> >>> {\n> >>> const ControlValue *val = find(ctrl.id());\n> >>> if (!val)\n> >>> - return T{};\n> >>> + return std::nullopt;\n> >>>\n> >>> return val->get();\n> >>> }\n> >>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> >>> index c7f664b9..853a78ed 100644\n> >>> --- a/src/cam/main.cpp\n> >>> +++ b/src/cam/main.cpp\n> >>> @@ -293,7 +293,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >>> * is only used if the location isn't present or is set to External.\n> >>> */\n> >>> if (props.contains(properties::Location)) {\n> >>> - switch (props.get(properties::Location)) {\n> >>> + switch (props.get(properties::Location).value_or(int32_t{})) {\n> >>\n> >> Is there a way to do this without the value_or() in conditions where the\n> >> value has already been guaranteed to exist?\n> >>\n> >> Here we have just checked that the lists contains a\n> >> properties::Lcoation, so we 'know' that it will never process the\n> >> '_or()' part.\n> >>\n> >> Looking at https://en.cppreference.com/w/cpp/utility/optional\n> >>\n> >> I guess we can just use .value() in the locations where we already check\n> >> for the presence. I suspect this could lead to a code refactor to just\n> >> use the optional to determine the properties existance instead of\n> >> .contains() - but that could certainly be done on top.\n> >>\n> >> Perhaps it might be better for consistency to use the value_or() variant\n> >> on occasions though - even if we know it must already exist?\n> >\n> > \".value()\" will throw an exception if the \"std::optional\" does not\n> > contain a value. If you can guarantee that a ControlValue contains a\n> > value, then you can skip the check via \".has_value()\" or the fallback\n> > via \".value_or(...)\" and use \".value()\" directly.\n> >\n> >>\n> >>\n> >>> case properties::CameraLocationFront:\n> >>> addModel = false;\n> >>> name = \"Internal front camera \";\n> >>> @@ -313,7 +313,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >>> * If the camera location is not availble use the camera model\n> >>> * to build the camera name.\n> >>> */\n> >>> - name = \"'\" + props.get(properties::Model) + \"' \";\n> >>> + name = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n> >>> }\n> >>>\n> >>> name += \"(\" + camera->id() + \")\";\n> >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>> index 5a5cdf66..93b32e94 100644\n> >>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> >>>\n> >>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> >>> {\n> >>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> >>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n> >>> RPiController::Metadata lastMetadata;\n> >>> Span embeddedBuffer;\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index 60e01917..394221cb 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -1146,7 +1146,7 @@ int PipelineHandlerIPU3::registerCameras()\n> >>> /* Convert the sensor rotation to a transformation */\n> >>> int32_t rotation = 0;\n> >>> if (data->properties_.contains(properties::Rotation))\n> >>> - rotation = data->properties_.get(properties::Rotation);\n> >>> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});\n> >>> else\n> >>> LOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> >>> << cio2->sensor()->id()\n> >>> @@ -1341,7 +1341,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> >>> request->metadata().set(controls::draft::PipelineDepth, 3);\n> >>> /* \\todo Actually apply the scaler crop region to the ImgU. */\n> >>> if (request->controls().contains(controls::ScalerCrop))\n> >>> - cropRegion_ = request->controls().get(controls::ScalerCrop);\n> >>> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n> >>> request->metadata().set(controls::ScalerCrop, cropRegion_);\n> >>>\n> >>> if (frameInfos_.tryComplete(info))\n> >>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>> ev.op = ipa::ipu3::EventStatReady;\n> >>> ev.frame = info->id;\n> >>> ev.bufferId = info->statBuffer->cookie();\n> >>> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> >>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n> >>> ev.sensorControls = info->effectiveSensorControls;\n> >>> ipa_->processEvent(ev);\n> >>> }\n> >>> @@ -1477,8 +1477,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> >>> if (!request->controls().contains(controls::draft::TestPatternMode))\n> >>> return;\n> >>>\n> >>> - const int32_t testPatternMode = request->controls().get(\n> >>> - controls::draft::TestPatternMode);\n> >>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});\n> >>\n> >> This looks like a section of code that could now use optional for\n> >> cleaner code I think. I see above we return early if the control is not\n> >> present, and only call setTestPatternMode if it is set.\n> >>\n> >>\n> >> Again, I think this patch is just bringing in the std::optional - so it\n> >> shouldn't have to 'make everything use the best implementation' - but I\n> >> can see benefits it can bring.\n> >>\n> >> I think even though we know it's guaranteed to exist here, the use of\n> >> value_or() is fine with me, as it highlights that this code could\n> >> perhaps be simplified later.\n> >>\n> >\n> > The current \".value_or(...)\" implementation is the closest to the old\n> > behaviour, which would return a default contructed object in case of\n> > failure. You certainly can change that behaviour if you arec ertain that\n> > a value exists.\n> >\n> >>\n> >>>\n> >>> int ret = cio2_.sensor()->setTestPatternMode(\n> >>> static_cast(testPatternMode));\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> index 0fa294d4..63d57033 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> >>> * error means the platform can never run. Let's just print a warning\n> >>> * and continue regardless; the rotation is effectively set to zero.\n> >>> */\n> >>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> >>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});\n> >>> bool success;\n> >>> Transform rotationTransform = transformFromRotation(rotation, &success);\n> >>> if (!success)\n> >>> @@ -1696,7 +1696,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> >>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> >>> */\n> >>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n> >>> - libcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n> >>> + libcamera::Span colourGains =\n> >>> + controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> >>> /* The control wants linear gains in the order B, Gb, Gr, R. */\n> >>> ControlList ctrls(sensor_->controls());\n> >>> std::array gains{\n> >>> @@ -2031,7 +2032,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> >>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> >>> {\n> >>> if (controls.contains(controls::ScalerCrop)) {\n> >>> - Rectangle nativeCrop = controls.get(controls::ScalerCrop);\n> >>> + Rectangle nativeCrop = controls.get(controls::ScalerCrop).value_or(Rectangle{});\n> >>\n> >> I'm starting to wonder if a templated get_or would be useful as the type\n> >> would be defined there (doesn't have to be here, just an idea)\n> >>\n> >> It would reduce line length on null initialisers:\n> >>\n> >> controls.get_or(controls::ScalerCrop, {});\n> >>\n> >> And easily allow default parameters to be defined:\n> >>\n> >> controls.get_or(controls::ScalerCrop, {640,480});\n> >>\n> >> I suspect seeing how this all gets used will determine if it has value\n> >> though.\n> >\n> > This is probably dependent on the situation, but I don't think that\n> > initialising control values with the default is a good idea in every\n> > case. The biggest advantage of \"std::optional\" is that you can properly\n> > test for errors. In most cases, it is probably better to notify the user\n> > about missing controls etc. instead of silently replacing the requested\n> > values with the defaults.\n> >\n> >>\n> >>>\n> >>> if (!nativeCrop.width || !nativeCrop.height)\n> >>> nativeCrop = { 0, 0, 1, 1 };\n> >>> @@ -2069,7 +2070,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> >>> Request *request)\n> >>> {\n> >>> request->metadata().set(controls::SensorTimestamp,\n> >>> - bufferControls.get(controls::SensorTimestamp));\n> >>> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));\n> >>>\n> >>> request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> >>> }\n> >>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> >>> index 2fb527d8..030432e3 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> >>> TIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> >>>\n> >>> if (cameraProperties.contains(properties::Model)) {\n> >>> - std::string model = cameraProperties.get(properties::Model);\n> >>> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});\n> >>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n> >>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> >>> }\n> >>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> const double eps = 1e-2;\n> >>>\n> >>> if (metadata.contains(controls::ColourGains)) {\n> >>> - Span const &colourGains = metadata.get(controls::ColourGains);\n> >>> + Span const &colourGains =\n> >>> + metadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> >>> if (colourGains[0] > eps && colourGains[1] > eps) {\n> >>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n> >>> neutral[0] = 1.0 / colourGains[0]; /* red */\n> >>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> }\n> >>> }\n> >>> if (metadata.contains(controls::ColourCorrectionMatrix)) {\n> >>> - Span const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n> >>> + Span const &coeffs =\n> >>> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\n> >>> Matrix3d ccmSupplied(coeffs);\n> >>> if (ccmSupplied.determinant() > eps)\n> >>> ccm = ccmSupplied;\n> >>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n> >>>\n> >>> if (metadata.contains(controls::SensorBlackLevels)) {\n> >>> - Span levels = metadata.get(controls::SensorBlackLevels);\n> >>> + Span levels =\n> >>> + metadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\n> >>>\n> >>> /*\n> >>> * 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> >>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n> >>>\n> >>> if (metadata.contains(controls::AnalogueGain)) {\n> >>> - float gain = metadata.get(controls::AnalogueGain);\n> >>> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});\n> >>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n> >>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n> >>> }\n> >>>\n> >>> if (metadata.contains(controls::ExposureTime)) {\n> >>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n> >>> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;\n> >>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> >>> }\n> >>>\n> >>> --\n> >>> 2.25.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 156F3C0F1B\n\tfor ;\n\tTue, 19 Apr 2022 10:48:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7402B65644;\n\tTue, 19 Apr 2022 12:48:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EAC946563C\n\tfor ;\n\tTue, 19 Apr 2022 12:48:55 +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 7C6C25D;\n\tTue, 19 Apr 2022 12:48:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650365337;\n\tbh=MJHcJn2m9xInczLBVVXD/iBk03cI8ggjLQqdWZfBu0g=;\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:\n\tFrom;\n\tb=xKz3oACOdMQtayQz0OoBCx9tC5HtG7VgYAvh3Go/LR0vmSFDKua+sCJMkGv2pFEwG\n\tQPj3KbZwUOF44zTmlh0tbLfL/FwSlRKPxZCNiYEv9poqgCQ5XWjYfTt4j13l3CsxL/\n\tTfHAP/FONJDi/pj+6LuCCUwgtt6h1frEPz4A5ansO3N5XWSVO76Y+qnF34mYdSGZxI\n\tcfMF033UYwdWOPfmDAbwiJ4R8EYxv0cH+tN/HrpEVxRouZHnKRgRcn4+CjZIDQKCea\n\tJxsMvoX2Dgq9zUhtQd1AQWPZBDh88FMqk+tTdpoyiD7tGb9Ciq/dnDGPzCTTjhNoeq\n\ttPUqmfU2wop7w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650365335;\n\tbh=MJHcJn2m9xInczLBVVXD/iBk03cI8ggjLQqdWZfBu0g=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=SK8SYEWj1RghIN/cPifOLbaLdCm+sZBV/ZIJuULmbhT5xlyQ1QfcXGjxMDxbXgC9Z\n\tv5VF+NKqZ9Lxj+ba4Rm89KmZF3hue8Rca64ryI8+uVSzYEDJoVW1Dmit10ZxFimaH2\n\t/soKpZaJq6zjcxUs+Ngk9wNqXosTxLAIhJ+KCOaI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SK8SYEWj\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>\n\t<164941960610.22830.16120063966032672613@Monstersaurus>\n\t<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>\n\t","To":"Christian Rauch ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 19 Apr 2022 11:48:53 +0100","Message-ID":"<165036533305.2548121.7066095784140188670@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22748,"web_url":"https://patchwork.libcamera.org/comment/22748/","msgid":"","date":"2022-04-19T20:46:08","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Christian and Kieran,\n\nOn Tue, Apr 19, 2022 at 11:48:53AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)\n> > Hi Kieran,\n> > \n> > Is my patch series, including the std::optional change, something you\n> > would consider? I think it's a useful addition as it properly \"types\"\n> > the Span Controls and makes the handling of invalid return \"get\" values\n> > explicit.\n> \n> I think overall it sounded good - but I think Laurent mentioned he has\n> some concerens about std::optional in public API, as we may have some\n> limitations there, that are preventing us having a full C++17 usage.\n> \n> I can't recall what is holding us to C++14 on public API - but I would\n> hope we can look at what is required to bring that up to '17.\n\nI like where this is headed, but my concern is indeed the dependency on\nC++17. We've refrained from requiring C++17 due to Chromium being\ncompiled with C++14 (we've actually briefly switched to C++17 and then\nreverted to C++14 when we noticed the compilation failures). Chromium\nseems to now support C++17, but can we assume everything else (or at\nleast everything else that matters) does ? How about OpenCV for instance\n?\n\nI also have a feeling that we could combine the existing ControlValue\nclass with std::optional to achieve a similar result, but I haven't\nlooked into it in details.\n\n> > Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:\n> > > Am 08.04.22 um 13:06 schrieb Kieran Bingham:\n> > >> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)\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> > >> This looks like a really good way to express the controls from a list. I\n> > >> really like the value_or() that it brings to allow the code to set a\n> > >> default.\n> > >>\n> > >>\n> > >> I expect this will have knock-on effects to other out of tree\n> > >> applications using the control framework so we might want to coordinate\n> > >> the merge of this.\n> > >>\n> > >> Though I notice there's fairly minimal changes to cam and qcam. Do you\n> > >> know if your build includes the v4l2 adaptation layer and gstreamer?\n> > >> Does this API change cause definate breakage to users?\n> > >>\n> > >> (It's ok if it does, that's preciesly why we are not ABI stable).\n> > >\n> > > This is definitely a breaking change as it changes the public API. But\n> > > the changes that have to be made are quite trivial. You only have to add\n> > > \".value()\" or \".value_or(...)\" to the old code.\n> > >\n> > > I don't know about the v4l2 wrapper and gstreamer. There might be some\n> > > code that is not compiled on my setup. But the \"qcam\" application still\n> > > works. And I think this one is using the v4l2 wrapper.\n> > >\n> > >>> Signed-off-by: Christian Rauch \n> > >>> ---\n> > >>> include/libcamera/controls.h | 6 +++---\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> > >>> 6 files changed, 24 insertions(+), 21 deletions(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > >>> index 665bcac1..57b777e9 100644\n> > >>> --- a/include/libcamera/controls.h\n> > >>> +++ b/include/libcamera/controls.h\n> > >>> @@ -167,7 +167,7 @@ public:\n> > >>>\n> > >>> using V = typename T::value_type;\n> > >>> const V *value = reinterpret_cast(data().data());\n> > >>> - return { value, numElements_ };\n> > >>> + return T{ value, numElements_ };\n> > >>> }\n> > >>>\n> > >>> #ifndef __DOXYGEN__\n> > >>> @@ -373,11 +373,11 @@ public:\n> > >>> bool contains(unsigned int id) const;\n> > >>>\n> > >>> template\n> > >>> - T get(const Control &ctrl) const\n> > >>> + std::optional get(const Control &ctrl) const\n> > >>> {\n> > >>> const ControlValue *val = find(ctrl.id());\n> > >>> if (!val)\n> > >>> - return T{};\n> > >>> + return std::nullopt;\n> > >>>\n> > >>> return val->get();\n> > >>> }\n> > >>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > >>> index c7f664b9..853a78ed 100644\n> > >>> --- a/src/cam/main.cpp\n> > >>> +++ b/src/cam/main.cpp\n> > >>> @@ -293,7 +293,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> > >>> * is only used if the location isn't present or is set to External.\n> > >>> */\n> > >>> if (props.contains(properties::Location)) {\n> > >>> - switch (props.get(properties::Location)) {\n> > >>> + switch (props.get(properties::Location).value_or(int32_t{})) {\n> > >>\n> > >> Is there a way to do this without the value_or() in conditions where the\n> > >> value has already been guaranteed to exist?\n> > >>\n> > >> Here we have just checked that the lists contains a\n> > >> properties::Lcoation, so we 'know' that it will never process the\n> > >> '_or()' part.\n> > >>\n> > >> Looking at https://en.cppreference.com/w/cpp/utility/optional\n> > >>\n> > >> I guess we can just use .value() in the locations where we already check\n> > >> for the presence. I suspect this could lead to a code refactor to just\n> > >> use the optional to determine the properties existance instead of\n> > >> .contains() - but that could certainly be done on top.\n> > >>\n> > >> Perhaps it might be better for consistency to use the value_or() variant\n> > >> on occasions though - even if we know it must already exist?\n> > >\n> > > \".value()\" will throw an exception if the \"std::optional\" does not\n> > > contain a value. If you can guarantee that a ControlValue contains a\n> > > value, then you can skip the check via \".has_value()\" or the fallback\n> > > via \".value_or(...)\" and use \".value()\" directly.\n> > >\n> > >>> case properties::CameraLocationFront:\n> > >>> addModel = false;\n> > >>> name = \"Internal front camera \";\n> > >>> @@ -313,7 +313,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> > >>> * If the camera location is not availble use the camera model\n> > >>> * to build the camera name.\n> > >>> */\n> > >>> - name = \"'\" + props.get(properties::Model) + \"' \";\n> > >>> + name = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n> > >>> }\n> > >>>\n> > >>> name += \"(\" + camera->id() + \")\";\n> > >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >>> index 5a5cdf66..93b32e94 100644\n> > >>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > >>>\n> > >>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> > >>> {\n> > >>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> > >>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n> > >>> RPiController::Metadata lastMetadata;\n> > >>> Span embeddedBuffer;\n> > >>>\n> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> index 60e01917..394221cb 100644\n> > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> @@ -1146,7 +1146,7 @@ int PipelineHandlerIPU3::registerCameras()\n> > >>> /* Convert the sensor rotation to a transformation */\n> > >>> int32_t rotation = 0;\n> > >>> if (data->properties_.contains(properties::Rotation))\n> > >>> - rotation = data->properties_.get(properties::Rotation);\n> > >>> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});\n> > >>> else\n> > >>> LOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> > >>> << cio2->sensor()->id()\n> > >>> @@ -1341,7 +1341,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > >>> request->metadata().set(controls::draft::PipelineDepth, 3);\n> > >>> /* \\todo Actually apply the scaler crop region to the ImgU. */\n> > >>> if (request->controls().contains(controls::ScalerCrop))\n> > >>> - cropRegion_ = request->controls().get(controls::ScalerCrop);\n> > >>> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n> > >>> request->metadata().set(controls::ScalerCrop, cropRegion_);\n> > >>>\n> > >>> if (frameInfos_.tryComplete(info))\n> > >>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > >>> ev.op = ipa::ipu3::EventStatReady;\n> > >>> ev.frame = info->id;\n> > >>> ev.bufferId = info->statBuffer->cookie();\n> > >>> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> > >>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n> > >>> ev.sensorControls = info->effectiveSensorControls;\n> > >>> ipa_->processEvent(ev);\n> > >>> }\n> > >>> @@ -1477,8 +1477,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> > >>> if (!request->controls().contains(controls::draft::TestPatternMode))\n> > >>> return;\n> > >>>\n> > >>> - const int32_t testPatternMode = request->controls().get(\n> > >>> - controls::draft::TestPatternMode);\n> > >>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});\n> > >>\n> > >> This looks like a section of code that could now use optional for\n> > >> cleaner code I think. I see above we return early if the control is not\n> > >> present, and only call setTestPatternMode if it is set.\n> > >>\n> > >>\n> > >> Again, I think this patch is just bringing in the std::optional - so it\n> > >> shouldn't have to 'make everything use the best implementation' - but I\n> > >> can see benefits it can bring.\n> > >>\n> > >> I think even though we know it's guaranteed to exist here, the use of\n> > >> value_or() is fine with me, as it highlights that this code could\n> > >> perhaps be simplified later.\n> > >\n> > > The current \".value_or(...)\" implementation is the closest to the old\n> > > behaviour, which would return a default contructed object in case of\n> > > failure. You certainly can change that behaviour if you arec ertain that\n> > > a value exists.\n> > >\n> > >>>\n> > >>> int ret = cio2_.sensor()->setTestPatternMode(\n> > >>> static_cast(testPatternMode));\n> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> index 0fa294d4..63d57033 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> > >>> * error means the platform can never run. Let's just print a warning\n> > >>> * and continue regardless; the rotation is effectively set to zero.\n> > >>> */\n> > >>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> > >>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});\n> > >>> bool success;\n> > >>> Transform rotationTransform = transformFromRotation(rotation, &success);\n> > >>> if (!success)\n> > >>> @@ -1696,7 +1696,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> > >>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> > >>> */\n> > >>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n> > >>> - libcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n> > >>> + libcamera::Span colourGains =\n> > >>> + controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> > >>> /* The control wants linear gains in the order B, Gb, Gr, R. */\n> > >>> ControlList ctrls(sensor_->controls());\n> > >>> std::array gains{\n> > >>> @@ -2031,7 +2032,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> > >>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > >>> {\n> > >>> if (controls.contains(controls::ScalerCrop)) {\n> > >>> - Rectangle nativeCrop = controls.get(controls::ScalerCrop);\n> > >>> + Rectangle nativeCrop = controls.get(controls::ScalerCrop).value_or(Rectangle{});\n> > >>\n> > >> I'm starting to wonder if a templated get_or would be useful as the type\n> > >> would be defined there (doesn't have to be here, just an idea)\n> > >>\n> > >> It would reduce line length on null initialisers:\n> > >>\n> > >> controls.get_or(controls::ScalerCrop, {});\n> > >>\n> > >> And easily allow default parameters to be defined:\n> > >>\n> > >> controls.get_or(controls::ScalerCrop, {640,480});\n> > >>\n> > >> I suspect seeing how this all gets used will determine if it has value\n> > >> though.\n> > >\n> > > This is probably dependent on the situation, but I don't think that\n> > > initialising control values with the default is a good idea in every\n> > > case. The biggest advantage of \"std::optional\" is that you can properly\n> > > test for errors. In most cases, it is probably better to notify the user\n> > > about missing controls etc. instead of silently replacing the requested\n> > > values with the defaults.\n> > >\n> > >>>\n> > >>> if (!nativeCrop.width || !nativeCrop.height)\n> > >>> nativeCrop = { 0, 0, 1, 1 };\n> > >>> @@ -2069,7 +2070,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> > >>> Request *request)\n> > >>> {\n> > >>> request->metadata().set(controls::SensorTimestamp,\n> > >>> - bufferControls.get(controls::SensorTimestamp));\n> > >>> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));\n> > >>>\n> > >>> request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > >>> }\n> > >>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> > >>> index 2fb527d8..030432e3 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> > >>> TIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> > >>>\n> > >>> if (cameraProperties.contains(properties::Model)) {\n> > >>> - std::string model = cameraProperties.get(properties::Model);\n> > >>> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});\n> > >>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n> > >>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> > >>> }\n> > >>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > >>> const double eps = 1e-2;\n> > >>>\n> > >>> if (metadata.contains(controls::ColourGains)) {\n> > >>> - Span const &colourGains = metadata.get(controls::ColourGains);\n> > >>> + Span const &colourGains =\n> > >>> + metadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> > >>> if (colourGains[0] > eps && colourGains[1] > eps) {\n> > >>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n> > >>> neutral[0] = 1.0 / colourGains[0]; /* red */\n> > >>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > >>> }\n> > >>> }\n> > >>> if (metadata.contains(controls::ColourCorrectionMatrix)) {\n> > >>> - Span const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n> > >>> + Span const &coeffs =\n> > >>> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\n> > >>> Matrix3d ccmSupplied(coeffs);\n> > >>> if (ccmSupplied.determinant() > eps)\n> > >>> ccm = ccmSupplied;\n> > >>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> > >>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n> > >>>\n> > >>> if (metadata.contains(controls::SensorBlackLevels)) {\n> > >>> - Span levels = metadata.get(controls::SensorBlackLevels);\n> > >>> + Span levels =\n> > >>> + metadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\n> > >>>\n> > >>> /*\n> > >>> * 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> > >>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n> > >>>\n> > >>> if (metadata.contains(controls::AnalogueGain)) {\n> > >>> - float gain = metadata.get(controls::AnalogueGain);\n> > >>> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});\n> > >>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n> > >>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n> > >>> }\n> > >>>\n> > >>> if (metadata.contains(controls::ExposureTime)) {\n> > >>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n> > >>> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;\n> > >>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> > >>> }\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 25CACC3256\n\tfor ;\n\tTue, 19 Apr 2022 20:46:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D23F665644;\n\tTue, 19 Apr 2022 22:46:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8ECE6604B2\n\tfor ;\n\tTue, 19 Apr 2022 22:46:11 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-5-145-nat.elisa-mobile.fi\n\t[85.76.5.145])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6569125B;\n\tTue, 19 Apr 2022 22:46:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650401172;\n\tbh=xHRJRAT0CI3zahPsSeQCLixNqSvuLMjw5l2r/7JsouU=;\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=X7LftMvAhV/gP3cP5pHcxNLeCi6NbZXxyWc3rYAndTYAlQEVY8puz4tBGePV0u7uF\n\tTaPVqRgfCUy5qEJp/PGlfHx1RbG5V9zvqUyWfrfUxjwrtjgHipq3jjfUMKRetY03Mt\n\t17OHSvTk3naYklwchw6wz7FHsuOCGttcY91wkjhKe+litpqUGQ+waB8tLmDSEjCllm\n\tLBKKpHjQY2r44mdk0L3LLL+m9SaQ/AJHg2v7ftVhyG2KsXDqJdc5BJLQys3cpDHktP\n\tzDg9muR1CLVQtsteTy3cXRwwdG8Xkc/Xelk9nNhR73vgzD3jwwJNk8D3nGzDmIUT+p\n\tEqf0GN8fIoU7A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650401171;\n\tbh=xHRJRAT0CI3zahPsSeQCLixNqSvuLMjw5l2r/7JsouU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d+DynZzAMAzkpdGtjMcqDnnxO+ru93BMyMACnr3II0IWy0pr4m8yI3bQYU2f+8Yql\n\tqUy00YredxA2niZGDGwAnPRqrrTqSQ2AqwmeCRiOyPaSUEcCOgXEeAFZrRnN6r+8e2\n\tFyqPxY1O7BQ4kuMzIhb9+UDpLtPH/s1wPJvDW2qo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"d+DynZzA\"; dkim-atps=neutral","Date":"Tue, 19 Apr 2022 23:46:08 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>\n\t<164941960610.22830.16120063966032672613@Monstersaurus>\n\t<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>\n\t\n\t<165036533305.2548121.7066095784140188670@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165036533305.2548121.7066095784140188670@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22750,"web_url":"https://patchwork.libcamera.org/comment/22750/","msgid":"","date":"2022-04-19T20:46:25","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Christian,\n\nThank you for the patch.\n\nOn Fri, Apr 08, 2022 at 02:42:31AM +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 | 6 +++---\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> 6 files changed, 24 insertions(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 665bcac1..57b777e9 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -167,7 +167,7 @@ public:\n> \n> \t\tusing V = typename T::value_type;\n> \t\tconst V *value = reinterpret_cast(data().data());\n> -\t\treturn { value, numElements_ };\n> +\t\treturn T{ value, numElements_ };\n> \t}\n> \n> #ifndef __DOXYGEN__\n> @@ -373,11 +373,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/cam/main.cpp b/src/cam/main.cpp\n> index c7f664b9..853a78ed 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -293,7 +293,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).value_or(int32_t{})) {\n> \t\tcase properties::CameraLocationFront:\n> \t\t\taddModel = false;\n> \t\t\tname = \"Internal front camera \";\n> @@ -313,7 +313,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).value_or(std::string{}) + \"' \";\n> \t}\n> \n> \tname += \"(\" + camera->id() + \")\";\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5a5cdf66..93b32e94 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> \n> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> {\n> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\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 60e01917..394221cb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1146,7 +1146,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).value_or(int32_t{});\n> \t\telse\n> \t\t\tLOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> \t\t\t\t\t << cio2->sensor()->id()\n> @@ -1341,7 +1341,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).value_or(Rectangle{});\n> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n> \n> \tif (frameInfos_.tryComplete(info))\n> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \tev.op = ipa::ipu3::EventStatReady;\n> \tev.frame = info->id;\n> \tev.bufferId = info->statBuffer->cookie();\n> -\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> +\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n> \tev.sensorControls = info->effectiveSensorControls;\n> \tipa_->processEvent(ev);\n> }\n> @@ -1477,8 +1477,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(int32_t{});\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 0fa294d4..63d57033 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(int32_t{});\n> \tbool success;\n> \tTransform rotationTransform = transformFromRotation(rotation, &success);\n> \tif (!success)\n> @@ -1696,7 +1696,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\tcontrols.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n\nThis is too long, we have a strick limit at 120 characters.\n\nDo we need the value_or() here, given that it duplicates the\ncontrols.contains() check ? I would expect either\n\n\tif (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n\t\tlibcamera::Span colourGains =\n\t\t\tcontrols.get(libcamera::controls::ColourGains).value();\n\nor\n\n\tstd::optional<...> colourGainsCtrl =\n\t\tcontrols.get(libcamera::controls::ColourGains);\n\tif (notifyGainsUnity_ && colourGainsCtrl) {\n\t\tlibcamera::Span colourGains = colourGainsCtrl.value();\n\nSame in quite a few locations below.\n\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> @@ -2031,7 +2032,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).value_or(Rectangle{});\n> \n> \t\tif (!nativeCrop.width || !nativeCrop.height)\n> \t\t\tnativeCrop = { 0, 0, 1, 1 };\n> @@ -2069,7 +2070,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(int64_t{}));\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 2fb527d8..030432e3 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).value_or(std::string{});\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\tmetadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\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\tmetadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\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\tmetadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\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).value_or(float{});\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).value_or(float{}) / 1e6;\n> \t\tTIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> \t}\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 59AECC3256\n\tfor ;\n\tTue, 19 Apr 2022 20:46:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0748565646;\n\tTue, 19 Apr 2022 22:46:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A1AA604B2\n\tfor ;\n\tTue, 19 Apr 2022 22:46:24 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-5-145-nat.elisa-mobile.fi\n\t[85.76.5.145])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 804AD25B;\n\tTue, 19 Apr 2022 22:46:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650401185;\n\tbh=+18MTtjSLCnk3i9K7pjtxvtvCcE/6kjGFOU+HVk9npM=;\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=TiGD0N4e6rzBJnkrYi++aDSbtLknDeXglw2KFXCQie9GOvwUqp5wX6yTySUbSyY+J\n\tXzbBhgRAvJSejmBXJgrho1B6tRdk+ed9toi5VDOKEIH5O+mr2eFvjfznGOHh0R+p9h\n\ttn9EVaDgyZm84g/WArEaVLhv5Enk/b9m9qTB3hOSSLULHQ32CCnP3tCAkKrb2kIJWI\n\t2YZYoZyzax6Cdagieh8b9C2BbwS6EsZVcTS8J3tyhuttu7biF5JvbItHvFFjXHg4H1\n\t4YdPoDQd2ZEhYM56jsMwKVVxMv0Y7nMWeAPrMtzAXglo10slTNfyG69gPm1YdSDioj\n\t3udhiHjDdbqYw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650401183;\n\tbh=+18MTtjSLCnk3i9K7pjtxvtvCcE/6kjGFOU+HVk9npM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ELswftLKiTxoEVSwn1GvHuAm0RYrwBteutgrGYjKU8fzR5O0SMh6Ndip/NYbI6HkU\n\tti2VlT+Mx0DZXDcAOdsN6cQjrtuHr6p4FPyuqKFxi4odGciBbV6mr/WiceDDYk3zJQ\n\tfh4efkzQj0t69n0AelSlZ3OkbHXqnrjnbH8vsCWQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ELswftLK\"; dkim-atps=neutral","Date":"Tue, 19 Apr 2022 23:46:25 +0300","To":"Christian Rauch ","Message-ID":"","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220408014231.231083-5-Rauch.Christian@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22752,"web_url":"https://patchwork.libcamera.org/comment/22752/","msgid":"","date":"2022-04-19T22:34:12","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Laurent and Kieran,\n\nHow is libcamera related to Chromium and OpenCV? I don't see any\nreferences to these projects in libcamera code. And do they require to\nuse the same C++ standard?\n\nBest,\nChristian\n\n\nAm 19.04.22 um 21:46 schrieb Laurent Pinchart:\n> Hi Christian and Kieran,\n>\n> On Tue, Apr 19, 2022 at 11:48:53AM +0100, Kieran Bingham via libcamera-devel wrote:\n>> Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)\n>>> Hi Kieran,\n>>>\n>>> Is my patch series, including the std::optional change, something you\n>>> would consider? I think it's a useful addition as it properly \"types\"\n>>> the Span Controls and makes the handling of invalid return \"get\" values\n>>> explicit.\n>>\n>> I think overall it sounded good - but I think Laurent mentioned he has\n>> some concerens about std::optional in public API, as we may have some\n>> limitations there, that are preventing us having a full C++17 usage.\n>>\n>> I can't recall what is holding us to C++14 on public API - but I would\n>> hope we can look at what is required to bring that up to '17.\n>\n> I like where this is headed, but my concern is indeed the dependency on\n> C++17. We've refrained from requiring C++17 due to Chromium being\n> compiled with C++14 (we've actually briefly switched to C++17 and then\n> reverted to C++14 when we noticed the compilation failures). Chromium\n> seems to now support C++17, but can we assume everything else (or at\n> least everything else that matters) does ? How about OpenCV for instance\n> ?\n>\n> I also have a feeling that we could combine the existing ControlValue\n> class with std::optional to achieve a similar result, but I haven't\n> looked into it in details.\n>\n>>> Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:\n>>>> Am 08.04.22 um 13:06 schrieb Kieran Bingham:\n>>>>> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)\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>>>>> This looks like a really good way to express the controls from a list. I\n>>>>> really like the value_or() that it brings to allow the code to set a\n>>>>> default.\n>>>>>\n>>>>>\n>>>>> I expect this will have knock-on effects to other out of tree\n>>>>> applications using the control framework so we might want to coordinate\n>>>>> the merge of this.\n>>>>>\n>>>>> Though I notice there's fairly minimal changes to cam and qcam. Do you\n>>>>> know if your build includes the v4l2 adaptation layer and gstreamer?\n>>>>> Does this API change cause definate breakage to users?\n>>>>>\n>>>>> (It's ok if it does, that's preciesly why we are not ABI stable).\n>>>>\n>>>> This is definitely a breaking change as it changes the public API. But\n>>>> the changes that have to be made are quite trivial. You only have to add\n>>>> \".value()\" or \".value_or(...)\" to the old code.\n>>>>\n>>>> I don't know about the v4l2 wrapper and gstreamer. There might be some\n>>>> code that is not compiled on my setup. But the \"qcam\" application still\n>>>> works. And I think this one is using the v4l2 wrapper.\n>>>>\n>>>>>> Signed-off-by: Christian Rauch \n>>>>>> ---\n>>>>>> include/libcamera/controls.h | 6 +++---\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>>>>>> 6 files changed, 24 insertions(+), 21 deletions(-)\n>>>>>>\n>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>>> index 665bcac1..57b777e9 100644\n>>>>>> --- a/include/libcamera/controls.h\n>>>>>> +++ b/include/libcamera/controls.h\n>>>>>> @@ -167,7 +167,7 @@ public:\n>>>>>>\n>>>>>> using V = typename T::value_type;\n>>>>>> const V *value = reinterpret_cast(data().data());\n>>>>>> - return { value, numElements_ };\n>>>>>> + return T{ value, numElements_ };\n>>>>>> }\n>>>>>>\n>>>>>> #ifndef __DOXYGEN__\n>>>>>> @@ -373,11 +373,11 @@ public:\n>>>>>> bool contains(unsigned int id) const;\n>>>>>>\n>>>>>> template\n>>>>>> - T get(const Control &ctrl) const\n>>>>>> + std::optional get(const Control &ctrl) const\n>>>>>> {\n>>>>>> const ControlValue *val = find(ctrl.id());\n>>>>>> if (!val)\n>>>>>> - return T{};\n>>>>>> + return std::nullopt;\n>>>>>>\n>>>>>> return val->get();\n>>>>>> }\n>>>>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n>>>>>> index c7f664b9..853a78ed 100644\n>>>>>> --- a/src/cam/main.cpp\n>>>>>> +++ b/src/cam/main.cpp\n>>>>>> @@ -293,7 +293,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>>>>> * is only used if the location isn't present or is set to External.\n>>>>>> */\n>>>>>> if (props.contains(properties::Location)) {\n>>>>>> - switch (props.get(properties::Location)) {\n>>>>>> + switch (props.get(properties::Location).value_or(int32_t{})) {\n>>>>>\n>>>>> Is there a way to do this without the value_or() in conditions where the\n>>>>> value has already been guaranteed to exist?\n>>>>>\n>>>>> Here we have just checked that the lists contains a\n>>>>> properties::Lcoation, so we 'know' that it will never process the\n>>>>> '_or()' part.\n>>>>>\n>>>>> Looking at https://en.cppreference.com/w/cpp/utility/optional\n>>>>>\n>>>>> I guess we can just use .value() in the locations where we already check\n>>>>> for the presence. I suspect this could lead to a code refactor to just\n>>>>> use the optional to determine the properties existance instead of\n>>>>> .contains() - but that could certainly be done on top.\n>>>>>\n>>>>> Perhaps it might be better for consistency to use the value_or() variant\n>>>>> on occasions though - even if we know it must already exist?\n>>>>\n>>>> \".value()\" will throw an exception if the \"std::optional\" does not\n>>>> contain a value. If you can guarantee that a ControlValue contains a\n>>>> value, then you can skip the check via \".has_value()\" or the fallback\n>>>> via \".value_or(...)\" and use \".value()\" directly.\n>>>>\n>>>>>> case properties::CameraLocationFront:\n>>>>>> addModel = false;\n>>>>>> name = \"Internal front camera \";\n>>>>>> @@ -313,7 +313,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>>>>> * If the camera location is not availble use the camera model\n>>>>>> * to build the camera name.\n>>>>>> */\n>>>>>> - name = \"'\" + props.get(properties::Model) + \"' \";\n>>>>>> + name = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n>>>>>> }\n>>>>>>\n>>>>>> name += \"(\" + camera->id() + \")\";\n>>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>>>> index 5a5cdf66..93b32e94 100644\n>>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>>>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>>>>>>\n>>>>>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>>>>>> {\n>>>>>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n>>>>>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n>>>>>> RPiController::Metadata lastMetadata;\n>>>>>> Span embeddedBuffer;\n>>>>>>\n>>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>>> index 60e01917..394221cb 100644\n>>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>>> @@ -1146,7 +1146,7 @@ int PipelineHandlerIPU3::registerCameras()\n>>>>>> /* Convert the sensor rotation to a transformation */\n>>>>>> int32_t rotation = 0;\n>>>>>> if (data->properties_.contains(properties::Rotation))\n>>>>>> - rotation = data->properties_.get(properties::Rotation);\n>>>>>> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});\n>>>>>> else\n>>>>>> LOG(IPU3, Warning) << \"Rotation control not exposed by \"\n>>>>>> << cio2->sensor()->id()\n>>>>>> @@ -1341,7 +1341,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>>>>> request->metadata().set(controls::draft::PipelineDepth, 3);\n>>>>>> /* \\todo Actually apply the scaler crop region to the ImgU. */\n>>>>>> if (request->controls().contains(controls::ScalerCrop))\n>>>>>> - cropRegion_ = request->controls().get(controls::ScalerCrop);\n>>>>>> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n>>>>>> request->metadata().set(controls::ScalerCrop, cropRegion_);\n>>>>>>\n>>>>>> if (frameInfos_.tryComplete(info))\n>>>>>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>>>>> ev.op = ipa::ipu3::EventStatReady;\n>>>>>> ev.frame = info->id;\n>>>>>> ev.bufferId = info->statBuffer->cookie();\n>>>>>> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n>>>>>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n>>>>>> ev.sensorControls = info->effectiveSensorControls;\n>>>>>> ipa_->processEvent(ev);\n>>>>>> }\n>>>>>> @@ -1477,8 +1477,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n>>>>>> if (!request->controls().contains(controls::draft::TestPatternMode))\n>>>>>> return;\n>>>>>>\n>>>>>> - const int32_t testPatternMode = request->controls().get(\n>>>>>> - controls::draft::TestPatternMode);\n>>>>>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});\n>>>>>\n>>>>> This looks like a section of code that could now use optional for\n>>>>> cleaner code I think. I see above we return early if the control is not\n>>>>> present, and only call setTestPatternMode if it is set.\n>>>>>\n>>>>>\n>>>>> Again, I think this patch is just bringing in the std::optional - so it\n>>>>> shouldn't have to 'make everything use the best implementation' - but I\n>>>>> can see benefits it can bring.\n>>>>>\n>>>>> I think even though we know it's guaranteed to exist here, the use of\n>>>>> value_or() is fine with me, as it highlights that this code could\n>>>>> perhaps be simplified later.\n>>>>\n>>>> The current \".value_or(...)\" implementation is the closest to the old\n>>>> behaviour, which would return a default contructed object in case of\n>>>> failure. You certainly can change that behaviour if you arec ertain that\n>>>> a value exists.\n>>>>\n>>>>>>\n>>>>>> int ret = cio2_.sensor()->setTestPatternMode(\n>>>>>> static_cast(testPatternMode));\n>>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>>>> index 0fa294d4..63d57033 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>>>>>> * error means the platform can never run. Let's just print a warning\n>>>>>> * and continue regardless; the rotation is effectively set to zero.\n>>>>>> */\n>>>>>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n>>>>>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});\n>>>>>> bool success;\n>>>>>> Transform rotationTransform = transformFromRotation(rotation, &success);\n>>>>>> if (!success)\n>>>>>> @@ -1696,7 +1696,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n>>>>>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n>>>>>> */\n>>>>>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n>>>>>> - libcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n>>>>>> + libcamera::Span colourGains =\n>>>>>> + controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n>>>>>> /* The control wants linear gains in the order B, Gb, Gr, R. */\n>>>>>> ControlList ctrls(sensor_->controls());\n>>>>>> std::array gains{\n>>>>>> @@ -2031,7 +2032,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n>>>>>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n>>>>>> {\n>>>>>> if (controls.contains(controls::ScalerCrop)) {\n>>>>>> - Rectangle nativeCrop = controls.get(controls::ScalerCrop);\n>>>>>> + Rectangle nativeCrop = controls.get(controls::ScalerCrop).value_or(Rectangle{});\n>>>>>\n>>>>> I'm starting to wonder if a templated get_or would be useful as the type\n>>>>> would be defined there (doesn't have to be here, just an idea)\n>>>>>\n>>>>> It would reduce line length on null initialisers:\n>>>>>\n>>>>> controls.get_or(controls::ScalerCrop, {});\n>>>>>\n>>>>> And easily allow default parameters to be defined:\n>>>>>\n>>>>> controls.get_or(controls::ScalerCrop, {640,480});\n>>>>>\n>>>>> I suspect seeing how this all gets used will determine if it has value\n>>>>> though.\n>>>>\n>>>> This is probably dependent on the situation, but I don't think that\n>>>> initialising control values with the default is a good idea in every\n>>>> case. The biggest advantage of \"std::optional\" is that you can properly\n>>>> test for errors. In most cases, it is probably better to notify the user\n>>>> about missing controls etc. instead of silently replacing the requested\n>>>> values with the defaults.\n>>>>\n>>>>>>\n>>>>>> if (!nativeCrop.width || !nativeCrop.height)\n>>>>>> nativeCrop = { 0, 0, 1, 1 };\n>>>>>> @@ -2069,7 +2070,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n>>>>>> Request *request)\n>>>>>> {\n>>>>>> request->metadata().set(controls::SensorTimestamp,\n>>>>>> - bufferControls.get(controls::SensorTimestamp));\n>>>>>> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));\n>>>>>>\n>>>>>> request->metadata().set(controls::ScalerCrop, scalerCrop_);\n>>>>>> }\n>>>>>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n>>>>>> index 2fb527d8..030432e3 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>>>>>> TIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n>>>>>>\n>>>>>> if (cameraProperties.contains(properties::Model)) {\n>>>>>> - std::string model = cameraProperties.get(properties::Model);\n>>>>>> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});\n>>>>>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n>>>>>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n>>>>>> }\n>>>>>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>>>>> const double eps = 1e-2;\n>>>>>>\n>>>>>> if (metadata.contains(controls::ColourGains)) {\n>>>>>> - Span const &colourGains = metadata.get(controls::ColourGains);\n>>>>>> + Span const &colourGains =\n>>>>>> + metadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n>>>>>> if (colourGains[0] > eps && colourGains[1] > eps) {\n>>>>>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n>>>>>> neutral[0] = 1.0 / colourGains[0]; /* red */\n>>>>>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>>>>> }\n>>>>>> }\n>>>>>> if (metadata.contains(controls::ColourCorrectionMatrix)) {\n>>>>>> - Span const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n>>>>>> + Span const &coeffs =\n>>>>>> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\n>>>>>> Matrix3d ccmSupplied(coeffs);\n>>>>>> if (ccmSupplied.determinant() > eps)\n>>>>>> ccm = ccmSupplied;\n>>>>>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>>>>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n>>>>>>\n>>>>>> if (metadata.contains(controls::SensorBlackLevels)) {\n>>>>>> - Span levels = metadata.get(controls::SensorBlackLevels);\n>>>>>> + Span levels =\n>>>>>> + metadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\n>>>>>>\n>>>>>> /*\n>>>>>> * 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>>>>>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n>>>>>>\n>>>>>> if (metadata.contains(controls::AnalogueGain)) {\n>>>>>> - float gain = metadata.get(controls::AnalogueGain);\n>>>>>> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});\n>>>>>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n>>>>>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n>>>>>> }\n>>>>>>\n>>>>>> if (metadata.contains(controls::ExposureTime)) {\n>>>>>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n>>>>>> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;\n>>>>>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n>>>>>> }\n>>>>>>\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 507F1C0F1B\n\tfor ;\n\tTue, 19 Apr 2022 22:34:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D48B165647;\n\tWed, 20 Apr 2022 00:34:15 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.15])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0E446563F\n\tfor ;\n\tWed, 20 Apr 2022 00:34:13 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx005\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id 1MfpOd-1o9NQS10yP-00gEHb\n\tfor\n\t; Wed, 20 Apr 2022 00:34:13 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650407655;\n\tbh=mzWR48xJN5TkiJoYdqEiVhfuz1/SIhJO7J3oWMYDYpo=;\n\th=Date:Cc: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=fi650R0u+l40TA282VmAoqGWv/WyNx4eoiVlAouYRnoQAM42jmbM10myFfDhFgqJX\n\tgyKs880SH7qrNMZtJTYwdCAI+7shRBSD/RKMuwPUoEH4SdRhkQgfUZExT12x+vK9RB\n\tUL4IPxHzx1jzy6zYa62CtNZirma2TB63/AvwJ3e3sSQOLL+Ez2Nc6qtIn6YcY/5BML\n\t5KLJJxvqbEgDXWjTfncpBW/LqVQR8edcOW4gBWNY5zZOKdHZftAm2uJ3PPoWMZACjy\n\ttOxPyM1p2jI6uMuP9SylhlqXR8MUjJu+FHyqH8wOTRyqNjqCySZgaEZnIfz8c7uGX4\n\t3yf07WZwz0sVQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1650407653;\n\tbh=mzWR48xJN5TkiJoYdqEiVhfuz1/SIhJO7J3oWMYDYpo=;\n\th=X-UI-Sender-Class:Date:Subject:Cc:References:From:In-Reply-To;\n\tb=dvxm+Cod93heCMOU8X+r2AM0+HQxOhahlupJ3CEs1P1OVV3VSOWAf+8qL6aAgpk9l\n\tTsnqhwIrFVAri2Kvih6XIr4HvvRw6akgbt7xjSKVNmJyF7bN8WvS8AYdHlIGUoyvMW\n\ttfZVGTeFeuqWQELT5kWfnFO12cYMB1sbxtZetENM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"dvxm+Cod\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Tue, 19 Apr 2022 23:34:12 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-CA","Cc":"libcamera-devel@lists.libcamera.org","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>\n\t<164941960610.22830.16120063966032672613@Monstersaurus>\n\t<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>\n\t\n\t<165036533305.2548121.7066095784140188670@Monstersaurus>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:qb0ld42dTo3BKHKQdt4aMFRJkKrl2iRUbTUFZ5RWV8tKtHr3ZeM\n\t1WqfHCuQH4H9lNcFdhNg7UcF4T7DAdYPpQYTXNyHYgJndRPZiLYtaG0KEE2tns6hxvC1VpY\n\t21FKyZAlF0GSvc9TaztBbugWYMOeqVxPGcVLRJRI1NvbqDog7MQqvEGub+aad3jvJiUssZ2\n\t7RXX/Pnc5AvDMEvYkdrOw==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:rIK2YpHkun8=:b4Yrh2P3VTTqJRsnNLirEr\n\tPDfwbgKJuBUL51jjapOqsM9vXD525oP/+JY+H6KvzjTMNPZRbm8pW6Hu/PQT7WpXCRZaqpN1D\n\t79wF5LIvjp4RrQL2jWep0OVehZsnbA13m2rHctDS4j7W0Lbg5UhfEcLkKIxCEMmZtyg/T/lqX\n\t13oEYW0PPs4/msfZf5yWbceACrQlhyagsBjNyUn8e/X2SXx9Sm5Dubcb0gHE1WeeJyaFJ7vGt\n\t4Sz1pOCSLlWG57b9dgfp8m/JnwOLDk+b9F7eZB5TZNLYDniAbcUhAW3PqabqBENmwkdi43tvo\n\tQOucqdnwwv/69MFFByb4arFd9AYvxVwWc26KzK8K0wlNx6HbmjcWeIYbwa0nhXuGDga3JkQqj\n\tQV62ENs+0PtsFidsSTmuerGh5mlzp02jxAMXLIBDlLOmy/uyRIzbHTvVMMxJGztl7CyBVWkkO\n\toZPahWAiec/4M+Ony6T7W0u4gR2DiepYf7BiThIdbOUWyUrM8M1VWhwr8uTC1Ll8uM4e/QP0I\n\t8fep2YLM/xYLKhKc1hwJYNxhnRjcykNR892WnglOOgV0Z0mhm1Ap2AK0+OJQ1dWdFZHXR5drX\n\t8n8GENmXmSOmckWMKvTjbdhgHZfewiTjaiLRwRVwzYCAOkg/CuUtdWvs57P1IsMsviIaq3Xus\n\tN6IV6RZUKpKmbVjeXoqKDXhx8cOJibU9oP9gMnEZkQEoFy55sVSbJVzZ/oBi60IDQR7A4Gycx\n\tbtUtDafh4oUJvSxNgd9JCOIYryG7WrHRgQqA6HLgM9EGDicg4pZ7/OlhqB+CbeoJsVhNer5d0\n\tD1qMcaOo1ZTSlWZJr7gZwXZiIddqMVSrz2NErooK0zXyL/sjj1fK1eFLZAofUR7I5nr9X4kfI\n\tZXB16kbTmkVHINQdhPg1wECA2Fk9KL6uC8EmDWVBtgvtXy/Ek0rN0L1pvgcv2glfTZlI8YQJw\n\t0t7xeVqZww5y+28dATFJC1LDmuSw7I4NmNAsX94o2hbi9CRJyBHvZAwfnfTwH6NX/6gBzRLmv\n\tuTYqymqCSIB5gkWSAzJCyCd74W1Ui0ImuzMFjnMaLuPJ5zeOCSGJAJcBjOSGa7XaDt1bnwHUp\n\tbHz/1KHqxV+2Sc=","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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":23024,"web_url":"https://patchwork.libcamera.org/comment/23024/","msgid":"<165283161694.2416244.4617439271177282149@Monstersaurus>","date":"2022-05-17T23:53:36","subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid control values","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Christian Rauch via libcamera-devel (2022-04-19 23:34:12)\n> Hi Laurent and Kieran,\n> \n> How is libcamera related to Chromium and OpenCV? I don't see any\n> references to these projects in libcamera code. And do they require to\n> use the same C++ standard?\n\nThere is some development work that integrates libcamera into chromium,\nplus ChromeOS (chromium derived, but separate) builds against libcamera\nand can use the libcamera android hal.\n\nHowever, I don't think we should be worried about being tied to a\nChromium release, as I would expect that to use pipewire in the (near)\nfuture, to talk to libcamera.\n\nOpenCV is currently using C++11 as far as I can tell.... so perhaps that\nputs a libcamera integration there at a point that it needs updating\nanyway?\n\n\n\n> \n> Best,\n> Christian\n> \n> \n> Am 19.04.22 um 21:46 schrieb Laurent Pinchart:\n> > Hi Christian and Kieran,\n> >\n> > On Tue, Apr 19, 2022 at 11:48:53AM +0100, Kieran Bingham via libcamera-devel wrote:\n> >> Quoting Christian Rauch via libcamera-devel (2022-04-16 20:41:21)\n> >>> Hi Kieran,\n> >>>\n> >>> Is my patch series, including the std::optional change, something you\n> >>> would consider? I think it's a useful addition as it properly \"types\"\n> >>> the Span Controls and makes the handling of invalid return \"get\" values\n> >>> explicit.\n> >>\n> >> I think overall it sounded good - but I think Laurent mentioned he has\n> >> some concerens about std::optional in public API, as we may have some\n> >> limitations there, that are preventing us having a full C++17 usage.\n> >>\n> >> I can't recall what is holding us to C++14 on public API - but I would\n> >> hope we can look at what is required to bring that up to '17.\n> >\n> > I like where this is headed, but my concern is indeed the dependency on\n> > C++17. We've refrained from requiring C++17 due to Chromium being\n> > compiled with C++14 (we've actually briefly switched to C++17 and then\n> > reverted to C++14 when we noticed the compilation failures). Chromium\n> > seems to now support C++17, but can we assume everything else (or at\n> > least everything else that matters) does ? How about OpenCV for instance\n> > ?\n> >\n> > I also have a feeling that we could combine the existing ControlValue\n> > class with std::optional to achieve a similar result, but I haven't\n> > looked into it in details.\n> >\n> >>> Am 08.04.22 um 23:29 schrieb Christian Rauch via libcamera-devel:\n> >>>> Am 08.04.22 um 13:06 schrieb Kieran Bingham:\n> >>>>> Quoting Christian Rauch via libcamera-devel (2022-04-08 02:42:31)\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> >>>>> This looks like a really good way to express the controls from a list. I\n> >>>>> really like the value_or() that it brings to allow the code to set a\n> >>>>> default.\n> >>>>>\n> >>>>>\n> >>>>> I expect this will have knock-on effects to other out of tree\n> >>>>> applications using the control framework so we might want to coordinate\n> >>>>> the merge of this.\n> >>>>>\n> >>>>> Though I notice there's fairly minimal changes to cam and qcam. Do you\n> >>>>> know if your build includes the v4l2 adaptation layer and gstreamer?\n> >>>>> Does this API change cause definate breakage to users?\n> >>>>>\n> >>>>> (It's ok if it does, that's preciesly why we are not ABI stable).\n> >>>>\n> >>>> This is definitely a breaking change as it changes the public API. But\n> >>>> the changes that have to be made are quite trivial. You only have to add\n> >>>> \".value()\" or \".value_or(...)\" to the old code.\n> >>>>\n> >>>> I don't know about the v4l2 wrapper and gstreamer. There might be some\n> >>>> code that is not compiled on my setup. But the \"qcam\" application still\n> >>>> works. And I think this one is using the v4l2 wrapper.\n> >>>>\n> >>>>>> Signed-off-by: Christian Rauch \n> >>>>>> ---\n> >>>>>> include/libcamera/controls.h | 6 +++---\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> >>>>>> 6 files changed, 24 insertions(+), 21 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>>>>> index 665bcac1..57b777e9 100644\n> >>>>>> --- a/include/libcamera/controls.h\n> >>>>>> +++ b/include/libcamera/controls.h\n> >>>>>> @@ -167,7 +167,7 @@ public:\n> >>>>>>\n> >>>>>> using V = typename T::value_type;\n> >>>>>> const V *value = reinterpret_cast(data().data());\n> >>>>>> - return { value, numElements_ };\n> >>>>>> + return T{ value, numElements_ };\n> >>>>>> }\n> >>>>>>\n> >>>>>> #ifndef __DOXYGEN__\n> >>>>>> @@ -373,11 +373,11 @@ public:\n> >>>>>> bool contains(unsigned int id) const;\n> >>>>>>\n> >>>>>> template\n> >>>>>> - T get(const Control &ctrl) const\n> >>>>>> + std::optional get(const Control &ctrl) const\n> >>>>>> {\n> >>>>>> const ControlValue *val = find(ctrl.id());\n> >>>>>> if (!val)\n> >>>>>> - return T{};\n> >>>>>> + return std::nullopt;\n> >>>>>>\n> >>>>>> return val->get();\n> >>>>>> }\n> >>>>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> >>>>>> index c7f664b9..853a78ed 100644\n> >>>>>> --- a/src/cam/main.cpp\n> >>>>>> +++ b/src/cam/main.cpp\n> >>>>>> @@ -293,7 +293,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >>>>>> * is only used if the location isn't present or is set to External.\n> >>>>>> */\n> >>>>>> if (props.contains(properties::Location)) {\n> >>>>>> - switch (props.get(properties::Location)) {\n> >>>>>> + switch (props.get(properties::Location).value_or(int32_t{})) {\n> >>>>>\n> >>>>> Is there a way to do this without the value_or() in conditions where the\n> >>>>> value has already been guaranteed to exist?\n> >>>>>\n> >>>>> Here we have just checked that the lists contains a\n> >>>>> properties::Lcoation, so we 'know' that it will never process the\n> >>>>> '_or()' part.\n> >>>>>\n> >>>>> Looking at https://en.cppreference.com/w/cpp/utility/optional\n> >>>>>\n> >>>>> I guess we can just use .value() in the locations where we already check\n> >>>>> for the presence. I suspect this could lead to a code refactor to just\n> >>>>> use the optional to determine the properties existance instead of\n> >>>>> .contains() - but that could certainly be done on top.\n> >>>>>\n> >>>>> Perhaps it might be better for consistency to use the value_or() variant\n> >>>>> on occasions though - even if we know it must already exist?\n> >>>>\n> >>>> \".value()\" will throw an exception if the \"std::optional\" does not\n> >>>> contain a value. If you can guarantee that a ControlValue contains a\n> >>>> value, then you can skip the check via \".has_value()\" or the fallback\n> >>>> via \".value_or(...)\" and use \".value()\" directly.\n> >>>>\n> >>>>>> case properties::CameraLocationFront:\n> >>>>>> addModel = false;\n> >>>>>> name = \"Internal front camera \";\n> >>>>>> @@ -313,7 +313,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >>>>>> * If the camera location is not availble use the camera model\n> >>>>>> * to build the camera name.\n> >>>>>> */\n> >>>>>> - name = \"'\" + props.get(properties::Model) + \"' \";\n> >>>>>> + name = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n> >>>>>> }\n> >>>>>>\n> >>>>>> name += \"(\" + camera->id() + \")\";\n> >>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>>> index 5a5cdf66..93b32e94 100644\n> >>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>>> @@ -934,7 +934,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> >>>>>>\n> >>>>>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> >>>>>> {\n> >>>>>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> >>>>>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n> >>>>>> RPiController::Metadata lastMetadata;\n> >>>>>> Span embeddedBuffer;\n> >>>>>>\n> >>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>>> index 60e01917..394221cb 100644\n> >>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>>> @@ -1146,7 +1146,7 @@ int PipelineHandlerIPU3::registerCameras()\n> >>>>>> /* Convert the sensor rotation to a transformation */\n> >>>>>> int32_t rotation = 0;\n> >>>>>> if (data->properties_.contains(properties::Rotation))\n> >>>>>> - rotation = data->properties_.get(properties::Rotation);\n> >>>>>> + rotation = data->properties_.get(properties::Rotation).value_or(int32_t{});\n> >>>>>> else\n> >>>>>> LOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> >>>>>> << cio2->sensor()->id()\n> >>>>>> @@ -1341,7 +1341,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> >>>>>> request->metadata().set(controls::draft::PipelineDepth, 3);\n> >>>>>> /* \\todo Actually apply the scaler crop region to the ImgU. */\n> >>>>>> if (request->controls().contains(controls::ScalerCrop))\n> >>>>>> - cropRegion_ = request->controls().get(controls::ScalerCrop);\n> >>>>>> + cropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n> >>>>>> request->metadata().set(controls::ScalerCrop, cropRegion_);\n> >>>>>>\n> >>>>>> if (frameInfos_.tryComplete(info))\n> >>>>>> @@ -1442,7 +1442,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>>>>> ev.op = ipa::ipu3::EventStatReady;\n> >>>>>> ev.frame = info->id;\n> >>>>>> ev.bufferId = info->statBuffer->cookie();\n> >>>>>> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> >>>>>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp).value_or(int64_t{});\n> >>>>>> ev.sensorControls = info->effectiveSensorControls;\n> >>>>>> ipa_->processEvent(ev);\n> >>>>>> }\n> >>>>>> @@ -1477,8 +1477,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> >>>>>> if (!request->controls().contains(controls::draft::TestPatternMode))\n> >>>>>> return;\n> >>>>>>\n> >>>>>> - const int32_t testPatternMode = request->controls().get(\n> >>>>>> - controls::draft::TestPatternMode);\n> >>>>>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(int32_t{});\n> >>>>>\n> >>>>> This looks like a section of code that could now use optional for\n> >>>>> cleaner code I think. I see above we return early if the control is not\n> >>>>> present, and only call setTestPatternMode if it is set.\n> >>>>>\n> >>>>>\n> >>>>> Again, I think this patch is just bringing in the std::optional - so it\n> >>>>> shouldn't have to 'make everything use the best implementation' - but I\n> >>>>> can see benefits it can bring.\n> >>>>>\n> >>>>> I think even though we know it's guaranteed to exist here, the use of\n> >>>>> value_or() is fine with me, as it highlights that this code could\n> >>>>> perhaps be simplified later.\n> >>>>\n> >>>> The current \".value_or(...)\" implementation is the closest to the old\n> >>>> behaviour, which would return a default contructed object in case of\n> >>>> failure. You certainly can change that behaviour if you arec ertain that\n> >>>> a value exists.\n> >>>>\n> >>>>>>\n> >>>>>> int ret = cio2_.sensor()->setTestPatternMode(\n> >>>>>> static_cast(testPatternMode));\n> >>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>>> index 0fa294d4..63d57033 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> >>>>>> * error means the platform can never run. Let's just print a warning\n> >>>>>> * and continue regardless; the rotation is effectively set to zero.\n> >>>>>> */\n> >>>>>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> >>>>>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(int32_t{});\n> >>>>>> bool success;\n> >>>>>> Transform rotationTransform = transformFromRotation(rotation, &success);\n> >>>>>> if (!success)\n> >>>>>> @@ -1696,7 +1696,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> >>>>>> * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> >>>>>> */\n> >>>>>> if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {\n> >>>>>> - libcamera::Span colourGains = controls.get(libcamera::controls::ColourGains);\n> >>>>>> + libcamera::Span colourGains =\n> >>>>>> + controls.get(libcamera::controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> >>>>>> /* The control wants linear gains in the order B, Gb, Gr, R. */\n> >>>>>> ControlList ctrls(sensor_->controls());\n> >>>>>> std::array gains{\n> >>>>>> @@ -2031,7 +2032,7 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> >>>>>> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> >>>>>> {\n> >>>>>> if (controls.contains(controls::ScalerCrop)) {\n> >>>>>> - Rectangle nativeCrop = controls.get(controls::ScalerCrop);\n> >>>>>> + Rectangle nativeCrop = controls.get(controls::ScalerCrop).value_or(Rectangle{});\n> >>>>>\n> >>>>> I'm starting to wonder if a templated get_or would be useful as the type\n> >>>>> would be defined there (doesn't have to be here, just an idea)\n> >>>>>\n> >>>>> It would reduce line length on null initialisers:\n> >>>>>\n> >>>>> controls.get_or(controls::ScalerCrop, {});\n> >>>>>\n> >>>>> And easily allow default parameters to be defined:\n> >>>>>\n> >>>>> controls.get_or(controls::ScalerCrop, {640,480});\n> >>>>>\n> >>>>> I suspect seeing how this all gets used will determine if it has value\n> >>>>> though.\n> >>>>\n> >>>> This is probably dependent on the situation, but I don't think that\n> >>>> initialising control values with the default is a good idea in every\n> >>>> case. The biggest advantage of \"std::optional\" is that you can properly\n> >>>> test for errors. In most cases, it is probably better to notify the user\n> >>>> about missing controls etc. instead of silently replacing the requested\n> >>>> values with the defaults.\n> >>>>\n> >>>>>>\n> >>>>>> if (!nativeCrop.width || !nativeCrop.height)\n> >>>>>> nativeCrop = { 0, 0, 1, 1 };\n> >>>>>> @@ -2069,7 +2070,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> >>>>>> Request *request)\n> >>>>>> {\n> >>>>>> request->metadata().set(controls::SensorTimestamp,\n> >>>>>> - bufferControls.get(controls::SensorTimestamp));\n> >>>>>> + bufferControls.get(controls::SensorTimestamp).value_or(int64_t{}));\n> >>>>>>\n> >>>>>> request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> >>>>>> }\n> >>>>>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> >>>>>> index 2fb527d8..030432e3 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> >>>>>> TIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> >>>>>>\n> >>>>>> if (cameraProperties.contains(properties::Model)) {\n> >>>>>> - std::string model = cameraProperties.get(properties::Model);\n> >>>>>> + std::string model = cameraProperties.get(properties::Model).value_or(std::string{});\n> >>>>>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());\n> >>>>>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> >>>>>> }\n> >>>>>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>>>>> const double eps = 1e-2;\n> >>>>>>\n> >>>>>> if (metadata.contains(controls::ColourGains)) {\n> >>>>>> - Span const &colourGains = metadata.get(controls::ColourGains);\n> >>>>>> + Span const &colourGains =\n> >>>>>> + metadata.get(controls::ColourGains).value_or(libcamera::Span({ 0, 0 }));\n> >>>>>> if (colourGains[0] > eps && colourGains[1] > eps) {\n> >>>>>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);\n> >>>>>> neutral[0] = 1.0 / colourGains[0]; /* red */\n> >>>>>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>>>>> }\n> >>>>>> }\n> >>>>>> if (metadata.contains(controls::ColourCorrectionMatrix)) {\n> >>>>>> - Span const &coeffs = metadata.get(controls::ColourCorrectionMatrix);\n> >>>>>> + Span const &coeffs =\n> >>>>>> + metadata.get(controls::ColourCorrectionMatrix).value_or(Span({ 0, 0, 0, 0, 0, 0, 0, 0, 0 }));\n> >>>>>> Matrix3d ccmSupplied(coeffs);\n> >>>>>> if (ccmSupplied.determinant() > eps)\n> >>>>>> ccm = ccmSupplied;\n> >>>>>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>>>>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;\n> >>>>>>\n> >>>>>> if (metadata.contains(controls::SensorBlackLevels)) {\n> >>>>>> - Span levels = metadata.get(controls::SensorBlackLevels);\n> >>>>>> + Span levels =\n> >>>>>> + metadata.get(controls::SensorBlackLevels).value_or(Span({ 0, 0, 0, 0 }));\n> >>>>>>\n> >>>>>> /*\n> >>>>>> * 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> >>>>>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);\n> >>>>>>\n> >>>>>> if (metadata.contains(controls::AnalogueGain)) {\n> >>>>>> - float gain = metadata.get(controls::AnalogueGain);\n> >>>>>> + float gain = metadata.get(controls::AnalogueGain).value_or(float{});\n> >>>>>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);\n> >>>>>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);\n> >>>>>> }\n> >>>>>>\n> >>>>>> if (metadata.contains(controls::ExposureTime)) {\n> >>>>>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;\n> >>>>>> + float exposureTime = metadata.get(controls::ExposureTime).value_or(float{}) / 1e6;\n> >>>>>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);\n> >>>>>> }\n> >>>>>>\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 135E8C3256\n\tfor ;\n\tTue, 17 May 2022 23:53:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F8C86150E;\n\tWed, 18 May 2022 01:53:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DC696041D\n\tfor ;\n\tWed, 18 May 2022 01:53:39 +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 D26994A8;\n\tWed, 18 May 2022 01:53:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652831621;\n\tbh=PhDF5PUy/Mnvtt0bW6jODoEM/koRcVUt+DFnK3xsoEo=;\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=QLREAmaToMEaFe+iIDQj+MSOYE61xnLKG5zg/k5b96yS2HnRmALbaXHiKvZ335ajh\n\tlG1r9Ap0NLbW5H7saQw3KYoieOEwdMGtvggFZ+theiZ8/7tlqmt19sME7PjB5WDjAj\n\t1EiGRXRn8NjmBBdqmot4JF2FJo88EyrIqTtw/2Kk6O0M2kFu7XT3Fr3wKQ0tAyE+z6\n\t1bE+yLJkx/o4QOGLcn9zvBM4P+9BcHC5v93Ykxyqrz62GQdKBf2kIQ0DX4MI4MvKOg\n\tZnZe+K4pAuYe5/Zrs6f1rHR0EQ5M1WXkgz17XM71iXI0rDImw4DCRmg9g0wmz/rkRA\n\t7LJTxCifsmYbg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652831618;\n\tbh=PhDF5PUy/Mnvtt0bW6jODoEM/koRcVUt+DFnK3xsoEo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Kxk6PognR3USFePBPUIkADs+9sOuGSGMCO48YfcRZMhNqAMcv3jxFwBqgsslGkDvx\n\tsSxJQjKUwLm+vQ8ztdGfaisfOAoSZs5iuX94MzE5vBfb6JZZaQwCE3o0n3EK+vt2Ws\n\tvOUsykfDZa/L0LJ1uAcMJJNYdzQsHbniLylpJfbA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Kxk6Pogn\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220408014231.231083-1-Rauch.Christian@gmx.de>\n\t<20220408014231.231083-5-Rauch.Christian@gmx.de>\n\t<164941960610.22830.16120063966032672613@Monstersaurus>\n\t<9b3c6558-fe9b-5695-d73d-fceb3c3cfb01@gmx.de>\n\t\n\t<165036533305.2548121.7066095784140188670@Monstersaurus>\n\t\n\t","To":"Christian Rauch ,\n\tChristian Rauch via libcamera-devel ","Date":"Wed, 18 May 2022 00:53:36 +0100","Message-ID":"<165283161694.2416244.4617439271177282149@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] use std::optional to handle\n\tinvalid 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\" "}}]