[{"id":23295,"web_url":"https://patchwork.libcamera.org/comment/23295/","msgid":"","date":"2022-06-02T09:58:32","subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use\n\tstd::optional to handle invalid 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 Thu, Jun 02, 2022 at 12:17:59AM +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\nI think I like this :-)\n\n> Signed-off-by: Christian Rauch \n> ---\n> include/libcamera/controls.h | 7 ++++---\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 | 10 ++++++----\n> src/qcam/dng_writer.cpp | 15 +++++++++------\n> 6 files changed, 26 insertions(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 665bcac1..363e7809 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> #include \n> +#include \n\nPlease keep headers alphabetically sorted. Are you using the\ncheckstyle.py style checker ? You can copy utils/hooks/post-commit (or\npre-commit if preferred) git hook to .git/hooks/ to automate this.\n\n> \n> #include \n> #include \n> @@ -167,7 +168,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\nThis seems unrelated. It makes the code more explicit though so I think\nit's good, but I'd at least mention it in the commit message as a \"While\nat it, ...\" item.\n\n> \t}\n> \n> #ifndef __DOXYGEN__\n> @@ -373,11 +374,11 @@ public:\n> \tbool contains(unsigned int id) const;\n> \n> \ttemplate\n> -\tT get(const Control &ctrl) const\n> +\tstd::optional get(const Control &ctrl) const\n> \t{\n> \t\tconst ControlValue *val = find(ctrl.id());\n> \t\tif (!val)\n> -\t\t\treturn T{};\n> +\t\t\treturn std::nullopt;\n> \n> \t\treturn val->get();\n> \t}\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index 79875ed7..9b773931 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> \t * is only used if the location isn't present or is set to External.\n> \t */\n> \tif (props.contains(properties::Location)) {\n> -\t\tswitch (props.get(properties::Location)) {\n> +\t\tswitch (props.get(properties::Location).value_or(int32_t{})) {\n\nGiven that the props.contains() check guarantees that the value is\npresent, we could use\n\n\t\tswitch (*props.get(properties::Location)) {\n\nhere. Alternatively, the code could be changed to\n\n\tconst auto location = props.get(properties::Location);\n\tif (location) {\n\t\tswitch (*location) {\n\nwhich avoids the double lookup. I think I like this one better. Any\nopinion from anyone ?\n\nThe same comment applies to several locations below.\n\n> \t\tcase properties::CameraLocationFront:\n> \t\t\taddModel = false;\n> \t\t\tname = \"Internal front camera \";\n> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> \t\t * If the camera location is not availble use the camera model\n> \t\t * to build the camera name.\n> \t\t */\n> -\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n> +\t\tname = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n\nThis brings the question of what we should do for properties (and\ncontrols below) that are mandatory. One option would be to simply write\n\n\t\tname = \"'\" + *props.get(properties::Model) + \"' \";\n\nThis leads to undefined behaviour if the property isn't present, which\nisn't very nice. On the other hand, the runtime check is in theory not\nnecessary, as the property is mandatory (which should be ensured through\ncompliance testing).\n\nIf we want to keep the check, I'd like to shorten the line a bit with\n\n\t\tname = \"'\" + props.get(properties::Model).value_or(\"\") + \"' \";\n> \t}\n> \n> \tname += \"(\" + camera->id() + \")\";\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 3b126bb5..00600a2e 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> \n> void IPARPi::prepareISP(const ISPConfig &data)\n> {\n> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n\nSame comment here, we could drop the value_or(), or call value_or(0).\nSame in other locations below where applicable.\n\n> \tRPiController::Metadata lastMetadata;\n> \tSpan embeddedBuffer;\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fd989e61..1e9e5081 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Convert the sensor rotation to a transformation */\n> \t\tint32_t rotation = 0;\n> \t\tif (data->properties_.contains(properties::Rotation))\n> -\t\t\trotation = data->properties_.get(properties::Rotation);\n> +\t\t\trotation = data->properties_.get(properties::Rotation).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> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> \tif (request->controls().contains(controls::ScalerCrop))\n> -\t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n> +\t\tcropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n> \n> \tif (frameInfos_.tryComplete(info))\n> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \t\treturn;\n> \t}\n> \n> -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(int64_t{}),\n> \t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n> }\n> \n> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> \tif (!request->controls().contains(controls::draft::TestPatternMode))\n> \t\treturn;\n> \n> -\tconst int32_t testPatternMode = request->controls().get(\n> -\t\tcontrols::draft::TestPatternMode);\n> +\tconst int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(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 adc397e8..556af882 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> @@ -1706,7 +1706,9 @@ 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).\\\n> +\t\t\t\tvalue_or(libcamera::Span({ 0, 0 }));\n> \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n> \t\tControlList ctrls(sensor_->controls());\n> \t\tstd::array gains{\n> @@ -2041,7 +2043,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> @@ -2079,7 +2081,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 34c8df5a..e119ca52 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 45AE1BD161\n\tfor ;\n\tThu, 2 Jun 2022 09:58:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7731265631;\n\tThu, 2 Jun 2022 11:58:38 +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 0BAA36040E\n\tfor ;\n\tThu, 2 Jun 2022 11:58:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8BC9D6BD;\n\tThu, 2 Jun 2022 11:58:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654163918;\n\tbh=U0xiQAmzf4tcwoV59NPiDQ8A1djguQ1lE7QTtsX+dQk=;\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=4iCkA0ASgHuhQoRxZVlcexThgIxIbGb42wDoG/CWJL5GAdVBrF/IFSlk29RMUK6nl\n\tnZ6imO5/XO8AyJpqUe4y/5TozBMkJzEMiEdzxhP9REp9XR4p1r6bw1R/A2rmlREHfo\n\t3B60qKBgbtDrPtAribt0cCx+qDts64X3Tl5D474wljurPxRBA53ArVcT4UhNYHF2gS\n\trvvsfHcHNLLPBXz+fOi1CTtsT6rY2w8+IywFAOt8TCfiZfpRoxW2phJMh7qHZEzLQG\n\tsHb+eKu/BQNV7ZUyuRnWuIn4H+gGgQZa8W+04/0PuL8AWduDz/+oKLaw/MPiaXyE4l\n\tJfqq//23jKMEQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654163916;\n\tbh=U0xiQAmzf4tcwoV59NPiDQ8A1djguQ1lE7QTtsX+dQk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H1ciLCfvoNujMA97ljsscfbJOy75pFpMPfB398yaZk1BgKznuHnsAcbNGiicQO7il\n\tv/wHaHP+HtMe36ih0quY0j5ZFn4S3hkH2XV2XfpCm0LsSpD9npdWjlsgbMa3aU/hd5\n\tQoMbyCYIf4qEuyg6egQJG+VEiBY1DLHTdOMn1uHk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"H1ciLCfv\"; dkim-atps=neutral","Date":"Thu, 2 Jun 2022 12:58:32 +0300","To":"Christian Rauch ","Message-ID":"","References":"<20220601231802.16735-1-Rauch.Christian@gmx.de>\n\t<20220601231802.16735-2-Rauch.Christian@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220601231802.16735-2-Rauch.Christian@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"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":23298,"web_url":"https://patchwork.libcamera.org/comment/23298/","msgid":"<474c1407-ecc0-9af4-d203-4a38efd8868f@gmx.de>","date":"2022-06-02T23:48:15","subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use\n\tstd::optional to handle invalid control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Laurent,\n\nThanks for looking at this.\n\nAm 02.06.22 um 10:58 schrieb Laurent Pinchart:\n> Hi Christian,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 02, 2022 at 12:17:59AM +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> I think I like this :-)\n>\n>> Signed-off-by: Christian Rauch \n>> ---\n>> include/libcamera/controls.h | 7 ++++---\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 | 10 ++++++----\n>> src/qcam/dng_writer.cpp | 15 +++++++++------\n>> 6 files changed, 26 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 665bcac1..363e7809 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -13,6 +13,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>\n> Please keep headers alphabetically sorted. Are you using the\n> checkstyle.py style checker ? You can copy utils/hooks/post-commit (or\n> pre-commit if preferred) git hook to .git/hooks/ to automate this.\n\nI stopped using the automated checker in my IDE because I got a lot of\n\"false positives\" where it adapted the style according to the\nclang-format file, that were later rejected on the mailing list. But I\nwill note down that this header order should be included.\n\n>\n>>\n>> #include \n>> #include \n>> @@ -167,7 +168,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>\n> This seems unrelated. It makes the code more explicit though so I think\n> it's good, but I'd at least mention it in the commit message as a \"While\n> at it, ...\" item.\n\nThis might have been a left-over from an earlier version of this patch\nset. I am removing this in a future version as it is not necessary.\n\n>\n>> \t}\n>>\n>> #ifndef __DOXYGEN__\n>> @@ -373,11 +374,11 @@ public:\n>> \tbool contains(unsigned int id) const;\n>>\n>> \ttemplate\n>> -\tT get(const Control &ctrl) const\n>> +\tstd::optional get(const Control &ctrl) const\n>> \t{\n>> \t\tconst ControlValue *val = find(ctrl.id());\n>> \t\tif (!val)\n>> -\t\t\treturn T{};\n>> +\t\t\treturn std::nullopt;\n>>\n>> \t\treturn val->get();\n>> \t}\n>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n>> index 79875ed7..9b773931 100644\n>> --- a/src/cam/main.cpp\n>> +++ b/src/cam/main.cpp\n>> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>> \t * is only used if the location isn't present or is set to External.\n>> \t */\n>> \tif (props.contains(properties::Location)) {\n>> -\t\tswitch (props.get(properties::Location)) {\n>> +\t\tswitch (props.get(properties::Location).value_or(int32_t{})) {\n>\n> Given that the props.contains() check guarantees that the value is\n> present, we could use\n>\n> \t\tswitch (*props.get(properties::Location)) {\n>\n> here. Alternatively, the code could be changed to\n>\n> \tconst auto location = props.get(properties::Location);\n> \tif (location) {\n> \t\tswitch (*location) {\n>\n> which avoids the double lookup. I think I like this one better. Any\n> opinion from anyone ?\n\nI initially replicated the old behaviour by returning a default\nconstructed object if the value is not valid. But now, I like your idea\nbetter since \"if (location)\" makes this much more clear.\n\n>\n> The same comment applies to several locations below.\n>\n>> \t\tcase properties::CameraLocationFront:\n>> \t\t\taddModel = false;\n>> \t\t\tname = \"Internal front camera \";\n>> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>> \t\t * If the camera location is not availble use the camera model\n>> \t\t * to build the camera name.\n>> \t\t */\n>> -\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n>> +\t\tname = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n>\n> This brings the question of what we should do for properties (and\n> controls below) that are mandatory. One option would be to simply write\n>\n> \t\tname = \"'\" + *props.get(properties::Model) + \"' \";\n>\n> This leads to undefined behaviour if the property isn't present, which\n> isn't very nice. On the other hand, the runtime check is in theory not\n> necessary, as the property is mandatory (which should be ensured through\n> compliance testing).\n>\n> If we want to keep the check, I'd like to shorten the line a bit with\n>\n> \t\tname = \"'\" + props.get(properties::Model).value_or(\"\") + \"' \";\n\nI would not use \"*props.get()\" unless it was checked before that\n\"props.has_value()\" is true. If the compliance test fails to recognise\nan undefined \"Model\" in some situation,s then this will dereference a\nNULL pointer, which is hard to debug.\n\nSo unless it is checked before that \"props\" is valid, I would always\nfall back to the default value like:\n\"props.get().value_or()\"\n\n>> \t}\n>>\n>> \tname += \"(\" + camera->id() + \")\";\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 3b126bb5..00600a2e 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>>\n>> void IPARPi::prepareISP(const ISPConfig &data)\n>> {\n>> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n>> +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n>\n> Same comment here, we could drop the value_or(), or call value_or(0).\n> Same in other locations below where applicable.\n\nDo you want to drop the \"value_or\" because it is somewhere else\nguaranteed that this control exists? Or do you just explicitely set \"0\"\ninstead of the default constructed int64_t?\n\n>\n>> \tRPiController::Metadata lastMetadata;\n>> \tSpan embeddedBuffer;\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index fd989e61..1e9e5081 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()\n>> \t\t/* Convert the sensor rotation to a transformation */\n>> \t\tint32_t rotation = 0;\n>> \t\tif (data->properties_.contains(properties::Rotation))\n>> -\t\t\trotation = data->properties_.get(properties::Rotation);\n>> +\t\t\trotation = data->properties_.get(properties::Rotation).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>> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n>> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n>> \tif (request->controls().contains(controls::ScalerCrop))\n>> -\t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n>> +\t\tcropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n>> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n>>\n>> \tif (frameInfos_.tryComplete(info))\n>> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>> \t\treturn;\n>> \t}\n>>\n>> -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n>> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(int64_t{}),\n>> \t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n>> }\n>>\n>> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n>> \tif (!request->controls().contains(controls::draft::TestPatternMode))\n>> \t\treturn;\n>>\n>> -\tconst int32_t testPatternMode = request->controls().get(\n>> -\t\tcontrols::draft::TestPatternMode);\n>> +\tconst int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(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 adc397e8..556af882 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>> @@ -1706,7 +1706,9 @@ 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).\\\n>> +\t\t\t\tvalue_or(libcamera::Span({ 0, 0 }));\n>> \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n>> \t\tControlList ctrls(sensor_->controls());\n>> \t\tstd::array gains{\n>> @@ -2041,7 +2043,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>> @@ -2079,7 +2081,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 34c8df5a..e119ca52 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>>\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 2FDC2BD161\n\tfor ;\n\tThu, 2 Jun 2022 23:48:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5280065638;\n\tFri, 3 Jun 2022 01:48:19 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.15])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81A4A60413\n\tfor ;\n\tFri, 3 Jun 2022 01:48:17 +0200 (CEST)","from [192.168.1.209] ([92.18.80.244]) by mail.gmx.net (mrgmx004\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id 1MOiHl-1o9Dwd2Zah-00Q7gm\n\tfor\n\t; Fri, 03 Jun 2022 01:48:16 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654213699;\n\tbh=1HmowXrCdZ2v9U6d8oN9DQsRTVRz25Qlxp46QAcd024=;\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=MqlcAAilDhiV1WxAWaNnvTaS+k8WjawJRRQyhUYCdu8FBHKH9HzYxeLRArU2V3t9D\n\tD5raeHCfqth2tKRPYkINsas35CuV5SIEU1IqSFhRJqLTmX6knN6XwauUYPnYpzmE/n\n\to1uf5TcSVcfWdQ8hPdl+4K/BP8PX68QWEffombTDbd69sh7pLkjvlgikF8stroUsOd\n\tHJ0FpKTQm2cWc97x6GqPv4PkntWof2AacLGalXxRXrroznm8O3xJVDSi7h3eaADCjy\n\tooNM60Jyyqj0lg+yWR/19gf5kY/+M6IWdLmfqVVUS/2jNBYI76ez5IFqkadj1mtGE6\n\tP/4LWzBR69/oA==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1654213697;\n\tbh=1HmowXrCdZ2v9U6d8oN9DQsRTVRz25Qlxp46QAcd024=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=SmMdmtdZSvzGF/eq+c0s9yfvGM4izjTXG0Gcj9p8jh2dM0BDPTrO8Q0ziqJXfG800\n\txi0uB1QMYbv7rAmNxgaPjIHyK2oQkSh/tbi2HgSdf/3KW5sPDBjJE426wODdkBiFYt\n\tU5pBalNC0/7T7dBGJyg497zJQOJHMjEI3yq30JXk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"SmMdmtdZ\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<474c1407-ecc0-9af4-d203-4a38efd8868f@gmx.de>","Date":"Fri, 3 Jun 2022 00:48:15 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-GB","To":"libcamera devel ","References":"<20220601231802.16735-1-Rauch.Christian@gmx.de>\n\t<20220601231802.16735-2-Rauch.Christian@gmx.de>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:wqY1qeGK7H3gNfXqQK3CJaT5KaOPlkvSRsDwHaUF1LuiNJPfyOX\n\tkmltAB6AMdCRIZf4Rqa3Cyfb4FdbqyPTOGh6KTnJf7+g5HXt+3gFvC/4uzxWd5wBTihGvCl\n\tYiH3iatY4WF5u05Eh+KGQxVu4M/Hv2q9u+mxWgxSamHVdcrFXXO4Y+nNNjFLuloJO4s0vhJ\n\t1Rgh+hYMiI6+sU22GwchA==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:d395HZHBcr0=:dcdbAr+zsNNqhau77ruIWH\n\tDZ7RLzjaopINns5tRDcngXbQNSuLiBvuTjFyXLJurQK6HIP/HfaPyhRakEQ+DJx20H2MG8Sq6\n\ttdEmHFHstduVxxhN5W0Gg+6o75BS6pmBEJw83nd14KDdXCP3Ejos37VgGZ6/3riVTZO852lHK\n\tnJCDj7DrjF/3pOh+cYaiLKgkwYFEdBZsqy7fOFnt7fN8ffhTbl9WMoml7e1N1vn8p3nKHFcSh\n\tBcctIeRV7XsTtDDYlGe5IxmTP6i64IpWs9aQZ1TRLsYbOzA4hoVtgChNGNmC56VI5YU5UnnDO\n\tXnM/ZujTVrESiLkJTD7U+BkDMS0q7XX/ZWsQbrPi1Qs9F6JpXQBDAcZC55ltjEkhSl2/mH9oY\n\tp3FwDmqV1w0pFF74dK8kAB9SGdMdqNhvPFpt7cpuoGuUEchilo7Jhdn9EqoLszeOyofuIAVnm\n\tqKtTXTIGNj4xQgXZcSLGxjbzpYw7lQ0IwCa7WtLc6f0iHwcXqQDb0o/txehOvzlU9GaYvpInV\n\tiqbGk9UE7REAq7nCksoou+xzl4bLMzcjWq8tR851RVeqlkBocoeHGWGyxhfmp+b6o5Gw3r+IL\n\tav79Agf+NsmJMCY5ZuPYpc+ou7n1gpKDzMl5sYmKSOtGuZqMxdilVyv/3WQoNW5GxXF83/rPB\n\tuZ1QgaSLU9McfGdujKv/L2lsnXtodAseFe5m9TY4CmNP3Txd/ZAJtFJbtQOnnRpPIaOVcbSR7\n\tnK/+UgSrAybzLBRR7pwFDb+pBcHMOCHcncDsgHv7UtjWt1T6xu1KMvjWOUoO2P1hbnlCQxtbZ\n\tX5wnA7YwF9pdNfSYSNord6UztzxrfbIVmBIuAwUvfYqmO4bU14/H9yonzGoRSC1ym1AOW54pv\n\tFOKt//ty5yruwBHR1YtPGWZvsfr9ugBtIDbDMnKYbvqReobOm5GpOA8ukd1Ov/jmqFOiyEJc4\n\trDdDbaFZmdrsQoev7PQQT59fPHH1gOr6Extkv/iWd8MIAAXZ/vYyBlPx17thHBHJjO+0jBT9X\n\t5hgx9FYpsBzmK/r+6VBxw/JOz3S5Lk1flCwgFVpkT6e8VgYSJfgj4TIXOlN/RVPKBWVIM6CQV\n\tZAQ3IoR5WBRR3DAO/jSx2ehBfQR2DysGTS185fdJHW1bMhO7POgYy10/w==","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23304,"web_url":"https://patchwork.libcamera.org/comment/23304/","msgid":"","date":"2022-06-03T09:49:30","subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use\n\tstd::optional to handle invalid 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\nOn Fri, Jun 03, 2022 at 12:48:15AM +0100, Christian Rauch via libcamera-devel wrote:\n> Am 02.06.22 um 10:58 schrieb Laurent Pinchart:\n> > On Thu, Jun 02, 2022 at 12:17:59AM +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> > I think I like this :-)\n> >\n> >> Signed-off-by: Christian Rauch \n> >> ---\n> >> include/libcamera/controls.h | 7 ++++---\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 | 10 ++++++----\n> >> src/qcam/dng_writer.cpp | 15 +++++++++------\n> >> 6 files changed, 26 insertions(+), 21 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >> index 665bcac1..363e7809 100644\n> >> --- a/include/libcamera/controls.h\n> >> +++ b/include/libcamera/controls.h\n> >> @@ -13,6 +13,7 @@\n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >\n> > Please keep headers alphabetically sorted. Are you using the\n> > checkstyle.py style checker ? You can copy utils/hooks/post-commit (or\n> > pre-commit if preferred) git hook to .git/hooks/ to automate this.\n> \n> I stopped using the automated checker in my IDE because I got a lot of\n> \"false positives\" where it adapted the style according to the\n> clang-format file, that were later rejected on the mailing list. But I\n> will note down that this header order should be included.\n\nUnfortunately we haven't been able to find a clang-format configuration\nthat works perfectly :-S I'd still recommend running checkstyle.py, and\ncherry-picking the warnings that you think are appropriate. We keep\nimproving checkstyle.py (albeit at a slow pace), but the issues related\nto clang-format are more difficult to address.\n\n> >>\n> >> #include \n> >> #include \n> >> @@ -167,7 +168,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> >\n> > This seems unrelated. It makes the code more explicit though so I think\n> > it's good, but I'd at least mention it in the commit message as a \"While\n> > at it, ...\" item.\n> \n> This might have been a left-over from an earlier version of this patch\n> set. I am removing this in a future version as it is not necessary.\n> \n> >> \t}\n> >>\n> >> #ifndef __DOXYGEN__\n> >> @@ -373,11 +374,11 @@ public:\n> >> \tbool contains(unsigned int id) const;\n> >>\n> >> \ttemplate\n> >> -\tT get(const Control &ctrl) const\n> >> +\tstd::optional get(const Control &ctrl) const\n> >> \t{\n> >> \t\tconst ControlValue *val = find(ctrl.id());\n> >> \t\tif (!val)\n> >> -\t\t\treturn T{};\n> >> +\t\t\treturn std::nullopt;\n> >>\n> >> \t\treturn val->get();\n> >> \t}\n> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> >> index 79875ed7..9b773931 100644\n> >> --- a/src/cam/main.cpp\n> >> +++ b/src/cam/main.cpp\n> >> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >> \t * is only used if the location isn't present or is set to External.\n> >> \t */\n> >> \tif (props.contains(properties::Location)) {\n> >> -\t\tswitch (props.get(properties::Location)) {\n> >> +\t\tswitch (props.get(properties::Location).value_or(int32_t{})) {\n> >\n> > Given that the props.contains() check guarantees that the value is\n> > present, we could use\n> >\n> > \t\tswitch (*props.get(properties::Location)) {\n> >\n> > here. Alternatively, the code could be changed to\n> >\n> > \tconst auto location = props.get(properties::Location);\n> > \tif (location) {\n> > \t\tswitch (*location) {\n> >\n> > which avoids the double lookup. I think I like this one better. Any\n> > opinion from anyone ?\n> \n> I initially replicated the old behaviour by returning a default\n> constructed object if the value is not valid. But now, I like your idea\n> better since \"if (location)\" makes this much more clear.\n> \n> > The same comment applies to several locations below.\n> >\n> >> \t\tcase properties::CameraLocationFront:\n> >> \t\t\taddModel = false;\n> >> \t\t\tname = \"Internal front camera \";\n> >> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >> \t\t * If the camera location is not availble use the camera model\n> >> \t\t * to build the camera name.\n> >> \t\t */\n> >> -\t\tname = \"'\" + props.get(properties::Model) + \"' \";\n> >> +\t\tname = \"'\" + props.get(properties::Model).value_or(std::string{}) + \"' \";\n> >\n> > This brings the question of what we should do for properties (and\n> > controls below) that are mandatory. One option would be to simply write\n> >\n> > \t\tname = \"'\" + *props.get(properties::Model) + \"' \";\n> >\n> > This leads to undefined behaviour if the property isn't present, which\n> > isn't very nice. On the other hand, the runtime check is in theory not\n> > necessary, as the property is mandatory (which should be ensured through\n> > compliance testing).\n> >\n> > If we want to keep the check, I'd like to shorten the line a bit with\n> >\n> > \t\tname = \"'\" + props.get(properties::Model).value_or(\"\") + \"' \";\n> \n> I would not use \"*props.get()\" unless it was checked before that\n> \"props.has_value()\" is true. If the compliance test fails to recognise\n> an undefined \"Model\" in some situation,s then this will dereference a\n> NULL pointer, which is hard to debug.\n> \n> So unless it is checked before that \"props\" is valid, I would always\n> fall back to the default value like:\n> \"props.get().value_or()\"\n\nAt the end of the day we need to ensure it won't crash, that's very\nclear. .value_or() ensures that, so would the implicit knowledge that\nthe value is present due to the fact that the API requires it. The\nlatter can be further enforced through compliance testing, but I agree\nit sounds more fragile, as compliance testing may not be 100% accurate.\nI'm not sure where exactly to draw the line, it's the usual question of\nhow to handle API contracts that are not met, what should a caller do\nwhen a callee returns something that is invalid according to the API\ndocumentation ?\n\n> >> \t}\n> >>\n> >> \tname += \"(\" + camera->id() + \")\";\n> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> index 3b126bb5..00600a2e 100644\n> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> >>\n> >> void IPARPi::prepareISP(const ISPConfig &data)\n> >> {\n> >> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);\n> >> +\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(int64_t{});\n> >\n> > Same comment here, we could drop the value_or(), or call value_or(0).\n> > Same in other locations below where applicable.\n> \n> Do you want to drop the \"value_or\" because it is somewhere else\n> guaranteed that this control exists? Or do you just explicitely set \"0\"\n> instead of the default constructed int64_t?\n\nEither :-) Dropping .value_or() is one option (based on the outcome of\nthe discussion above), and if we want to keep it, I'd write .value_or(0)\ninstead of using the default-constructed int64_t as it's shorter and\nmore explicit.\n\n> >> \tRPiController::Metadata lastMetadata;\n> >> \tSpan embeddedBuffer;\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index fd989e61..1e9e5081 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()\n> >> \t\t/* Convert the sensor rotation to a transformation */\n> >> \t\tint32_t rotation = 0;\n> >> \t\tif (data->properties_.contains(properties::Rotation))\n> >> -\t\t\trotation = data->properties_.get(properties::Rotation);\n> >> +\t\t\trotation = data->properties_.get(properties::Rotation).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> >> @@ -1331,7 +1331,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> >> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> >> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> >> \tif (request->controls().contains(controls::ScalerCrop))\n> >> -\t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n> >> +\t\tcropRegion_ = request->controls().get(controls::ScalerCrop).value_or(Rectangle{});\n> >> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n> >>\n> >> \tif (frameInfos_.tryComplete(info))\n> >> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >> \t\treturn;\n> >> \t}\n> >>\n> >> -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> >> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(int64_t{}),\n> >> \t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n> >> }\n> >>\n> >> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)\n> >> \tif (!request->controls().contains(controls::draft::TestPatternMode))\n> >> \t\treturn;\n> >>\n> >> -\tconst int32_t testPatternMode = request->controls().get(\n> >> -\t\tcontrols::draft::TestPatternMode);\n> >> +\tconst int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(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 adc397e8..556af882 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> >> @@ -1706,7 +1706,9 @@ 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).\\\n> >> +\t\t\t\tvalue_or(libcamera::Span({ 0, 0 }));\n> >> \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n> >> \t\tControlList ctrls(sensor_->controls());\n> >> \t\tstd::array gains{\n> >> @@ -2041,7 +2043,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> >> @@ -2079,7 +2081,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 34c8df5a..e119ca52 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 8AD31BD160\n\tfor ;\n\tFri, 3 Jun 2022 09:49:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CED5965636;\n\tFri, 3 Jun 2022 11:49:35 +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 CA0C4633A6\n\tfor ;\n\tFri, 3 Jun 2022 11:49:34 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E93D730;\n\tFri, 3 Jun 2022 11:49:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654249775;\n\tbh=AlDKQGO9wtL57VuCyaBcS4esDrhFf+Y/sA5V8q6E4a0=;\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=csjSYCXh1M5yS6abCl91yxHqxdLln+BWUeoEKX6A9dsvMvrsLrw3UUWzzmOeQ44Jl\n\t9RJN2kP64XVEUTuIxzPikZ47fJIGyzQ4xDNRx3+A7YbrvkKuFS2rho3NZykaVSrBS/\n\tMNiuzmvpqLjxpiItVDg3Y6wERaTzaklIjtNrCLiekclvkAnghXy/64oKg8vkX86O6T\n\tIe8TVeg4NfdNHWXwGYCc6U2mhfKSjNB1ORecvr9Jgnmz6ZMJYL/IFx8lcBabsqOxgB\n\tJQZoP+3Sw+jM4InNEMBBtnE2LKVl9Sf3MkhhNea1vl7J4p3QMZs4GLkD49wHI/UIjV\n\tjt31uSURJv3JA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654249774;\n\tbh=AlDKQGO9wtL57VuCyaBcS4esDrhFf+Y/sA5V8q6E4a0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Jx2jXButWAfsOvFV/CO4O68FV5j0+BHP+hqWj/faIoBk6UOLYf7ipdssHQczZKcKV\n\ttTA7gXELdVzCOoSnoNkRXVanwRFexXBwjmtS7FS+wCjizxgGjYt6RekV9VsJrssQBO\n\tzF5+XbXjUuFuRriDiRJmByqTS9tru/PmtllHvJZE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Jx2jXBut\"; dkim-atps=neutral","Date":"Fri, 3 Jun 2022 12:49:30 +0300","To":"Christian Rauch ","Message-ID":"","References":"<20220601231802.16735-1-Rauch.Christian@gmx.de>\n\t<20220601231802.16735-2-Rauch.Christian@gmx.de>\n\t\n\t<474c1407-ecc0-9af4-d203-4a38efd8868f@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<474c1407-ecc0-9af4-d203-4a38efd8868f@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: controls: Use\n\tstd::optional to handle invalid control values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]