| Message ID | 20241126180302.685265-2-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, (CC'ing Harvey) Thank you for the patch. On Tue, Nov 26, 2024 at 06:03:10PM +0000, Barnabás Pőcze wrote: > Most of these have been found by the `performance-for-range-copy` > check of `clang-tidy`. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/camera_manager.cpp | 2 +- > src/libcamera/converter.cpp | 5 +++-- > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- > src/libcamera/pipeline_handler.cpp | 2 +- > test/gstreamer/gstreamer_device_provider_test.cpp | 9 ++------- > 5 files changed, 8 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index c7cc5adb..87e6717e 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -388,7 +388,7 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &id) > > MutexLocker locker(d->mutex_); > > - for (std::shared_ptr<Camera> camera : d->cameras_) { > + for (const std::shared_ptr<Camera> &camera : d->cameras_) { > if (camera->id() == id) > return camera; > } > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp > index 945f2527..3a3f8434 100644 > --- a/src/libcamera/converter.cpp > +++ b/src/libcamera/converter.cpp > @@ -325,8 +325,9 @@ std::vector<std::string> ConverterFactoryBase::names() > > for (ConverterFactoryBase *factory : factories) { > list.push_back(factory->name_); > - for (auto alias : factory->compatibles()) > - list.push_back(alias); > + > + const auto &compatibles = factory->compatibles(); > + list.insert(list.end(), compatibles.begin(), compatibles.end()); > } > > return list; > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index 2baef588..277efbb0 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames) > * For each file in the directory, load the image, > * convert it to NV12, and store the pointer. > */ > - for (std::filesystem::path path : imageFrames.files) { > + for (const auto &path : imageFrames.files) { Unrelated to this patch, just because I notice this now in the virtual pipeline handler that was merged recently. A while ago, we merged commit 6a2f971035c2df711b10200f9c8c011d9a420e58 Author: Nicholas Roth <nicholas@rothemail.net> Date: Thu Oct 27 22:17:21 2022 -0500 android: remove references to std::filesystem Android 11's toolchain does not support std::filesystem, but camera_hal_config.cpp currently uses it. Remove references to std::filesystem in order to support Android <= 11. We should drop usage of std::filesystem in src/libcamera/pipeline/virtual/. I've been thinking of either extending the libcamera base File class, or implementing new classes, with APIs influenced by https://doc.qt.io/qt-6/qfileinfo.html and https://doc.qt.io/qt-6/qdir.html. This would also replace existing use of stat() and fstat() through the code base. > File file(path); > if (!file.open(File::OpenModeFlag::ReadOnly)) { > LOG(Virtual, Error) << "Failed to open image file " << file.fileName() > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 991b06f2..caa5c20e 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager) > > PipelineHandler::~PipelineHandler() > { > - for (std::shared_ptr<MediaDevice> media : mediaDevices_) > + for (std::shared_ptr<MediaDevice> &media : mediaDevices_) > media->release(); > } > > diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp > index 8b8e7cba..4b087fcb 100644 > --- a/test/gstreamer/gstreamer_device_provider_test.cpp > +++ b/test/gstreamer/gstreamer_device_provider_test.cpp > @@ -52,17 +52,12 @@ protected: > for (l = devices; l != NULL; l = g_list_next(l)) { > GstDevice *device = GST_DEVICE(l->data); > g_autofree gchar *gst_name; > - bool matched = false; > > g_autoptr(GstElement) element = gst_device_create_element(device, NULL); > g_object_get(element, "camera-name", &gst_name, NULL); > > - for (auto name : cameraNames) { > - if (strcmp(name.c_str(), gst_name) == 0) { > - matched = true; > - break; > - } > - } > + bool matched = > + std::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end(); > > if (!matched) > return TestFail; Maybe you could now drop the matched variable and write if (std::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end()) return TestFail; ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> This makes me wish C++ had std::contains(). I suppose C++23's std::ranges:contains() will do the job in several years. There's also if (std::any_of(cameraNames.begin(), cameraNames.end(), [&gst_name](const std::string &name) { return name == gst_name; }) return TestFail; which I don't think would be much of an improvement :-)
Hi 2024. november 26., kedd 22:08 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > (CC'ing Harvey) > > Thank you for the patch. > > On Tue, Nov 26, 2024 at 06:03:10PM +0000, Barnabás Pőcze wrote: > > Most of these have been found by the `performance-for-range-copy` > > check of `clang-tidy`. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/libcamera/camera_manager.cpp | 2 +- > > src/libcamera/converter.cpp | 5 +++-- > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- > > src/libcamera/pipeline_handler.cpp | 2 +- > > test/gstreamer/gstreamer_device_provider_test.cpp | 9 ++------- > > 5 files changed, 8 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index c7cc5adb..87e6717e 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -388,7 +388,7 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &id) > > > > MutexLocker locker(d->mutex_); > > > > - for (std::shared_ptr<Camera> camera : d->cameras_) { > > + for (const std::shared_ptr<Camera> &camera : d->cameras_) { > > if (camera->id() == id) > > return camera; > > } > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp > > index 945f2527..3a3f8434 100644 > > --- a/src/libcamera/converter.cpp > > +++ b/src/libcamera/converter.cpp > > @@ -325,8 +325,9 @@ std::vector<std::string> ConverterFactoryBase::names() > > > > for (ConverterFactoryBase *factory : factories) { > > list.push_back(factory->name_); > > - for (auto alias : factory->compatibles()) > > - list.push_back(alias); > > + > > + const auto &compatibles = factory->compatibles(); > > + list.insert(list.end(), compatibles.begin(), compatibles.end()); > > } > > > > return list; > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > index 2baef588..277efbb0 100644 > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames) > > * For each file in the directory, load the image, > > * convert it to NV12, and store the pointer. > > */ > > - for (std::filesystem::path path : imageFrames.files) { > > + for (const auto &path : imageFrames.files) { > > Unrelated to this patch, just because I notice this now in the virtual > pipeline handler that was merged recently. A while ago, we merged > > commit 6a2f971035c2df711b10200f9c8c011d9a420e58 > Author: Nicholas Roth <nicholas@rothemail.net> > Date: Thu Oct 27 22:17:21 2022 -0500 > > android: remove references to std::filesystem > > Android 11's toolchain does not support std::filesystem, but > camera_hal_config.cpp currently uses it. Remove references to > std::filesystem in order to support Android <= 11. > > We should drop usage of std::filesystem in > src/libcamera/pipeline/virtual/. I've been thinking of either extending > the libcamera base File class, or implementing new classes, with APIs > influenced by https://doc.qt.io/qt-6/qfileinfo.html and > https://doc.qt.io/qt-6/qdir.html. This would also replace existing use > of stat() and fstat() through the code base. That is quite unfortunate... I think it was me who suggested using std::filesystem. Sadly it will not be as easy as in the referenced commit. What is the android support policy? Also, I am a bit puzzled by that commit. According to https://developer.android.com/ndk/downloads/revision_history NDK r22b (March 2021) added std::filesystem support. Additionally, https://github.com/android/ndk/wiki/Compatibility implies that r23 supports everything above API 16 (Jelly Bean), which, according to https://apilevels.com/ is around android 4. In any case, my experience is limited with android is limited, so I could've missed an important detail. Regards, Barnabás Pőcze > > > File file(path); > > if (!file.open(File::OpenModeFlag::ReadOnly)) { > > LOG(Virtual, Error) << "Failed to open image file " << file.fileName() > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 991b06f2..caa5c20e 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager) > > > > PipelineHandler::~PipelineHandler() > > { > > - for (std::shared_ptr<MediaDevice> media : mediaDevices_) > > + for (std::shared_ptr<MediaDevice> &media : mediaDevices_) > > media->release(); > > } > > > > diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp > > index 8b8e7cba..4b087fcb 100644 > > --- a/test/gstreamer/gstreamer_device_provider_test.cpp > > +++ b/test/gstreamer/gstreamer_device_provider_test.cpp > > @@ -52,17 +52,12 @@ protected: > > for (l = devices; l != NULL; l = g_list_next(l)) { > > GstDevice *device = GST_DEVICE(l->data); > > g_autofree gchar *gst_name; > > - bool matched = false; > > > > g_autoptr(GstElement) element = gst_device_create_element(device, NULL); > > g_object_get(element, "camera-name", &gst_name, NULL); > > > > - for (auto name : cameraNames) { > > - if (strcmp(name.c_str(), gst_name) == 0) { > > - matched = true; > > - break; > > - } > > - } > > + bool matched = > > + std::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end(); > > > > if (!matched) > > return TestFail; > > Maybe you could now drop the matched variable and write > > if (std::find(cameraNames.begin(), cameraNames.end(), gst_name) != > cameraNames.end()) > return TestFail; > > ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > This makes me wish C++ had std::contains(). I suppose C++23's > std::ranges:contains() will do the job in several years. There's also > > if (std::any_of(cameraNames.begin(), cameraNames.end(), > [&gst_name](const std::string &name) { return name == gst_name; }) > return TestFail; > > which I don't think would be much of an improvement :-) > > -- > Regards, > > Laurent Pinchart
CC'ing Nicholas. On Tue, Nov 26, 2024 at 10:28:52PM +0000, Barnabás Pőcze wrote: > 2024. november 26., kedd 22:08 keltezéssel, Laurent Pinchart írta: > > > Hi Barnabás, > > > > (CC'ing Harvey) > > > > Thank you for the patch. > > > > On Tue, Nov 26, 2024 at 06:03:10PM +0000, Barnabás Pőcze wrote: > > > Most of these have been found by the `performance-for-range-copy` > > > check of `clang-tidy`. > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > src/libcamera/camera_manager.cpp | 2 +- > > > src/libcamera/converter.cpp | 5 +++-- > > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- > > > src/libcamera/pipeline_handler.cpp | 2 +- > > > test/gstreamer/gstreamer_device_provider_test.cpp | 9 ++------- > > > 5 files changed, 8 insertions(+), 12 deletions(-) > > > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > > index c7cc5adb..87e6717e 100644 > > > --- a/src/libcamera/camera_manager.cpp > > > +++ b/src/libcamera/camera_manager.cpp > > > @@ -388,7 +388,7 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &id) > > > > > > MutexLocker locker(d->mutex_); > > > > > > - for (std::shared_ptr<Camera> camera : d->cameras_) { > > > + for (const std::shared_ptr<Camera> &camera : d->cameras_) { > > > if (camera->id() == id) > > > return camera; > > > } > > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp > > > index 945f2527..3a3f8434 100644 > > > --- a/src/libcamera/converter.cpp > > > +++ b/src/libcamera/converter.cpp > > > @@ -325,8 +325,9 @@ std::vector<std::string> ConverterFactoryBase::names() > > > > > > for (ConverterFactoryBase *factory : factories) { > > > list.push_back(factory->name_); > > > - for (auto alias : factory->compatibles()) > > > - list.push_back(alias); > > > + > > > + const auto &compatibles = factory->compatibles(); > > > + list.insert(list.end(), compatibles.begin(), compatibles.end()); > > > } > > > > > > return list; > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > index 2baef588..277efbb0 100644 > > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames) > > > * For each file in the directory, load the image, > > > * convert it to NV12, and store the pointer. > > > */ > > > - for (std::filesystem::path path : imageFrames.files) { > > > + for (const auto &path : imageFrames.files) { > > > > Unrelated to this patch, just because I notice this now in the virtual > > pipeline handler that was merged recently. A while ago, we merged > > > > commit 6a2f971035c2df711b10200f9c8c011d9a420e58 > > Author: Nicholas Roth <nicholas@rothemail.net> > > Date: Thu Oct 27 22:17:21 2022 -0500 > > > > android: remove references to std::filesystem > > > > Android 11's toolchain does not support std::filesystem, but > > camera_hal_config.cpp currently uses it. Remove references to > > std::filesystem in order to support Android <= 11. > > > > We should drop usage of std::filesystem in > > src/libcamera/pipeline/virtual/. I've been thinking of either extending > > the libcamera base File class, or implementing new classes, with APIs > > influenced by https://doc.qt.io/qt-6/qfileinfo.html and > > https://doc.qt.io/qt-6/qdir.html. This would also replace existing use > > of stat() and fstat() through the code base. > > That is quite unfortunate... I think it was me who suggested using std::filesystem. > Sadly it will not be as easy as in the referenced commit. > > What is the android support policy? Not clear yet :-) Nicholas, how far back do you need to support ? > Also, I am a bit puzzled by that commit. According to https://developer.android.com/ndk/downloads/revision_history > NDK r22b (March 2021) added std::filesystem support. Additionally, > https://github.com/android/ndk/wiki/Compatibility implies that r23 supports > everything above API 16 (Jelly Bean), which, according to https://apilevels.com/ > is around android 4. > > In any case, my experience is limited with android is limited, so I could've > missed an important detail. > > > > File file(path); > > > if (!file.open(File::OpenModeFlag::ReadOnly)) { > > > LOG(Virtual, Error) << "Failed to open image file " << file.fileName() > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 991b06f2..caa5c20e 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager) > > > > > > PipelineHandler::~PipelineHandler() > > > { > > > - for (std::shared_ptr<MediaDevice> media : mediaDevices_) > > > + for (std::shared_ptr<MediaDevice> &media : mediaDevices_) > > > media->release(); > > > } > > > > > > diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp > > > index 8b8e7cba..4b087fcb 100644 > > > --- a/test/gstreamer/gstreamer_device_provider_test.cpp > > > +++ b/test/gstreamer/gstreamer_device_provider_test.cpp > > > @@ -52,17 +52,12 @@ protected: > > > for (l = devices; l != NULL; l = g_list_next(l)) { > > > GstDevice *device = GST_DEVICE(l->data); > > > g_autofree gchar *gst_name; > > > - bool matched = false; > > > > > > g_autoptr(GstElement) element = gst_device_create_element(device, NULL); > > > g_object_get(element, "camera-name", &gst_name, NULL); > > > > > > - for (auto name : cameraNames) { > > > - if (strcmp(name.c_str(), gst_name) == 0) { > > > - matched = true; > > > - break; > > > - } > > > - } > > > + bool matched = > > > + std::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end(); > > > > > > if (!matched) > > > return TestFail; > > > > Maybe you could now drop the matched variable and write > > > > if (std::find(cameraNames.begin(), cameraNames.end(), gst_name) != > > cameraNames.end()) > > return TestFail; > > > > ? > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > This makes me wish C++ had std::contains(). I suppose C++23's > > std::ranges:contains() will do the job in several years. There's also > > > > if (std::any_of(cameraNames.begin(), cameraNames.end(), > > [&gst_name](const std::string &name) { return name == gst_name; }) > > return TestFail; > > > > which I don't think would be much of an improvement :-)
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index c7cc5adb..87e6717e 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -388,7 +388,7 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &id) MutexLocker locker(d->mutex_); - for (std::shared_ptr<Camera> camera : d->cameras_) { + for (const std::shared_ptr<Camera> &camera : d->cameras_) { if (camera->id() == id) return camera; } diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp index 945f2527..3a3f8434 100644 --- a/src/libcamera/converter.cpp +++ b/src/libcamera/converter.cpp @@ -325,8 +325,9 @@ std::vector<std::string> ConverterFactoryBase::names() for (ConverterFactoryBase *factory : factories) { list.push_back(factory->name_); - for (auto alias : factory->compatibles()) - list.push_back(alias); + + const auto &compatibles = factory->compatibles(); + list.insert(list.end(), compatibles.begin(), compatibles.end()); } return list; diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp index 2baef588..277efbb0 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames) * For each file in the directory, load the image, * convert it to NV12, and store the pointer. */ - for (std::filesystem::path path : imageFrames.files) { + for (const auto &path : imageFrames.files) { File file(path); if (!file.open(File::OpenModeFlag::ReadOnly)) { LOG(Virtual, Error) << "Failed to open image file " << file.fileName() diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 991b06f2..caa5c20e 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager) PipelineHandler::~PipelineHandler() { - for (std::shared_ptr<MediaDevice> media : mediaDevices_) + for (std::shared_ptr<MediaDevice> &media : mediaDevices_) media->release(); } diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp index 8b8e7cba..4b087fcb 100644 --- a/test/gstreamer/gstreamer_device_provider_test.cpp +++ b/test/gstreamer/gstreamer_device_provider_test.cpp @@ -52,17 +52,12 @@ protected: for (l = devices; l != NULL; l = g_list_next(l)) { GstDevice *device = GST_DEVICE(l->data); g_autofree gchar *gst_name; - bool matched = false; g_autoptr(GstElement) element = gst_device_create_element(device, NULL); g_object_get(element, "camera-name", &gst_name, NULL); - for (auto name : cameraNames) { - if (strcmp(name.c_str(), gst_name) == 0) { - matched = true; - break; - } - } + bool matched = + std::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end(); if (!matched) return TestFail;
Most of these have been found by the `performance-for-range-copy` check of `clang-tidy`. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/camera_manager.cpp | 2 +- src/libcamera/converter.cpp | 5 +++-- src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- src/libcamera/pipeline_handler.cpp | 2 +- test/gstreamer/gstreamer_device_provider_test.cpp | 9 ++------- 5 files changed, 8 insertions(+), 12 deletions(-)