[PATCH v1] treewide: Avoid some copies in range-based for loops
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 27 07:16:05 CET 2024
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 at 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 at 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 at 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
More information about the libcamera-devel
mailing list