[{"id":32361,"web_url":"https://patchwork.libcamera.org/comment/32361/","msgid":"<173253702521.1143262.11871580303620865234@ping.linuxembedded.co.uk>","date":"2024-11-25T12:17:05","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2024-11-25 00:08:37)\n> A copy is made in the range-based for loop, and thus `setStream()`\n> operates on this copy, leading to the `stream_` member of the given\n> `StreamConfiguration` object in `*config` never being set to `nullptr`.\n> \n> Fix that by taking a reference in the range-based for loop.\n> \n> Also rename the variable from `it` since it is not an iterator.\n> \n> Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n\nOuch - I would not have spotted that!\n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/libcamera/camera.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 25135d46..82a5186a 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> if (ret < 0)\n> return ret;\n> \n> - for (auto it : *config)\n> - it.setStream(nullptr);\n> + for (auto &cfg : *config)\n> + cfg.setStream(nullptr);\n> \n> if (config->validate() != CameraConfiguration::Valid) {\n> LOG(Camera, Error)\n> -- \n> 2.47.0\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 2DF50BD160\n\tfor ;\n\tMon, 25 Nov 2024 12:17:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FA9666027;\n\tMon, 25 Nov 2024 13:17:10 +0100 (CET)","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 694EE6600E\n\tfor ;\n\tMon, 25 Nov 2024 13:17:08 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B827EA2F;\n\tMon, 25 Nov 2024 13:16:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tdLxDWiG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732537006;\n\tbh=j3Uj8W5drVi5PtN8pEis/SJvLGLwh2p7qaU4H2yUbEk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=tdLxDWiGy0dz3TezT+oZCOHoxokD2Y8+yyG74dWaxr0cbjo7gx1nBMR4sev+g5Dsu\n\tHzhJU8kDJBSrJyu8tQP2eRXVGwPwhEjFG+ueoqE3YDfIldtIwNhaXlgIM2Dn6pyjES\n\t3+Kh1F9EybS/bVYMGIT2XDrTCcrcdZr3NMGHaJZ4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241125000834.396815-1-pobrn@protonmail.com>","References":"<20241125000834.396815-1-pobrn@protonmail.com>","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 25 Nov 2024 12:17:05 +0000","Message-ID":"<173253702521.1143262.11871580303620865234@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32375,"web_url":"https://patchwork.libcamera.org/comment/32375/","msgid":"<20241125224822.GX19381@pendragon.ideasonboard.com>","date":"2024-11-25T22:48:22","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:\n> A copy is made in the range-based for loop, and thus `setStream()`\n> operates on this copy, leading to the `stream_` member of the given\n> `StreamConfiguration` object in `*config` never being set to `nullptr`.\n> \n> Fix that by taking a reference in the range-based for loop.\n> \n> Also rename the variable from `it` since it is not an iterator.\n> \n> Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n\nMissing SoB line.\n\n> ---\n> src/libcamera/camera.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 25135d46..82a5186a 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tfor (auto it : *config)\n> -\t\tit.setStream(nullptr);\n> +\tfor (auto &cfg : *config)\n> +\t\tcfg.setStream(nullptr);\n\nThat's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.\nOr worse, that it won't tell us in the future if this occurs again.\n\nIt turns out we can do better by marking the copy constructor explicit:\n\ndiff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\nindex 071b71698acb..96b1ef36702e 100644\n--- a/include/libcamera/stream.h\n+++ b/include/libcamera/stream.h\n@@ -41,6 +41,12 @@ struct StreamConfiguration {\n \tStreamConfiguration();\n \tStreamConfiguration(const StreamFormats &formats);\n\n+\texplicit StreamConfiguration(const StreamConfiguration &cfg) = default;\n+\tStreamConfiguration(StreamConfiguration &&cfg) = default;\n+\n+\tStreamConfiguration &operator=(const StreamConfiguration &cfg) = default;\n+\tStreamConfiguration &operator=(StreamConfiguration &&cfg) = default;\n+\n \tPixelFormat pixelFormat;\n \tSize size;\n \tunsigned int stride;\ndiff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\nindex 1f75dbbc5b64..e34a112e7b01 100644\n--- a/src/libcamera/stream.cpp\n+++ b/src/libcamera/stream.cpp\n@@ -294,6 +294,34 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n {\n }\n\n+/**\n+ * \\fn StreamConfiguration::StreamConfiguration(const StreamConfiguration &other)\n+ * \\brief Copy constructor, create a StreamConfiguration from a copy of \\a other\n+ * \\param[in] other The other StreamConfiguration\n+ */\n+\n+/**\n+ * \\fn StreamConfiguration::StreamConfiguration(StreamConfiguration &&other)\n+ * \\brief Move constructor, create a StreamConfiguration by taking over \\a other\n+ * \\param[in] other The other StreamConfiguration\n+ */\n+\n+/**\n+ * \\fn StreamConfiguration::operator=(const StreamConfiguration &other)\n+ * \\brief Copy assignment operator, replace the StreamConfiguration with a copy\n+ * of \\a other\n+ * \\param[in] other The other StreamConfiguration\n+ * \\return This StreamConfiguration\n+ */\n+\n+/**\n+ * \\fn StreamConfiguration::operator=(StreamConfiguration &&other)\n+ * \\brief Move assignment operator, replace the StreamConfiguration by taking\n+ * over \\a other\n+ * \\param[in] other The other StreamConfiguration\n+ * \\return This StreamConfiguration\n+ */\n+\n /**\n * \\var StreamConfiguration::size\n * \\brief Stream size in pixels\n\n(We could possibly guard the functions with #ifndef __DOXYGEN__ in the\nheader file instead of documenting them.)\n\nWe will then need to make use of the explicit copy constructor or the\ncompiler won't be happy:\n\ndiff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\nindex 1d68540d7e50..83b7249b88e5 100644\n--- a/src/android/camera_stream.cpp\n+++ b/src/android/camera_stream.cpp\n@@ -87,7 +87,7 @@ int CameraStream::configure()\n \tif (type_ == Type::Internal || type_ == Type::Mapped) {\n \t\tconst PixelFormat outFormat =\n \t\t\tcameraDevice_->capabilities()->toPixelFormat(camera3Stream_->format);\n-\t\tStreamConfiguration output = configuration();\n+\t\tStreamConfiguration output{ configuration() };\n \t\toutput.pixelFormat = outFormat;\n \t\toutput.size.width = camera3Stream_->width;\n \t\toutput.size.height = camera3Stream_->height;\ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex 0069d5e2558f..7ef18b00aee8 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -280,7 +280,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n \tbool mainOutputAvailable = true;\n \tfor (unsigned int i = 0; i < config_.size(); ++i) {\n \t\tconst PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);\n-\t\tconst StreamConfiguration originalCfg = config_[i];\n+\t\tconst StreamConfiguration originalCfg{ config_[i] };\n \t\tStreamConfiguration *cfg = &config_[i];\n\n \t\tLOG(IPU3, Debug) << \"Validating stream: \" << config_[i].toString();\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\nindex 6c6d711f91b3..6935018bec13 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n@@ -562,7 +562,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n\n \t\t/* Try to match stream without adjusting configuration. */\n \t\tif (mainPathAvailable) {\n-\t\t\tStreamConfiguration tryCfg = cfg;\n+\t\t\tStreamConfiguration tryCfg{ cfg };\n \t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n \t\t\t\tmainPathAvailable = false;\n \t\t\t\tcfg = tryCfg;\n@@ -572,7 +572,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n \t\t}\n\n \t\tif (selfPathAvailable) {\n-\t\t\tStreamConfiguration tryCfg = cfg;\n+\t\t\tStreamConfiguration tryCfg{ cfg };\n \t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n \t\t\t\tselfPathAvailable = false;\n \t\t\t\tcfg = tryCfg;\n@@ -583,7 +583,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n\n \t\t/* Try to match stream allowing adjusting configuration. */\n \t\tif (mainPathAvailable) {\n-\t\t\tStreamConfiguration tryCfg = cfg;\n+\t\t\tStreamConfiguration tryCfg{ cfg };\n \t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n \t\t\t\tmainPathAvailable = false;\n \t\t\t\tcfg = tryCfg;\n@@ -594,7 +594,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n \t\t}\n\n \t\tif (selfPathAvailable) {\n-\t\t\tStreamConfiguration tryCfg = cfg;\n+\t\t\tStreamConfiguration tryCfg{ cfg };\n \t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n \t\t\t\tselfPathAvailable = false;\n \t\t\t\tcfg = tryCfg;\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\nindex 236d05af7a2f..120e3a0bc263 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n@@ -259,7 +259,7 @@ RkISP1Path::validate(const CameraSensor *sensor,\n \tconst std::vector &mbusCodes = sensor->mbusCodes();\n \tSize resolution = filterSensorResolution(sensor);\n\n-\tconst StreamConfiguration reqCfg = *cfg;\n+\tconst StreamConfiguration reqCfg{ *cfg };\n \tCameraConfiguration::Status status = CameraConfiguration::Valid;\n\n \t/*\n\nNo functional change so far. Then the compiler will complain about the\ncode fixed by your patch:\n\ndiff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\nindex 7507e9ddae77..4c865a46af53 100644\n--- a/src/libcamera/camera.cpp\n+++ b/src/libcamera/camera.cpp\n@@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n \tif (ret < 0)\n \t\treturn ret;\n\n-\tfor (auto it : *config)\n-\t\tit.setStream(nullptr);\n+\tfor (auto &cfg : *config)\n+\t\tcfg.setStream(nullptr);\n\n \tif (config->validate() != CameraConfiguration::Valid) {\n \t\tLOG(Camera, Error)\n\nBut... surprise surprise, it complains somewhere else !\n\ndiff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\nindex 9e2d9d23c527..6f278b29331a 100644\n--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n@@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n \tStatus status = Valid;\n \tyuvColorSpace_.reset();\n\n-\tfor (auto cfg : config_) {\n+\tfor (auto &cfg : config_) {\n \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n@@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n \trgbColorSpace_->range = ColorSpace::Range::Full;\n\n \t/* Go through the streams again and force everyone to the same colour space. */\n-\tfor (auto cfg : config_) {\n+\tfor (auto &cfg : config_) {\n \t\tif (cfg.colorSpace == ColorSpace::Raw)\n \t\t\tcontinue;\n\nWould you be able to respin this in a small series? I'd start invoking\ncopy constructor explicitly through the code, then fixing the bugs, and\nfinally marking the copy constructor as explicit in a third patch.\n\n> \n> \tif (config->validate() != CameraConfiguration::Valid) {\n> \t\tLOG(Camera, Error)","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 536B4BD160\n\tfor ;\n\tMon, 25 Nov 2024 22:48:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A2EE6604A;\n\tMon, 25 Nov 2024 23:48:34 +0100 (CET)","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 C62D665EF7\n\tfor ;\n\tMon, 25 Nov 2024 23:48:32 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CC826B5;\n\tMon, 25 Nov 2024 23:48:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GwUjQloZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732574890;\n\tbh=U1Bme5lK1PND6nX187wdJ7rjrsyJiBs6YE1LssfncXI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GwUjQloZG9cKbTJ7jfDH9GV40hfZOL4xOoUzLieKoVwkbvoKP0APe14j5G1JW7WKy\n\tfG9p76s0/OTRuAF0oxpQdiowNjOZ6AgCnDH27FJiHz6Hz/59qBOBYxhoM7+ZDG80Yo\n\tptroSduYdMHwyKHnu+DPSLjEolG6rDObhufseaPg=","Date":"Tue, 26 Nov 2024 00:48:22 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","Message-ID":"<20241125224822.GX19381@pendragon.ideasonboard.com>","References":"<20241125000834.396815-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241125000834.396815-1-pobrn@protonmail.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32380,"web_url":"https://patchwork.libcamera.org/comment/32380/","msgid":"","date":"2024-11-26T02:03:09","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:\n\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:\n> > A copy is made in the range-based for loop, and thus `setStream()`\n> > operates on this copy, leading to the `stream_` member of the given\n> > `StreamConfiguration` object in `*config` never being set to `nullptr`.\n> >\n> > Fix that by taking a reference in the range-based for loop.\n> >\n> > Also rename the variable from `it` since it is not an iterator.\n> >\n> > Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> \n> Missing SoB line.\n> \n> > ---\n> > src/libcamera/camera.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 25135d46..82a5186a 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> >\n> > -\tfor (auto it : *config)\n> > -\t\tit.setStream(nullptr);\n> > +\tfor (auto &cfg : *config)\n> > +\t\tcfg.setStream(nullptr);\n> \n> That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.\n> Or worse, that it won't tell us in the future if this occurs again.\n> \n> It turns out we can do better by marking the copy constructor explicit:\n\nI am not a fan of that for two reasons:\n - I like using `=` :-)\n - it is limited to just this type\n\n`clang-tidy` has a check for a very close scenario:\nhttps://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html\n\nI believe integrating that into the CI or something would be a more general solution.\n\nExecuting\n\n clang-tidy \\\n -p ./build/compile_commands.json \\\n --config=\"{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }\" \\\n $(find -type f -name '*.cpp')\n\nreveals:\n\n./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n 328 | for (auto alias : factory->compatibles())\n | ^\n | const &\n./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n 108 | for (auto cfg : config_) {\n | ^\n | const &\n./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n 133 | for (auto cfg : config_) {\n | ^\n | const &\n./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]\n 42 | for (std::filesystem::path path : imageFrames.files) {\n | ^\n | const &\n./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n 60 | for (auto name : cameraNames) {\n | ^\n | const &\n\nApart from the two you mention below, they all seem to be performance\nissues that do not affect correctness.\n\nI have just discovered that there is already a .clang-tidy file, and that meson\nautomatically generates a clang-tidy target if it is installed and a .clang-tidy\nfile is found, so I am suggesting the following:\n\ndiff --git a/.clang-tidy b/.clang-tidy\nindex 8056d7a8..6ef418d9 100644\n--- a/.clang-tidy\n+++ b/.clang-tidy\n@@ -1,4 +1,10 @@\n # SPDX-License-Identifier: CC0-1.0\n \n-Checks: -clang-diagnostic-c99-designator\n+Checks:\n+ - '-clang-diagnostic-c99-designator'\n+ - 'performance-*'\n+CheckOptions:\n+ - key: 'performance-for-range-copy.WarnOnAllAutoCopies'\n+ value: true\n+WarningsAsErrors: 'performance-for-range-copy'\n FormatStyle: file\n\nand adding `ninja -C build clang-tidy` to an appropriate CI job.\n\nAdmittedly, there are cases that this won't report, but I think\nthis is worthwhile nonetheless.\n\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 071b71698acb..96b1ef36702e 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -41,6 +41,12 @@ struct StreamConfiguration {\n> \tStreamConfiguration();\n> \tStreamConfiguration(const StreamFormats &formats);\n> \n> +\texplicit StreamConfiguration(const StreamConfiguration &cfg) = default;\n> +\tStreamConfiguration(StreamConfiguration &&cfg) = default;\n> +\n> +\tStreamConfiguration &operator=(const StreamConfiguration &cfg) = default;\n> +\tStreamConfiguration &operator=(StreamConfiguration &&cfg) = default;\n> +\n> \tPixelFormat pixelFormat;\n> \tSize size;\n> \tunsigned int stride;\n> [...]\n> No functional change so far. Then the compiler will complain about the\n> code fixed by your patch:\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 7507e9ddae77..4c865a46af53 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tfor (auto it : *config)\n> -\t\tit.setStream(nullptr);\n> +\tfor (auto &cfg : *config)\n> +\t\tcfg.setStream(nullptr);\n> \n> \tif (config->validate() != CameraConfiguration::Valid) {\n> \t\tLOG(Camera, Error)\n> \n> But... surprise surprise, it complains somewhere else !\n\nAhh... I should've looked further.\n\n\n> \n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 9e2d9d23c527..6f278b29331a 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> \tStatus status = Valid;\n> \tyuvColorSpace_.reset();\n> \n> -\tfor (auto cfg : config_) {\n> +\tfor (auto &cfg : config_) {\n> \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> \trgbColorSpace_->range = ColorSpace::Range::Full;\n> \n> \t/* Go through the streams again and force everyone to the same colour space. */\n> -\tfor (auto cfg : config_) {\n> +\tfor (auto &cfg : config_) {\n> \t\tif (cfg.colorSpace == ColorSpace::Raw)\n> \t\t\tcontinue;\n> \n> Would you be able to respin this in a small series? I'd start invoking\n> copy constructor explicitly through the code, then fixing the bugs, and\n> finally marking the copy constructor as explicit in a third patch.\n> \n> >\n> > \tif (config->validate() != CameraConfiguration::Valid) {\n> > \t\tLOG(Camera, Error)\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\n> \n\n\nRegards,\nBarnabás Pőcze","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 61B5BBD16B\n\tfor ;\n\tTue, 26 Nov 2024 02:03:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69DA666060;\n\tTue, 26 Nov 2024 03:03:18 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D7D165FF5\n\tfor ;\n\tTue, 26 Nov 2024 03:03:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"mQbl35dj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1732586595; x=1732845795;\n\tbh=7mmWSGukOcmY9IAp1EOKbblK0ZGRdl/PeJA+HIS9Rn4=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=mQbl35djlAG9bwdF3kHdkU1+j3YNKPLhmSv2ai/APQxUFhjLLa2K9x3DdFoyp1630\n\tafWvrlFBEnudHHcZ0K7BTtf9bYVEVI7Ac6gJfQzXgKj2HC/yb3iFWululiTvKPGCi9\n\twPJqKQkOQWX2j4b3wiXXW5/TUXUiYDJwdH5QHw7CWFdTPlSOhJNmxCJrIE8ugcCs1X\n\tyoVZdl7TKRdqX1Fna20gcyNQVDPESTjMsjgrGMPSrg48SMnQdrhRGA/m7z2VsbIGYn\n\t5lYWQ50ji46n6yknNEI4s6nvZP9dGb83XxHLAqDh0ScqKnQMpNgYbul4L3DjCZpPLm\n\th6lXtczQV7bRw==","Date":"Tue, 26 Nov 2024 02:03:09 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","Message-ID":"","In-Reply-To":"<20241125224822.GX19381@pendragon.ideasonboard.com>","References":"<20241125000834.396815-1-pobrn@protonmail.com>\n\t<20241125224822.GX19381@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"6a64e168962eccc0693ce3c645bb7975fc42e574","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32383,"web_url":"https://patchwork.libcamera.org/comment/32383/","msgid":"<20241126120846.GD5461@pendragon.ideasonboard.com>","date":"2024-11-26T12:08:46","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:\n> 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:\n> > > A copy is made in the range-based for loop, and thus `setStream()`\n> > > operates on this copy, leading to the `stream_` member of the given\n> > > `StreamConfiguration` object in `*config` never being set to `nullptr`.\n> > >\n> > > Fix that by taking a reference in the range-based for loop.\n> > >\n> > > Also rename the variable from `it` since it is not an iterator.\n> > >\n> > > Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> > \n> > Missing SoB line.\n> > \n> > > ---\n> > > src/libcamera/camera.cpp | 4 ++--\n> > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 25135d46..82a5186a 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > >\n> > > -\tfor (auto it : *config)\n> > > -\t\tit.setStream(nullptr);\n> > > +\tfor (auto &cfg : *config)\n> > > +\t\tcfg.setStream(nullptr);\n> > \n> > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.\n> > Or worse, that it won't tell us in the future if this occurs again.\n> > \n> > It turns out we can do better by marking the copy constructor explicit:\n> \n> I am not a fan of that for two reasons:\n> - I like using `=` :-)\n\nI've actually grown fond of the\n\n\tClass foo{ other };\n\nsyntax, which I didn't like in the first place, as it make it clear that\nwe're invoking the copy constructor. Writing\n\n\tClass foo = other;\n\nlooks like a default construction followed by a copy assignment. The\ncompiler optimizes that, but it's still not explicit.\n\n> - it is limited to just this type\n\nYes, I thought about that too, and was thinking about going through\nother classes to see if we should mark their copy constructor explicit.\nThat however won't cover all classes that can be iterated over, as shown\nin the clang-tidy report below. Using clang-tidy is possibly a better\noption.\n\n> `clang-tidy` has a check for a very close scenario:\n> https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html\n> \n> I believe integrating that into the CI or something would be a more general solution.\n> \n> Executing\n> \n> clang-tidy \\\n> -p ./build/compile_commands.json \\\n> --config=\"{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }\" \\\n> $(find -type f -name '*.cpp')\n> \n> reveals:\n> \n> ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> 328 | for (auto alias : factory->compatibles())\n> | ^\n> | const &\n> ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> 108 | for (auto cfg : config_) {\n> | ^\n> | const &\n> ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> 133 | for (auto cfg : config_) {\n> | ^\n> | const &\n> ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]\n> 42 | for (std::filesystem::path path : imageFrames.files) {\n> | ^\n> | const &\n> ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> 60 | for (auto name : cameraNames) {\n> | ^\n> | const &\n\nWould you send a patch series to fix all of those ?\n\n> Apart from the two you mention below, they all seem to be performance\n> issues that do not affect correctness.\n> \n> I have just discovered that there is already a .clang-tidy file, and that meson\n> automatically generates a clang-tidy target if it is installed and a .clang-tidy\n> file is found, so I am suggesting the following:\n> \n> diff --git a/.clang-tidy b/.clang-tidy\n> index 8056d7a8..6ef418d9 100644\n> --- a/.clang-tidy\n> +++ b/.clang-tidy\n> @@ -1,4 +1,10 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> -Checks: -clang-diagnostic-c99-designator\n> +Checks:\n> + - '-clang-diagnostic-c99-designator'\n> + - 'performance-*'\n> +CheckOptions:\n> + - key: 'performance-for-range-copy.WarnOnAllAutoCopies'\n> + value: true\n> +WarningsAsErrors: 'performance-for-range-copy'\n> FormatStyle: file\n> \n> and adding `ninja -C build clang-tidy` to an appropriate CI job.\n> \n> Admittedly, there are cases that this won't report, but I think\n> this is worthwhile nonetheless.\n\nIdeally that should be integrated in checkstyle.py.\n\nHow do we handle false positives in CI if we use clang-tidy ? We strive\nfor a warning-less build, and CI checks that output known warnings will\njust be ignored. Keeping a database of known warnings may be one option,\nbut I'd like to avoid that if possible (partly because I'm not sure how\nwe could implement it).\n\n> > \n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index 071b71698acb..96b1ef36702e 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -41,6 +41,12 @@ struct StreamConfiguration {\n> > \tStreamConfiguration();\n> > \tStreamConfiguration(const StreamFormats &formats);\n> > \n> > +\texplicit StreamConfiguration(const StreamConfiguration &cfg) = default;\n> > +\tStreamConfiguration(StreamConfiguration &&cfg) = default;\n> > +\n> > +\tStreamConfiguration &operator=(const StreamConfiguration &cfg) = default;\n> > +\tStreamConfiguration &operator=(StreamConfiguration &&cfg) = default;\n> > +\n> > \tPixelFormat pixelFormat;\n> > \tSize size;\n> > \tunsigned int stride;\n> > [...]\n> > No functional change so far. Then the compiler will complain about the\n> > code fixed by your patch:\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 7507e9ddae77..4c865a46af53 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > -\tfor (auto it : *config)\n> > -\t\tit.setStream(nullptr);\n> > +\tfor (auto &cfg : *config)\n> > +\t\tcfg.setStream(nullptr);\n> > \n> > \tif (config->validate() != CameraConfiguration::Valid) {\n> > \t\tLOG(Camera, Error)\n> > \n> > But... surprise surprise, it complains somewhere else !\n> \n> Ahh... I should've looked further.\n> \n> \n> > \n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 9e2d9d23c527..6f278b29331a 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > \tStatus status = Valid;\n> > \tyuvColorSpace_.reset();\n> > \n> > -\tfor (auto cfg : config_) {\n> > +\tfor (auto &cfg : config_) {\n> > \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> > \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> > \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > \trgbColorSpace_->range = ColorSpace::Range::Full;\n> > \n> > \t/* Go through the streams again and force everyone to the same colour space. */\n> > -\tfor (auto cfg : config_) {\n> > +\tfor (auto &cfg : config_) {\n> > \t\tif (cfg.colorSpace == ColorSpace::Raw)\n> > \t\t\tcontinue;\n> > \n> > Would you be able to respin this in a small series? I'd start invoking\n> > copy constructor explicitly through the code, then fixing the bugs, and\n> > finally marking the copy constructor as explicit in a third patch.\n> > \n> > >\n> > > \tif (config->validate() != CameraConfiguration::Valid) {\n> > > \t\tLOG(Camera, Error)","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 6472ABD16B\n\tfor ;\n\tTue, 26 Nov 2024 12:08:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76CB965FD0;\n\tTue, 26 Nov 2024 13:08:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB4DF65FA0\n\tfor ;\n\tTue, 26 Nov 2024 13:08:55 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7014C526;\n\tTue, 26 Nov 2024 13:08:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KKKBxfGn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732622913;\n\tbh=XVW04eiUTROquAERz6yF3eZRQcSJitPBwa24wkndn9o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KKKBxfGnD59OSyszZwbzNoal5fZM7rxUj1Obdu/cS/fXQAvzMezR6UT3CikMypNDb\n\tLxhBTEjdvuLINiHR1emAqpE/v6cuTdZ6O9GPma2UtYgmezle/qyouvYRfEOVUHaFLj\n\tKkf525CHeK/UUKtKaOIZUERxR5scgNo7NVd+okMc=","Date":"Tue, 26 Nov 2024 14:08:46 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","Message-ID":"<20241126120846.GD5461@pendragon.ideasonboard.com>","References":"<20241125000834.396815-1-pobrn@protonmail.com>\n\t<20241125224822.GX19381@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32384,"web_url":"https://patchwork.libcamera.org/comment/32384/","msgid":"","date":"2024-11-26T12:19:49","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"On 26.11.24 13:08, Laurent Pinchart wrote:\n> Ideally that should be integrated in checkstyle.py.\n>\n> How do we handle false positives in CI if we use clang-tidy ? We strive\n> for a warning-less build, and CI checks that output known warnings will\n> just be ignored. Keeping a database of known warnings may be one option,\n> but I'd like to avoid that if possible (partly because I'm not sure how\n> we could implement it).\n\nApparently there are multiple NOLINT inline comments that one can use, \nsee \nhttps://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics","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 E2702BD16B\n\tfor ;\n\tTue, 26 Nov 2024 12:19:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E63266065;\n\tTue, 26 Nov 2024 13:19:57 +0100 (CET)","from sender4-op-o12.zoho.com (sender4-op-o12.zoho.com\n\t[136.143.188.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30FDD65FA0\n\tfor ;\n\tTue, 26 Nov 2024 13:19:56 +0100 (CET)","by mx.zohomail.com with SMTPS id 1732623591588219.10167424376164; \n\tTue, 26 Nov 2024 04:19:51 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"jhtL/ri/\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1732623593; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=ROnknPjWYVbwVABW58LtmkOmLRSCf39kbs3V5DZJ8kiRFQ2fiEYc/cLzAts1UwD24dYAbF9kDjarhvLBBAhkg8Q/TJC+jrOKXAaKXlVPRxt55/9VRe9OntRVHqbPFROp6fIgK+COEL6R8GZW261jBW813gibWg0ksU3RiTdAZCQ=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1732623593;\n\th=Content-Type:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=pSKHdG+G06NGBrv3vU61kCBX6W35PhV7e/MRDUBubKE=; \n\tb=AN7mSf/tmi8zN0lwkGIwP5RWHzQlKeikoy13LpyH4efxVGnbZEec/duzEwU7dSEeeLWVtxWIha79ZD9iNaYG2yuQaDvEbPmki7qn9ySa7H4TLs3I9c/c9Kq8n0b6dm5wMo9Z7gQKEGMPyKDpm8UTZhmiiNHNGbReL757bA5zx0E=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1732623593;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Content-Type:Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Message-Id:Reply-To:Cc;\n\tbh=pSKHdG+G06NGBrv3vU61kCBX6W35PhV7e/MRDUBubKE=;\n\tb=jhtL/ri/Y5d6OWlYzmMI+CRQ/AikbOpvUsfwq6u2VL3RBdZcFIeiLzkVHXkz5ETd\n\tf16JFKKAcN7P7xcr98kvcrSC9Wwzh2FmKIQVWB8e9mSVVM6nX4AaCw0uyujcYY9jVH0\n\t4PLA8QMYa+s5bl2jIvLGecFWXa8KDKWOryN7VNo8=","Content-Type":"multipart/alternative;\n\tboundary=\"------------N3GbNQq16RlOGqrCdq4Q4eRi\"","Message-ID":"","Date":"Tue, 26 Nov 2024 13:19:49 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","To":"libcamera-devel@lists.libcamera.org","References":"<20241125000834.396815-1-pobrn@protonmail.com>\n\t<20241125224822.GX19381@pendragon.ideasonboard.com>\n\t\n\t<20241126120846.GD5461@pendragon.ideasonboard.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<20241126120846.GD5461@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32387,"web_url":"https://patchwork.libcamera.org/comment/32387/","msgid":"<20241126155518.GB5493@pendragon.ideasonboard.com>","date":"2024-11-26T15:55:18","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Nov 26, 2024 at 02:08:46PM +0200, Laurent Pinchart wrote:\n> On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:\n> > 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:\n> > > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:\n> > > > A copy is made in the range-based for loop, and thus `setStream()`\n> > > > operates on this copy, leading to the `stream_` member of the given\n> > > > `StreamConfiguration` object in `*config` never being set to `nullptr`.\n> > > >\n> > > > Fix that by taking a reference in the range-based for loop.\n> > > >\n> > > > Also rename the variable from `it` since it is not an iterator.\n> > > >\n> > > > Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> > > \n> > > Missing SoB line.\n> > > \n> > > > ---\n> > > > src/libcamera/camera.cpp | 4 ++--\n> > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 25135d46..82a5186a 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > > > \tif (ret < 0)\n> > > > \t\treturn ret;\n> > > >\n> > > > -\tfor (auto it : *config)\n> > > > -\t\tit.setStream(nullptr);\n> > > > +\tfor (auto &cfg : *config)\n> > > > +\t\tcfg.setStream(nullptr);\n> > > \n> > > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.\n> > > Or worse, that it won't tell us in the future if this occurs again.\n> > > \n> > > It turns out we can do better by marking the copy constructor explicit:\n> > \n> > I am not a fan of that for two reasons:\n> > - I like using `=` :-)\n> \n> I've actually grown fond of the\n> \n> \tClass foo{ other };\n> \n> syntax, which I didn't like in the first place, as it make it clear that\n> we're invoking the copy constructor. Writing\n> \n> \tClass foo = other;\n> \n> looks like a default construction followed by a copy assignment. The\n> compiler optimizes that, but it's still not explicit.\n> \n> > - it is limited to just this type\n> \n> Yes, I thought about that too, and was thinking about going through\n> other classes to see if we should mark their copy constructor explicit.\n> That however won't cover all classes that can be iterated over, as shown\n> in the clang-tidy report below. Using clang-tidy is possibly a better\n> option.\n> \n> > `clang-tidy` has a check for a very close scenario:\n> > https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html\n> > \n> > I believe integrating that into the CI or something would be a more general solution.\n> > \n> > Executing\n> > \n> > clang-tidy \\\n> > -p ./build/compile_commands.json \\\n> > --config=\"{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }\" \\\n> > $(find -type f -name '*.cpp')\n> > \n> > reveals:\n> > \n> > ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > 328 | for (auto alias : factory->compatibles())\n> > | ^\n> > | const &\n> > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > 108 | for (auto cfg : config_) {\n> > | ^\n> > | const &\n> > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > 133 | for (auto cfg : config_) {\n> > | ^\n> > | const &\n> > ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]\n> > 42 | for (std::filesystem::path path : imageFrames.files) {\n> > | ^\n> > | const &\n> > ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > 60 | for (auto name : cameraNames) {\n> > | ^\n> > | const &\n> \n> Would you send a patch series to fix all of those ?\n> \n> > Apart from the two you mention below, they all seem to be performance\n> > issues that do not affect correctness.\n> > \n> > I have just discovered that there is already a .clang-tidy file, and that meson\n> > automatically generates a clang-tidy target if it is installed and a .clang-tidy\n> > file is found, so I am suggesting the following:\n> > \n> > diff --git a/.clang-tidy b/.clang-tidy\n> > index 8056d7a8..6ef418d9 100644\n> > --- a/.clang-tidy\n> > +++ b/.clang-tidy\n> > @@ -1,4 +1,10 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> > \n> > -Checks: -clang-diagnostic-c99-designator\n> > +Checks:\n> > + - '-clang-diagnostic-c99-designator'\n> > + - 'performance-*'\n> > +CheckOptions:\n> > + - key: 'performance-for-range-copy.WarnOnAllAutoCopies'\n> > + value: true\n> > +WarningsAsErrors: 'performance-for-range-copy'\n> > FormatStyle: file\n> > \n> > and adding `ninja -C build clang-tidy` to an appropriate CI job.\n> > \n> > Admittedly, there are cases that this won't report, but I think\n> > this is worthwhile nonetheless.\n\nI had a look at this, and it's *very* noisy. We could suppress some\nwarnings to make it better, and fix other issues, but we also import\nfiles such as kernel headers or Android sources verbatim, and clang-tidy\nreports lots of issues there. I haven't yet found a way to exclude\nfiles.\n\n> Ideally that should be integrated in checkstyle.py.\n> \n> How do we handle false positives in CI if we use clang-tidy ? We strive\n> for a warning-less build, and CI checks that output known warnings will\n> just be ignored. Keeping a database of known warnings may be one option,\n> but I'd like to avoid that if possible (partly because I'm not sure how\n> we could implement it).\n> \n> > > \n> > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > > index 071b71698acb..96b1ef36702e 100644\n> > > --- a/include/libcamera/stream.h\n> > > +++ b/include/libcamera/stream.h\n> > > @@ -41,6 +41,12 @@ struct StreamConfiguration {\n> > > \tStreamConfiguration();\n> > > \tStreamConfiguration(const StreamFormats &formats);\n> > > \n> > > +\texplicit StreamConfiguration(const StreamConfiguration &cfg) = default;\n> > > +\tStreamConfiguration(StreamConfiguration &&cfg) = default;\n> > > +\n> > > +\tStreamConfiguration &operator=(const StreamConfiguration &cfg) = default;\n> > > +\tStreamConfiguration &operator=(StreamConfiguration &&cfg) = default;\n> > > +\n> > > \tPixelFormat pixelFormat;\n> > > \tSize size;\n> > > \tunsigned int stride;\n> > > [...]\n> > > No functional change so far. Then the compiler will complain about the\n> > > code fixed by your patch:\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 7507e9ddae77..4c865a46af53 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > > \n> > > -\tfor (auto it : *config)\n> > > -\t\tit.setStream(nullptr);\n> > > +\tfor (auto &cfg : *config)\n> > > +\t\tcfg.setStream(nullptr);\n> > > \n> > > \tif (config->validate() != CameraConfiguration::Valid) {\n> > > \t\tLOG(Camera, Error)\n> > > \n> > > But... surprise surprise, it complains somewhere else !\n> > \n> > Ahh... I should've looked further.\n> > \n> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > index 9e2d9d23c527..6f278b29331a 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > \tStatus status = Valid;\n> > > \tyuvColorSpace_.reset();\n> > > \n> > > -\tfor (auto cfg : config_) {\n> > > +\tfor (auto &cfg : config_) {\n> > > \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> > > \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> > > \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> > > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > \trgbColorSpace_->range = ColorSpace::Range::Full;\n> > > \n> > > \t/* Go through the streams again and force everyone to the same colour space. */\n> > > -\tfor (auto cfg : config_) {\n> > > +\tfor (auto &cfg : config_) {\n> > > \t\tif (cfg.colorSpace == ColorSpace::Raw)\n> > > \t\t\tcontinue;\n> > > \n> > > Would you be able to respin this in a small series? I'd start invoking\n> > > copy constructor explicitly through the code, then fixing the bugs, and\n> > > finally marking the copy constructor as explicit in a third patch.\n> > > \n> > > >\n> > > > \tif (config->validate() != CameraConfiguration::Valid) {\n> > > > \t\tLOG(Camera, Error)","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 DE993BD16B\n\tfor ;\n\tTue, 26 Nov 2024 15:55:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6B6665FCD;\n\tTue, 26 Nov 2024 16:55:29 +0100 (CET)","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 D9EEC65F9E\n\tfor ;\n\tTue, 26 Nov 2024 16:55:27 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4801B6BE;\n\tTue, 26 Nov 2024 16:55:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WP516CNj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732636505;\n\tbh=+MpH4WIMCWHpIXqRoF23WDoS6KBxNZdxtxpRmWblJ6E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WP516CNjh/PBHj4L4q2kPn5iJ/VIHRRqsUBeT9G3FM7J5lFBc2BMhu+crX3lSR6OJ\n\t4YKRML8G7D0UpFAwDYyBHE66zWqAeHseuicb3d1jP2XnWatK3qJDEhVdanZDNPxVIc\n\tv5QFZ1rNDe/I9YNYkt5+4ZZdGAOZWxvOXYPQnZiU=","Date":"Tue, 26 Nov 2024 17:55:18 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","Message-ID":"<20241126155518.GB5493@pendragon.ideasonboard.com>","References":"<20241125000834.396815-1-pobrn@protonmail.com>\n\t<20241125224822.GX19381@pendragon.ideasonboard.com>\n\t\n\t<20241126120846.GD5461@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241126120846.GD5461@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32392,"web_url":"https://patchwork.libcamera.org/comment/32392/","msgid":"","date":"2024-11-26T16:14:21","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2024. november 26., kedd 16:55 keltezéssel, Laurent Pinchart írta:\n\n> On Tue, Nov 26, 2024 at 02:08:46PM +0200, Laurent Pinchart wrote:\n> > On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:\n> > > 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:\n> > > > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:\n> > > > > A copy is made in the range-based for loop, and thus `setStream()`\n> > > > > operates on this copy, leading to the `stream_` member of the given\n> > > > > `StreamConfiguration` object in `*config` never being set to `nullptr`.\n> > > > >\n> > > > > Fix that by taking a reference in the range-based for loop.\n> > > > >\n> > > > > Also rename the variable from `it` since it is not an iterator.\n> > > > >\n> > > > > Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> > > >\n> > > > Missing SoB line.\n> > > >\n> > > > > ---\n> > > > > src/libcamera/camera.cpp | 4 ++--\n> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index 25135d46..82a5186a 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > > > > \tif (ret < 0)\n> > > > > \t\treturn ret;\n> > > > >\n> > > > > -\tfor (auto it : *config)\n> > > > > -\t\tit.setStream(nullptr);\n> > > > > +\tfor (auto &cfg : *config)\n> > > > > +\t\tcfg.setStream(nullptr);\n> > > >\n> > > > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.\n> > > > Or worse, that it won't tell us in the future if this occurs again.\n> > > >\n> > > > It turns out we can do better by marking the copy constructor explicit:\n> > >\n> > > I am not a fan of that for two reasons:\n> > > - I like using `=` :-)\n> >\n> > I've actually grown fond of the\n> >\n> > \tClass foo{ other };\n> >\n> > syntax, which I didn't like in the first place, as it make it clear that\n> > we're invoking the copy constructor. Writing\n> >\n> > \tClass foo = other;\n> >\n> > looks like a default construction followed by a copy assignment. The\n> > compiler optimizes that, but it's still not explicit.\n> >\n> > > - it is limited to just this type\n> >\n> > Yes, I thought about that too, and was thinking about going through\n> > other classes to see if we should mark their copy constructor explicit.\n> > That however won't cover all classes that can be iterated over, as shown\n> > in the clang-tidy report below. Using clang-tidy is possibly a better\n> > option.\n> >\n> > > `clang-tidy` has a check for a very close scenario:\n> > > https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html\n> > >\n> > > I believe integrating that into the CI or something would be a more general solution.\n> > >\n> > > Executing\n> > >\n> > > clang-tidy \\\n> > > -p ./build/compile_commands.json \\\n> > > --config=\"{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }\" \\\n> > > $(find -type f -name '*.cpp')\n> > >\n> > > reveals:\n> > >\n> > > ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > 328 | for (auto alias : factory->compatibles())\n> > > | ^\n> > > | const &\n> > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > 108 | for (auto cfg : config_) {\n> > > | ^\n> > > | const &\n> > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > 133 | for (auto cfg : config_) {\n> > > | ^\n> > > | const &\n> > > ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]\n> > > 42 | for (std::filesystem::path path : imageFrames.files) {\n> > > | ^\n> > > | const &\n> > > ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > 60 | for (auto name : cameraNames) {\n> > > | ^\n> > > | const &\n> >\n> > Would you send a patch series to fix all of those ?\n> >\n> > > Apart from the two you mention below, they all seem to be performance\n> > > issues that do not affect correctness.\n> > >\n> > > I have just discovered that there is already a .clang-tidy file, and that meson\n> > > automatically generates a clang-tidy target if it is installed and a .clang-tidy\n> > > file is found, so I am suggesting the following:\n> > >\n> > > diff --git a/.clang-tidy b/.clang-tidy\n> > > index 8056d7a8..6ef418d9 100644\n> > > --- a/.clang-tidy\n> > > +++ b/.clang-tidy\n> > > @@ -1,4 +1,10 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > >\n> > > -Checks: -clang-diagnostic-c99-designator\n> > > +Checks:\n> > > + - '-clang-diagnostic-c99-designator'\n> > > + - 'performance-*'\n> > > +CheckOptions:\n> > > + - key: 'performance-for-range-copy.WarnOnAllAutoCopies'\n> > > + value: true\n> > > +WarningsAsErrors: 'performance-for-range-copy'\n> > > FormatStyle: file\n> > >\n> > > and adding `ninja -C build clang-tidy` to an appropriate CI job.\n> > >\n> > > Admittedly, there are cases that this won't report, but I think\n> > > this is worthwhile nonetheless.\n> \n> I had a look at this, and it's *very* noisy. We could suppress some\n> warnings to make it better, and fix other issues, but we also import\n> files such as kernel headers or Android sources verbatim, and clang-tidy\n> reports lots of issues there. I haven't yet found a way to exclude\n> files.\n\nThere is `ExcludeHeaderFilterRegex` and `HeaderFilterRegex`: https://clang.llvm.org/extra/clang-tidy/index.html\nI believe that should be sufficient to ignore the linux and android headers.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> > Ideally that should be integrated in checkstyle.py.\n> >\n> > How do we handle false positives in CI if we use clang-tidy ? We strive\n> > for a warning-less build, and CI checks that output known warnings will\n> > just be ignored. Keeping a database of known warnings may be one option,\n> > but I'd like to avoid that if possible (partly because I'm not sure how\n> > we could implement it).\n> >\n> > > >\n> > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > > > index 071b71698acb..96b1ef36702e 100644\n> > > > --- a/include/libcamera/stream.h\n> > > > +++ b/include/libcamera/stream.h\n> > > > @@ -41,6 +41,12 @@ struct StreamConfiguration {\n> > > > \tStreamConfiguration();\n> > > > \tStreamConfiguration(const StreamFormats &formats);\n> > > >\n> > > > +\texplicit StreamConfiguration(const StreamConfiguration &cfg) = default;\n> > > > +\tStreamConfiguration(StreamConfiguration &&cfg) = default;\n> > > > +\n> > > > +\tStreamConfiguration &operator=(const StreamConfiguration &cfg) = default;\n> > > > +\tStreamConfiguration &operator=(StreamConfiguration &&cfg) = default;\n> > > > +\n> > > > \tPixelFormat pixelFormat;\n> > > > \tSize size;\n> > > > \tunsigned int stride;\n> > > > [...]\n> > > > No functional change so far. Then the compiler will complain about the\n> > > > code fixed by your patch:\n> > > >\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 7507e9ddae77..4c865a46af53 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > > > \tif (ret < 0)\n> > > > \t\treturn ret;\n> > > >\n> > > > -\tfor (auto it : *config)\n> > > > -\t\tit.setStream(nullptr);\n> > > > +\tfor (auto &cfg : *config)\n> > > > +\t\tcfg.setStream(nullptr);\n> > > >\n> > > > \tif (config->validate() != CameraConfiguration::Valid) {\n> > > > \t\tLOG(Camera, Error)\n> > > >\n> > > > But... surprise surprise, it complains somewhere else !\n> > >\n> > > Ahh... I should've looked further.\n> > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > index 9e2d9d23c527..6f278b29331a 100644\n> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > > \tStatus status = Valid;\n> > > > \tyuvColorSpace_.reset();\n> > > >\n> > > > -\tfor (auto cfg : config_) {\n> > > > +\tfor (auto &cfg : config_) {\n> > > > \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> > > > \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> > > > \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> > > > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > > \trgbColorSpace_->range = ColorSpace::Range::Full;\n> > > >\n> > > > \t/* Go through the streams again and force everyone to the same colour space. */\n> > > > -\tfor (auto cfg : config_) {\n> > > > +\tfor (auto &cfg : config_) {\n> > > > \t\tif (cfg.colorSpace == ColorSpace::Raw)\n> > > > \t\t\tcontinue;\n> > > >\n> > > > Would you be able to respin this in a small series? I'd start invoking\n> > > > copy constructor explicitly through the code, then fixing the bugs, and\n> > > > finally marking the copy constructor as explicit in a third patch.\n> > > >\n> > > > >\n> > > > > \tif (config->validate() != CameraConfiguration::Valid) {\n> > > > > \t\tLOG(Camera, Error)\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\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 E6A59BD16B\n\tfor ;\n\tTue, 26 Nov 2024 16:14:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CA9165FD5;\n\tTue, 26 Nov 2024 17:14:58 +0100 (CET)","from mail-40134.protonmail.ch (mail-40134.protonmail.ch\n\t[185.70.40.134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 69B1665F9E\n\tfor ;\n\tTue, 26 Nov 2024 17:14:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"E3y0oqcF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1732637694; x=1732896894;\n\tbh=rsFxvkyS4OoybUgtwpat/B+0nD21+yX6EC7NuPs1i6A=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=E3y0oqcFkNViLV7TLbyACfm6xgkUgEBqgZdx3lHjWJWcghmVGNQopKW1yXKXyEEy4\n\thBUHNV2gU4oGXi5eS5Ya9NPxGMXm4Tk2Lo4M/UI+0UNyyDgzAVbXjyvg8ctPFIupb9\n\tQvdHN/CX1Uzc/xc2/uKPPCDXkVeHvGcBJPei2clT+RubcZwhltP/4wraaB4eKmyuDd\n\tkE2VRCWva9ckuty50W5vW7vMMsXpYmNz3PJpmq1i/mwSYO0mCySQO65fk6HQWq6vyl\n\t8gqEtelcZe0gALKhnjnVpvQVDnwIHu5zBHn6+zDPVbNMbT2KuG0dFBSZqOvgQyprTl\n\t/yja1+2FauLbA==","Date":"Tue, 26 Nov 2024 16:14:21 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","Message-ID":"","In-Reply-To":"<20241126155518.GB5493@pendragon.ideasonboard.com>","References":"<20241125000834.396815-1-pobrn@protonmail.com>\n\t<20241125224822.GX19381@pendragon.ideasonboard.com>\n\t\n\t<20241126120846.GD5461@pendragon.ideasonboard.com>\n\t<20241126155518.GB5493@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"f72b94b625303555fc27889137cbde0b195350e8","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32393,"web_url":"https://patchwork.libcamera.org/comment/32393/","msgid":"<20241126165852.GI5461@pendragon.ideasonboard.com>","date":"2024-11-26T16:58:52","subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Nov 26, 2024 at 04:14:21PM +0000, Barnabás Pőcze wrote:\n> 2024. november 26., kedd 16:55 keltezéssel, Laurent Pinchart írta:\n> > On Tue, Nov 26, 2024 at 02:08:46PM +0200, Laurent Pinchart wrote:\n> > > On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:\n> > > > 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:\n> > > > > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:\n> > > > > > A copy is made in the range-based for loop, and thus `setStream()`\n> > > > > > operates on this copy, leading to the `stream_` member of the given\n> > > > > > `StreamConfiguration` object in `*config` never being set to `nullptr`.\n> > > > > >\n> > > > > > Fix that by taking a reference in the range-based for loop.\n> > > > > >\n> > > > > > Also rename the variable from `it` since it is not an iterator.\n> > > > > >\n> > > > > > Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> > > > >\n> > > > > Missing SoB line.\n> > > > >\n> > > > > > ---\n> > > > > > src/libcamera/camera.cpp | 4 ++--\n> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > > index 25135d46..82a5186a 100644\n> > > > > > --- a/src/libcamera/camera.cpp\n> > > > > > +++ b/src/libcamera/camera.cpp\n> > > > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > > > > > \tif (ret < 0)\n> > > > > > \t\treturn ret;\n> > > > > >\n> > > > > > -\tfor (auto it : *config)\n> > > > > > -\t\tit.setStream(nullptr);\n> > > > > > +\tfor (auto &cfg : *config)\n> > > > > > +\t\tcfg.setStream(nullptr);\n> > > > >\n> > > > > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.\n> > > > > Or worse, that it won't tell us in the future if this occurs again.\n> > > > >\n> > > > > It turns out we can do better by marking the copy constructor explicit:\n> > > >\n> > > > I am not a fan of that for two reasons:\n> > > > - I like using `=` :-)\n> > >\n> > > I've actually grown fond of the\n> > >\n> > > \tClass foo{ other };\n> > >\n> > > syntax, which I didn't like in the first place, as it make it clear that\n> > > we're invoking the copy constructor. Writing\n> > >\n> > > \tClass foo = other;\n> > >\n> > > looks like a default construction followed by a copy assignment. The\n> > > compiler optimizes that, but it's still not explicit.\n> > >\n> > > > - it is limited to just this type\n> > >\n> > > Yes, I thought about that too, and was thinking about going through\n> > > other classes to see if we should mark their copy constructor explicit.\n> > > That however won't cover all classes that can be iterated over, as shown\n> > > in the clang-tidy report below. Using clang-tidy is possibly a better\n> > > option.\n> > >\n> > > > `clang-tidy` has a check for a very close scenario:\n> > > > https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html\n> > > >\n> > > > I believe integrating that into the CI or something would be a more general solution.\n> > > >\n> > > > Executing\n> > > >\n> > > > clang-tidy \\\n> > > > -p ./build/compile_commands.json \\\n> > > > --config=\"{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }\" \\\n> > > > $(find -type f -name '*.cpp')\n> > > >\n> > > > reveals:\n> > > >\n> > > > ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > > 328 | for (auto alias : factory->compatibles())\n> > > > | ^\n> > > > | const &\n> > > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > > 108 | for (auto cfg : config_) {\n> > > > | ^\n> > > > | const &\n> > > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > > 133 | for (auto cfg : config_) {\n> > > > | ^\n> > > > | const &\n> > > > ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]\n> > > > 42 | for (std::filesystem::path path : imageFrames.files) {\n> > > > | ^\n> > > > | const &\n> > > > ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]\n> > > > 60 | for (auto name : cameraNames) {\n> > > > | ^\n> > > > | const &\n> > >\n> > > Would you send a patch series to fix all of those ?\n> > >\n> > > > Apart from the two you mention below, they all seem to be performance\n> > > > issues that do not affect correctness.\n> > > >\n> > > > I have just discovered that there is already a .clang-tidy file, and that meson\n> > > > automatically generates a clang-tidy target if it is installed and a .clang-tidy\n> > > > file is found, so I am suggesting the following:\n> > > >\n> > > > diff --git a/.clang-tidy b/.clang-tidy\n> > > > index 8056d7a8..6ef418d9 100644\n> > > > --- a/.clang-tidy\n> > > > +++ b/.clang-tidy\n> > > > @@ -1,4 +1,10 @@\n> > > > # SPDX-License-Identifier: CC0-1.0\n> > > >\n> > > > -Checks: -clang-diagnostic-c99-designator\n> > > > +Checks:\n> > > > + - '-clang-diagnostic-c99-designator'\n> > > > + - 'performance-*'\n> > > > +CheckOptions:\n> > > > + - key: 'performance-for-range-copy.WarnOnAllAutoCopies'\n> > > > + value: true\n> > > > +WarningsAsErrors: 'performance-for-range-copy'\n> > > > FormatStyle: file\n> > > >\n> > > > and adding `ninja -C build clang-tidy` to an appropriate CI job.\n> > > >\n> > > > Admittedly, there are cases that this won't report, but I think\n> > > > this is worthwhile nonetheless.\n> > \n> > I had a look at this, and it's *very* noisy. We could suppress some\n> > warnings to make it better, and fix other issues, but we also import\n> > files such as kernel headers or Android sources verbatim, and clang-tidy\n> > reports lots of issues there. I haven't yet found a way to exclude\n> > files.\n> \n> There is `ExcludeHeaderFilterRegex` and `HeaderFilterRegex`: https://clang.llvm.org/extra/clang-tidy/index.html\n> I believe that should be sufficient to ignore the linux and android headers.\n\nThat option was introduced in clang-tidy 19. Debian stable ships clang\n14, and even on Gentoo the latest stable version is 18 :-S\n\nThis doesn't prevent us from fixing issues reporting by clang-tidy, but\nCI integration will be difficult short term. This raises the question of\nwhether we want to be defensive against these code patterns through\nother means in the meantime, such as marking copy constructors as\nexplicit, or if that would be overkill.\n\n> > > Ideally that should be integrated in checkstyle.py.\n> > >\n> > > How do we handle false positives in CI if we use clang-tidy ? We strive\n> > > for a warning-less build, and CI checks that output known warnings will\n> > > just be ignored. Keeping a database of known warnings may be one option,\n> > > but I'd like to avoid that if possible (partly because I'm not sure how\n> > > we could implement it).\n> > >\n> > > > >\n> > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > > > > index 071b71698acb..96b1ef36702e 100644\n> > > > > --- a/include/libcamera/stream.h\n> > > > > +++ b/include/libcamera/stream.h\n> > > > > @@ -41,6 +41,12 @@ struct StreamConfiguration {\n> > > > > \tStreamConfiguration();\n> > > > > \tStreamConfiguration(const StreamFormats &formats);\n> > > > >\n> > > > > +\texplicit StreamConfiguration(const StreamConfiguration &cfg) = default;\n> > > > > +\tStreamConfiguration(StreamConfiguration &&cfg) = default;\n> > > > > +\n> > > > > +\tStreamConfiguration &operator=(const StreamConfiguration &cfg) = default;\n> > > > > +\tStreamConfiguration &operator=(StreamConfiguration &&cfg) = default;\n> > > > > +\n> > > > > \tPixelFormat pixelFormat;\n> > > > > \tSize size;\n> > > > > \tunsigned int stride;\n> > > > > [...]\n> > > > > No functional change so far. Then the compiler will complain about the\n> > > > > code fixed by your patch:\n> > > > >\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index 7507e9ddae77..4c865a46af53 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > > > > \tif (ret < 0)\n> > > > > \t\treturn ret;\n> > > > >\n> > > > > -\tfor (auto it : *config)\n> > > > > -\t\tit.setStream(nullptr);\n> > > > > +\tfor (auto &cfg : *config)\n> > > > > +\t\tcfg.setStream(nullptr);\n> > > > >\n> > > > > \tif (config->validate() != CameraConfiguration::Valid) {\n> > > > > \t\tLOG(Camera, Error)\n> > > > >\n> > > > > But... surprise surprise, it complains somewhere else !\n> > > >\n> > > > Ahh... I should've looked further.\n> > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > index 9e2d9d23c527..6f278b29331a 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > > > \tStatus status = Valid;\n> > > > > \tyuvColorSpace_.reset();\n> > > > >\n> > > > > -\tfor (auto cfg : config_) {\n> > > > > +\tfor (auto &cfg : config_) {\n> > > > > \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> > > > > \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> > > > > \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> > > > > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > > > \trgbColorSpace_->range = ColorSpace::Range::Full;\n> > > > >\n> > > > > \t/* Go through the streams again and force everyone to the same colour space. */\n> > > > > -\tfor (auto cfg : config_) {\n> > > > > +\tfor (auto &cfg : config_) {\n> > > > > \t\tif (cfg.colorSpace == ColorSpace::Raw)\n> > > > > \t\t\tcontinue;\n> > > > >\n> > > > > Would you be able to respin this in a small series? I'd start invoking\n> > > > > copy constructor explicitly through the code, then fixing the bugs, and\n> > > > > finally marking the copy constructor as explicit in a third patch.\n> > > > >\n> > > > > >\n> > > > > > \tif (config->validate() != CameraConfiguration::Valid) {\n> > > > > > \t\tLOG(Camera, Error)","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 013E3C3200\n\tfor ;\n\tTue, 26 Nov 2024 16:59:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47D8865FD5;\n\tTue, 26 Nov 2024 17:59:03 +0100 (CET)","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 1BB9665FA0\n\tfor ;\n\tTue, 26 Nov 2024 17:59:02 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6EEB9526;\n\tTue, 26 Nov 2024 17:58:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FRgEwIeF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732640319;\n\tbh=RJRhEp0e2GZ0M2O77qni3W8l5MV6Ey58Ypem974Jqs0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FRgEwIeFMik7QsLHNwitIQCwdm5/30n8ELQnykQVf/pLwQHilk7EgueGupg1KEeQX\n\tATZIEx1sj1GoH0aYCzdvneyhYOMWKlgjALL2dDkwn8YjZbYyo46kinHFfxlBM1Si0Z\n\tKaUcrjhOeY7P7c2qZWLywi5vmYLramvRY8NJFncM=","Date":"Tue, 26 Nov 2024 18:58:52 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: camera: Fix clearing of stream\n\tassociations before `validate()`","Message-ID":"<20241126165852.GI5461@pendragon.ideasonboard.com>","References":"<20241125000834.396815-1-pobrn@protonmail.com>\n\t<20241125224822.GX19381@pendragon.ideasonboard.com>\n\t\n\t<20241126120846.GD5461@pendragon.ideasonboard.com>\n\t<20241126155518.GB5493@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]