[{"id":32398,"web_url":"https://patchwork.libcamera.org/comment/32398/","msgid":"<20241126210858.GO5461@pendragon.ideasonboard.com>","date":"2024-11-26T21:08:58","subject":"Re: [PATCH v1] treewide: Avoid some copies in range-based for loops","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\n(CC'ing Harvey)\n\nThank you for the patch.\n\nOn Tue, Nov 26, 2024 at 06:03:10PM +0000, Barnabás Pőcze wrote:\n> Most of these have been found by the `performance-for-range-copy`\n> check of `clang-tidy`.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/camera_manager.cpp | 2 +-\n> src/libcamera/converter.cpp | 5 +++--\n> src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 2 +-\n> test/gstreamer/gstreamer_device_provider_test.cpp | 9 ++-------\n> 5 files changed, 8 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index c7cc5adb..87e6717e 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -388,7 +388,7 @@ std::shared_ptr CameraManager::get(const std::string &id)\n> \n> \tMutexLocker locker(d->mutex_);\n> \n> -\tfor (std::shared_ptr camera : d->cameras_) {\n> +\tfor (const std::shared_ptr &camera : d->cameras_) {\n> \t\tif (camera->id() == id)\n> \t\t\treturn camera;\n> \t}\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 945f2527..3a3f8434 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -325,8 +325,9 @@ std::vector ConverterFactoryBase::names()\n> \n> \tfor (ConverterFactoryBase *factory : factories) {\n> \t\tlist.push_back(factory->name_);\n> -\t\tfor (auto alias : factory->compatibles())\n> -\t\t\tlist.push_back(alias);\n> +\n> +\t\tconst auto &compatibles = factory->compatibles();\n> +\t\tlist.insert(list.end(), compatibles.begin(), compatibles.end());\n> \t}\n> \n> \treturn list;\n> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> index 2baef588..277efbb0 100644\n> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)\n> \t * For each file in the directory, load the image,\n> \t * convert it to NV12, and store the pointer.\n> \t */\n> -\tfor (std::filesystem::path path : imageFrames.files) {\n> +\tfor (const auto &path : imageFrames.files) {\n\nUnrelated to this patch, just because I notice this now in the virtual\npipeline handler that was merged recently. A while ago, we merged\n\ncommit 6a2f971035c2df711b10200f9c8c011d9a420e58\nAuthor: Nicholas Roth \nDate: Thu Oct 27 22:17:21 2022 -0500\n\n android: remove references to std::filesystem\n\n Android 11's toolchain does not support std::filesystem, but\n camera_hal_config.cpp currently uses it. Remove references to\n std::filesystem in order to support Android <= 11.\n\nWe should drop usage of std::filesystem in\nsrc/libcamera/pipeline/virtual/. I've been thinking of either extending\nthe libcamera base File class, or implementing new classes, with APIs\ninfluenced by https://doc.qt.io/qt-6/qfileinfo.html and\nhttps://doc.qt.io/qt-6/qdir.html. This would also replace existing use\nof stat() and fstat() through the code base.\n\n> \t\tFile file(path);\n> \t\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> \t\t\tLOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 991b06f2..caa5c20e 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager)\n> \n> PipelineHandler::~PipelineHandler()\n> {\n> -\tfor (std::shared_ptr media : mediaDevices_)\n> +\tfor (std::shared_ptr &media : mediaDevices_)\n> \t\tmedia->release();\n> }\n> \n> diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp\n> index 8b8e7cba..4b087fcb 100644\n> --- a/test/gstreamer/gstreamer_device_provider_test.cpp\n> +++ b/test/gstreamer/gstreamer_device_provider_test.cpp\n> @@ -52,17 +52,12 @@ protected:\n> \t\tfor (l = devices; l != NULL; l = g_list_next(l)) {\n> \t\t\tGstDevice *device = GST_DEVICE(l->data);\n> \t\t\tg_autofree gchar *gst_name;\n> -\t\t\tbool matched = false;\n> \n> \t\t\tg_autoptr(GstElement) element = gst_device_create_element(device, NULL);\n> \t\t\tg_object_get(element, \"camera-name\", &gst_name, NULL);\n> \n> -\t\t\tfor (auto name : cameraNames) {\n> -\t\t\t\tif (strcmp(name.c_str(), gst_name) == 0) {\n> -\t\t\t\t\tmatched = true;\n> -\t\t\t\t\tbreak;\n> -\t\t\t\t}\n> -\t\t\t}\n> +\t\t\tbool matched =\n> +\t\t\t\tstd::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end();\n> \n> \t\t\tif (!matched)\n> \t\t\t\treturn TestFail;\n\nMaybe you could now drop the matched variable and write\n\n\t\t\tif (std::find(cameraNames.begin(), cameraNames.end(), gst_name) !=\n\t\t\t cameraNames.end())\n\t\t\t\treturn TestFail;\n\n?\n\nReviewed-by: Laurent Pinchart \n\nThis makes me wish C++ had std::contains(). I suppose C++23's\nstd::ranges:contains() will do the job in several years. There's also\n\n\t\t\tif (std::any_of(cameraNames.begin(), cameraNames.end(),\n\t\t\t\t [&gst_name](const std::string &name) { return name == gst_name; })\n\t\t\t\treturn TestFail;\n\nwhich I don't think would be much of an improvement :-)","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 11A73BD16B\n\tfor ;\n\tTue, 26 Nov 2024 21:09:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CB1766090;\n\tTue, 26 Nov 2024 22:09:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 249B066083\n\tfor ;\n\tTue, 26 Nov 2024 22:09:09 +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 5BAE2B63;\n\tTue, 26 Nov 2024 22:08: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=\"pww5PaKr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732655326;\n\tbh=Xp53nDj9S/vQwdFQRRIq+f2uMeeCXZx6cA93KlpB8Qk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pww5PaKrdJmEd0GbECywaXjEzKkdZNddg9iAD0WwyYpKeBSwsxf6Pw8zHZ64bhZUt\n\tZ5zQpJvxooErU0QrArHwM0rDcLoyFqdKl+MDuOnaewmiBGwGKsL4E0LyZzMV70G//4\n\t3Zc2LIeZUTZTCtgilZ2EyiaQLhtnFzMSXys8hFgo=","Date":"Tue, 26 Nov 2024 23:08:58 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Subject":"Re: [PATCH v1] treewide: Avoid some copies in range-based for loops","Message-ID":"<20241126210858.GO5461@pendragon.ideasonboard.com>","References":"<20241126180302.685265-1-pobrn@protonmail.com>\n\t<20241126180302.685265-2-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241126180302.685265-2-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":32399,"web_url":"https://patchwork.libcamera.org/comment/32399/","msgid":"","date":"2024-11-26T22:28:52","subject":"Re: [PATCH v1] treewide: Avoid some copies in range-based for loops","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 26., kedd 22:08 keltezéssel, Laurent Pinchart írta:\n\n> Hi Barnabás,\n> \n> (CC'ing Harvey)\n> \n> Thank you for the patch.\n> \n> On Tue, Nov 26, 2024 at 06:03:10PM +0000, Barnabás Pőcze wrote:\n> > Most of these have been found by the `performance-for-range-copy`\n> > check of `clang-tidy`.\n> >\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > src/libcamera/camera_manager.cpp | 2 +-\n> > src/libcamera/converter.cpp | 5 +++--\n> > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 2 +-\n> > test/gstreamer/gstreamer_device_provider_test.cpp | 9 ++-------\n> > 5 files changed, 8 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index c7cc5adb..87e6717e 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -388,7 +388,7 @@ std::shared_ptr CameraManager::get(const std::string &id)\n> >\n> > \tMutexLocker locker(d->mutex_);\n> >\n> > -\tfor (std::shared_ptr camera : d->cameras_) {\n> > +\tfor (const std::shared_ptr &camera : d->cameras_) {\n> > \t\tif (camera->id() == id)\n> > \t\t\treturn camera;\n> > \t}\n> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > index 945f2527..3a3f8434 100644\n> > --- a/src/libcamera/converter.cpp\n> > +++ b/src/libcamera/converter.cpp\n> > @@ -325,8 +325,9 @@ std::vector ConverterFactoryBase::names()\n> >\n> > \tfor (ConverterFactoryBase *factory : factories) {\n> > \t\tlist.push_back(factory->name_);\n> > -\t\tfor (auto alias : factory->compatibles())\n> > -\t\t\tlist.push_back(alias);\n> > +\n> > +\t\tconst auto &compatibles = factory->compatibles();\n> > +\t\tlist.insert(list.end(), compatibles.begin(), compatibles.end());\n> > \t}\n> >\n> > \treturn list;\n> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > index 2baef588..277efbb0 100644\n> > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)\n> > \t * For each file in the directory, load the image,\n> > \t * convert it to NV12, and store the pointer.\n> > \t */\n> > -\tfor (std::filesystem::path path : imageFrames.files) {\n> > +\tfor (const auto &path : imageFrames.files) {\n> \n> Unrelated to this patch, just because I notice this now in the virtual\n> pipeline handler that was merged recently. A while ago, we merged\n> \n> commit 6a2f971035c2df711b10200f9c8c011d9a420e58\n> Author: Nicholas Roth \n> Date: Thu Oct 27 22:17:21 2022 -0500\n> \n> android: remove references to std::filesystem\n> \n> Android 11's toolchain does not support std::filesystem, but\n> camera_hal_config.cpp currently uses it. Remove references to\n> std::filesystem in order to support Android <= 11.\n> \n> We should drop usage of std::filesystem in\n> src/libcamera/pipeline/virtual/. I've been thinking of either extending\n> the libcamera base File class, or implementing new classes, with APIs\n> influenced by https://doc.qt.io/qt-6/qfileinfo.html and\n> https://doc.qt.io/qt-6/qdir.html. This would also replace existing use\n> of stat() and fstat() through the code base.\n\nThat is quite unfortunate... I think it was me who suggested using std::filesystem.\nSadly it will not be as easy as in the referenced commit.\n\nWhat is the android support policy?\n\nAlso, I am a bit puzzled by that commit. According to https://developer.android.com/ndk/downloads/revision_history\nNDK r22b (March 2021) added std::filesystem support. Additionally,\nhttps://github.com/android/ndk/wiki/Compatibility implies that r23 supports\neverything above API 16 (Jelly Bean), which, according to https://apilevels.com/\nis around android 4.\n\nIn any case, my experience is limited with android is limited, so I could've\nmissed an important detail.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> > \t\tFile file(path);\n> > \t\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > \t\t\tLOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 991b06f2..caa5c20e 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager)\n> >\n> > PipelineHandler::~PipelineHandler()\n> > {\n> > -\tfor (std::shared_ptr media : mediaDevices_)\n> > +\tfor (std::shared_ptr &media : mediaDevices_)\n> > \t\tmedia->release();\n> > }\n> >\n> > diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp\n> > index 8b8e7cba..4b087fcb 100644\n> > --- a/test/gstreamer/gstreamer_device_provider_test.cpp\n> > +++ b/test/gstreamer/gstreamer_device_provider_test.cpp\n> > @@ -52,17 +52,12 @@ protected:\n> > \t\tfor (l = devices; l != NULL; l = g_list_next(l)) {\n> > \t\t\tGstDevice *device = GST_DEVICE(l->data);\n> > \t\t\tg_autofree gchar *gst_name;\n> > -\t\t\tbool matched = false;\n> >\n> > \t\t\tg_autoptr(GstElement) element = gst_device_create_element(device, NULL);\n> > \t\t\tg_object_get(element, \"camera-name\", &gst_name, NULL);\n> >\n> > -\t\t\tfor (auto name : cameraNames) {\n> > -\t\t\t\tif (strcmp(name.c_str(), gst_name) == 0) {\n> > -\t\t\t\t\tmatched = true;\n> > -\t\t\t\t\tbreak;\n> > -\t\t\t\t}\n> > -\t\t\t}\n> > +\t\t\tbool matched =\n> > +\t\t\t\tstd::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end();\n> >\n> > \t\t\tif (!matched)\n> > \t\t\t\treturn TestFail;\n> \n> Maybe you could now drop the matched variable and write\n> \n> \t\t\tif (std::find(cameraNames.begin(), cameraNames.end(), gst_name) !=\n> \t\t\t cameraNames.end())\n> \t\t\t\treturn TestFail;\n> \n> ?\n> \n> Reviewed-by: Laurent Pinchart \n> \n> This makes me wish C++ had std::contains(). I suppose C++23's\n> std::ranges:contains() will do the job in several years. There's also\n> \n> \t\t\tif (std::any_of(cameraNames.begin(), cameraNames.end(),\n> \t\t\t\t [&gst_name](const std::string &name) { return name == gst_name; })\n> \t\t\t\treturn TestFail;\n> \n> which I don't think would be much of an improvement :-)\n> \n> --\n> Regards,\n> \n> Laurent Pinchart","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 0FA98C3200\n\tfor ;\n\tTue, 26 Nov 2024 22:29:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37CB766090;\n\tTue, 26 Nov 2024 23:29:00 +0100 (CET)","from mail-10630.protonmail.ch (mail-10630.protonmail.ch\n\t[79.135.106.30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3F7666083\n\tfor ;\n\tTue, 26 Nov 2024 23:28:58 +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=\"BtHam2LY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1732660137; x=1732919337;\n\tbh=YTV0eH2vqcaROV7PJyktLXQhvBH6bThF+AcEDNNySnY=;\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=BtHam2LYojPj4jz1gAsWIUrSNqWN3q+GPBlavKQeaWYStG8Dfcc4JieW+pATul1mQ\n\tzYTsCL6gdBtw6/4ZdaeE4GclgFXAfz4T/iVKpYSELj8PgyFwKhHp06cOC39fdiR5Pv\n\tV/yGlvaYg2rA7Dg76ztMNvo/g+t4uqsqyAiEMeCfTu2oXI+KoH+SuD6y7R2FTZZ5T8\n\tWkVC7ZxG9/zDtRsLqGxGBUmIOsUYIMLWLndWLhwBoU5p76Wltt1IXQc9EWtmqpFsQX\n\tiDFo3baFCU94CV+V5nrddKugYXQ/oicdC0S6BSrjP3TWR/oaDh9MLS3RugwjtCfaFt\n\t8kEeS8Vq7R/WQ==","Date":"Tue, 26 Nov 2024 22:28:52 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Subject":"Re: [PATCH v1] treewide: Avoid some copies in range-based for loops","Message-ID":"","In-Reply-To":"<20241126210858.GO5461@pendragon.ideasonboard.com>","References":"<20241126180302.685265-1-pobrn@protonmail.com>\n\t<20241126180302.685265-2-pobrn@protonmail.com>\n\t<20241126210858.GO5461@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"a1f30ba62332c6f68075e0bcb7c1745b0db1e43e","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":32401,"web_url":"https://patchwork.libcamera.org/comment/32401/","msgid":"<20241127061605.GQ5461@pendragon.ideasonboard.com>","date":"2024-11-27T06:16:05","subject":"Re: [PATCH v1] treewide: Avoid some copies in range-based for loops","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"CC'ing Nicholas.\n\nOn Tue, Nov 26, 2024 at 10:28:52PM +0000, Barnabás Pőcze wrote:\n> 2024. november 26., kedd 22:08 keltezéssel, Laurent Pinchart írta:\n> \n> > Hi Barnabás,\n> > \n> > (CC'ing Harvey)\n> > \n> > Thank you for the patch.\n> > \n> > On Tue, Nov 26, 2024 at 06:03:10PM +0000, Barnabás Pőcze wrote:\n> > > Most of these have been found by the `performance-for-range-copy`\n> > > check of `clang-tidy`.\n> > >\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/libcamera/camera_manager.cpp | 2 +-\n> > > src/libcamera/converter.cpp | 5 +++--\n> > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +-\n> > > src/libcamera/pipeline_handler.cpp | 2 +-\n> > > test/gstreamer/gstreamer_device_provider_test.cpp | 9 ++-------\n> > > 5 files changed, 8 insertions(+), 12 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > > index c7cc5adb..87e6717e 100644\n> > > --- a/src/libcamera/camera_manager.cpp\n> > > +++ b/src/libcamera/camera_manager.cpp\n> > > @@ -388,7 +388,7 @@ std::shared_ptr CameraManager::get(const std::string &id)\n> > >\n> > > \tMutexLocker locker(d->mutex_);\n> > >\n> > > -\tfor (std::shared_ptr camera : d->cameras_) {\n> > > +\tfor (const std::shared_ptr &camera : d->cameras_) {\n> > > \t\tif (camera->id() == id)\n> > > \t\t\treturn camera;\n> > > \t}\n> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > index 945f2527..3a3f8434 100644\n> > > --- a/src/libcamera/converter.cpp\n> > > +++ b/src/libcamera/converter.cpp\n> > > @@ -325,8 +325,9 @@ std::vector ConverterFactoryBase::names()\n> > >\n> > > \tfor (ConverterFactoryBase *factory : factories) {\n> > > \t\tlist.push_back(factory->name_);\n> > > -\t\tfor (auto alias : factory->compatibles())\n> > > -\t\t\tlist.push_back(alias);\n> > > +\n> > > +\t\tconst auto &compatibles = factory->compatibles();\n> > > +\t\tlist.insert(list.end(), compatibles.begin(), compatibles.end());\n> > > \t}\n> > >\n> > > \treturn list;\n> > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > index 2baef588..277efbb0 100644\n> > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp\n> > > @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)\n> > > \t * For each file in the directory, load the image,\n> > > \t * convert it to NV12, and store the pointer.\n> > > \t */\n> > > -\tfor (std::filesystem::path path : imageFrames.files) {\n> > > +\tfor (const auto &path : imageFrames.files) {\n> > \n> > Unrelated to this patch, just because I notice this now in the virtual\n> > pipeline handler that was merged recently. A while ago, we merged\n> > \n> > commit 6a2f971035c2df711b10200f9c8c011d9a420e58\n> > Author: Nicholas Roth \n> > Date: Thu Oct 27 22:17:21 2022 -0500\n> > \n> > android: remove references to std::filesystem\n> > \n> > Android 11's toolchain does not support std::filesystem, but\n> > camera_hal_config.cpp currently uses it. Remove references to\n> > std::filesystem in order to support Android <= 11.\n> > \n> > We should drop usage of std::filesystem in\n> > src/libcamera/pipeline/virtual/. I've been thinking of either extending\n> > the libcamera base File class, or implementing new classes, with APIs\n> > influenced by https://doc.qt.io/qt-6/qfileinfo.html and\n> > https://doc.qt.io/qt-6/qdir.html. This would also replace existing use\n> > of stat() and fstat() through the code base.\n> \n> That is quite unfortunate... I think it was me who suggested using std::filesystem.\n> Sadly it will not be as easy as in the referenced commit.\n> \n> What is the android support policy?\n\nNot clear yet :-) Nicholas, how far back do you need to support ?\n\n> Also, I am a bit puzzled by that commit. According to https://developer.android.com/ndk/downloads/revision_history\n> NDK r22b (March 2021) added std::filesystem support. Additionally,\n> https://github.com/android/ndk/wiki/Compatibility implies that r23 supports\n> everything above API 16 (Jelly Bean), which, according to https://apilevels.com/\n> is around android 4.\n> \n> In any case, my experience is limited with android is limited, so I could've\n> missed an important detail.\n> \n> > > \t\tFile file(path);\n> > > \t\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > \t\t\tLOG(Virtual, Error) << \"Failed to open image file \" << file.fileName()\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 991b06f2..caa5c20e 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager)\n> > >\n> > > PipelineHandler::~PipelineHandler()\n> > > {\n> > > -\tfor (std::shared_ptr media : mediaDevices_)\n> > > +\tfor (std::shared_ptr &media : mediaDevices_)\n> > > \t\tmedia->release();\n> > > }\n> > >\n> > > diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp\n> > > index 8b8e7cba..4b087fcb 100644\n> > > --- a/test/gstreamer/gstreamer_device_provider_test.cpp\n> > > +++ b/test/gstreamer/gstreamer_device_provider_test.cpp\n> > > @@ -52,17 +52,12 @@ protected:\n> > > \t\tfor (l = devices; l != NULL; l = g_list_next(l)) {\n> > > \t\t\tGstDevice *device = GST_DEVICE(l->data);\n> > > \t\t\tg_autofree gchar *gst_name;\n> > > -\t\t\tbool matched = false;\n> > >\n> > > \t\t\tg_autoptr(GstElement) element = gst_device_create_element(device, NULL);\n> > > \t\t\tg_object_get(element, \"camera-name\", &gst_name, NULL);\n> > >\n> > > -\t\t\tfor (auto name : cameraNames) {\n> > > -\t\t\t\tif (strcmp(name.c_str(), gst_name) == 0) {\n> > > -\t\t\t\t\tmatched = true;\n> > > -\t\t\t\t\tbreak;\n> > > -\t\t\t\t}\n> > > -\t\t\t}\n> > > +\t\t\tbool matched =\n> > > +\t\t\t\tstd::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end();\n> > >\n> > > \t\t\tif (!matched)\n> > > \t\t\t\treturn TestFail;\n> > \n> > Maybe you could now drop the matched variable and write\n> > \n> > \t\t\tif (std::find(cameraNames.begin(), cameraNames.end(), gst_name) !=\n> > \t\t\t cameraNames.end())\n> > \t\t\t\treturn TestFail;\n> > \n> > ?\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > This makes me wish C++ had std::contains(). I suppose C++23's\n> > std::ranges:contains() will do the job in several years. There's also\n> > \n> > \t\t\tif (std::any_of(cameraNames.begin(), cameraNames.end(),\n> > \t\t\t\t [&gst_name](const std::string &name) { return name == gst_name; })\n> > \t\t\t\treturn TestFail;\n> > \n> > which I don't think would be much of an improvement :-)","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 15DD4C3200\n\tfor ;\n\tWed, 27 Nov 2024 06:16:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 365BE6607E;\n\tWed, 27 Nov 2024 07:16:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CDB265FA0\n\tfor ;\n\tWed, 27 Nov 2024 07:16:16 +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 20C5C792;\n\tWed, 27 Nov 2024 07:15:53 +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=\"suWqM/k5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732688153;\n\tbh=hLSMQAkrZqagzvoY7v+CC6tYUPqZgmaHN/EXaJgobdQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=suWqM/k5ImbI/24LsZqgjh684/wMQYXBQhLKjiRYu3ulFFOgDZD2qAj2HJtfb5YCz\n\thBDA5wRbbnmEjRuXaS8Lqwv+GYfJ/zr7Enjau3s1JAL0BsH6U59WNEyHlkYfwpp/uu\n\tM12xwRalC0gZR5GmMoC73n37ItuZbfiMO5SMH258=","Date":"Wed, 27 Nov 2024 08:16:05 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ,\n\tNicholas Roth ","Subject":"Re: [PATCH v1] treewide: Avoid some copies in range-based for loops","Message-ID":"<20241127061605.GQ5461@pendragon.ideasonboard.com>","References":"<20241126180302.685265-1-pobrn@protonmail.com>\n\t<20241126180302.685265-2-pobrn@protonmail.com>\n\t<20241126210858.GO5461@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\" "}}]