[PATCH v1] treewide: move construct/assign `shared_ptr`s where possible
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jul 31 13:44:58 CEST 2024
Quoting Barnabás Pőcze (2024-07-29 19:33:52)
> 2024. június 18., kedd 1:12 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:
>
> > Quoting Barnabás Pőcze (2024-06-14 22:22:59)
> > > Copy construction/assignment is more expensive than move construction/assignment
> > > because it involves managing the reference counts. In fact, libstdc++ even
> > > checks a global variable to determine whether threads are used.
> > >
> > > In contrast, move construction can be done without any conditionals
> > > or reference count changes. So prefer move construction and assignment
> > > where possible.
> >
> > CI-'Green': https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1203640
> >
> > Except for the Soraka hardware test - but I don't yet know if that's
> > an expected failure or not ...
> >
>
> Is there anything I can help with here?
>
I was about to re-run ... but I think there's another failure
(unrelated) on the Soraka hardware so I can't yet from what I can tell.
Will try again once the ChromeOS tests are back online.
--
Kieran
>
> Regards,
> Barnabás Pőcze
>
>
> > >
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> >
> > Otherwise this sounds reasonable to me, and the builds are fine...
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > ---
> > > src/android/camera_capabilities.cpp | 2 +-
> > > src/apps/cam/camera_session.cpp | 2 +-
> > > src/apps/cam/capture_script.cpp | 2 +-
> > > src/apps/lc-compliance/helpers/capture.cpp | 8 ++++----
> > > src/apps/lc-compliance/main.cpp | 4 +---
> > > src/apps/qcam/main_window.cpp | 4 ++--
> > > src/gstreamer/gstlibcameraprovider.cpp | 3 +--
> > > src/gstreamer/gstlibcamerasrc.cpp | 9 ++++-----
> > > src/libcamera/base/bound_method.cpp | 4 ++--
> > > src/libcamera/base/message.cpp | 2 +-
> > > src/libcamera/camera_manager.cpp | 4 ++--
> > > src/libcamera/pipeline_handler.cpp | 8 +++-----
> > > src/py/libcamera/py_main.cpp | 2 +-
> > > src/v4l2/v4l2_camera.cpp | 2 +-
> > > src/v4l2/v4l2_compat_manager.cpp | 2 +-
> > > 15 files changed, 26 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index 71043e12..d34ed480 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -401,7 +401,7 @@ void CameraCapabilities::computeHwLevel(
> > > int CameraCapabilities::initialize(std::shared_ptr<Camera> camera,
> > > int orientation, int facing)
> > > {
> > > - camera_ = camera;
> > > + camera_ = std::move(camera);
> > > orientation_ = orientation;
> > > facing_ = facing;
> > > rawStreamAvailable_ = false;
> > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > > index 097dc479..4b6799b1 100644
> > > --- a/src/apps/cam/camera_session.cpp
> > > +++ b/src/apps/cam/camera_session.cpp
> > > @@ -43,7 +43,7 @@ CameraSession::CameraSession(CameraManager *cm,
> > > if (*endptr == '\0' && index > 0) {
> > > auto cameras = cm->cameras();
> > > if (index <= cameras.size())
> > > - camera_ = cameras[index - 1];
> > > + camera_ = std::move(cameras[index - 1]);
> > > }
> > >
> > > if (!camera_)
> > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp
> > > index fc1dfa75..7f502f5f 100644
> > > --- a/src/apps/cam/capture_script.cpp
> > > +++ b/src/apps/cam/capture_script.cpp
> > > @@ -15,7 +15,7 @@ using namespace libcamera;
> > >
> > > CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > const std::string &fileName)
> > > - : camera_(camera), loop_(0), valid_(false)
> > > + : camera_(std::move(camera)), loop_(0), valid_(false)
> > > {
> > > FILE *fh = fopen(fileName.c_str(), "r");
> > > if (!fh) {
> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> > > index 90c1530b..d1dafb6c 100644
> > > --- a/src/apps/lc-compliance/helpers/capture.cpp
> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp
> > > @@ -12,8 +12,8 @@
> > > using namespace libcamera;
> > >
> > > Capture::Capture(std::shared_ptr<Camera> camera)
> > > - : loop_(nullptr), camera_(camera),
> > > - allocator_(std::make_unique<FrameBufferAllocator>(camera))
> > > + : loop_(nullptr), camera_(std::move(camera)),
> > > + allocator_(std::make_unique<FrameBufferAllocator>(camera_))
> > > {
> > > }
> > >
> > > @@ -72,7 +72,7 @@ void Capture::stop()
> > > /* CaptureBalanced */
> > >
> > > CaptureBalanced::CaptureBalanced(std::shared_ptr<Camera> camera)
> > > - : Capture(camera)
> > > + : Capture(std::move(camera))
> > > {
> > > }
> > >
> > > @@ -144,7 +144,7 @@ void CaptureBalanced::requestComplete(Request *request)
> > > /* CaptureUnbalanced */
> > >
> > > CaptureUnbalanced::CaptureUnbalanced(std::shared_ptr<Camera> camera)
> > > - : Capture(camera)
> > > + : Capture(std::move(camera))
> > > {
> > > }
> > >
> > > diff --git a/src/apps/lc-compliance/main.cpp b/src/apps/lc-compliance/main.cpp
> > > index 3f1d2a61..98f2573d 100644
> > > --- a/src/apps/lc-compliance/main.cpp
> > > +++ b/src/apps/lc-compliance/main.cpp
> > > @@ -50,8 +50,6 @@ static void listCameras(CameraManager *cm)
> > >
> > > static int initCamera(CameraManager *cm, OptionsParser::Options options)
> > > {
> > > - std::shared_ptr<Camera> camera;
> > > -
> > > int ret = cm->start();
> > > if (ret) {
> > > std::cout << "Failed to start camera manager: "
> > > @@ -66,7 +64,7 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options)
> > > }
> > >
> > > const std::string &cameraId = options[OptCamera];
> > > - camera = cm->get(cameraId);
> > > + std::shared_ptr<Camera> camera = cm->get(cameraId);
> > > if (!camera) {
> > > std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl;
> > > listCameras(cm);
> > > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> > > index d515beed..bf7ddadb 100644
> > > --- a/src/apps/qcam/main_window.cpp
> > > +++ b/src/apps/qcam/main_window.cpp
> > > @@ -269,7 +269,7 @@ void MainWindow::switchCamera()
> > > if (camera_ && newCameraId == camera_->id())
> > > return;
> > >
> > > - const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
> > > + std::shared_ptr<Camera> cam = cm_->get(newCameraId);
> > >
> > > if (cam->acquire()) {
> > > qInfo() << "Failed to acquire camera" << cam->id().c_str();
> > > @@ -287,7 +287,7 @@ void MainWindow::switchCamera()
> > > if (camera_)
> > > camera_->release();
> > >
> > > - camera_ = cam;
> > > + camera_ = std::move(cam);
> > >
> > > startStopAction_->setChecked(true);
> > >
> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> > > index 4fb1b007..60816562 100644
> > > --- a/src/gstreamer/gstlibcameraprovider.cpp
> > > +++ b/src/gstreamer/gstlibcameraprovider.cpp
> > > @@ -189,7 +189,6 @@ static GList *
> > > gst_libcamera_provider_probe(GstDeviceProvider *provider)
> > > {
> > > GstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider);
> > > - std::shared_ptr<CameraManager> cm;
> > > GList *devices = nullptr;
> > > gint ret;
> > >
> > > @@ -200,7 +199,7 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)
> > > * gains monitoring support. Meanwhile we need to cycle start()/stop()
> > > * to ensure every probe() calls return the latest list.
> > > */
> > > - cm = gst_libcamera_get_camera_manager(ret);
> > > + std::shared_ptr<CameraManager> cm = gst_libcamera_get_camera_manager(ret);
> > > if (ret) {
> > > GST_ERROR_OBJECT(self, "Failed to retrieve device list: %s",
> > > g_strerror(-ret));
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 9680d809..428170d8 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -355,13 +355,12 @@ void GstLibcameraSrcState::clearRequests()
> > > static bool
> > > gst_libcamera_src_open(GstLibcameraSrc *self)
> > > {
> > > - std::shared_ptr<CameraManager> cm;
> > > std::shared_ptr<Camera> cam;
> > > gint ret;
> > >
> > > GST_DEBUG_OBJECT(self, "Opening camera device ...");
> > >
> > > - cm = gst_libcamera_get_camera_manager(ret);
> > > + std::shared_ptr<CameraManager> cm = gst_libcamera_get_camera_manager(ret);
> > > if (ret) {
> > > GST_ELEMENT_ERROR(self, LIBRARY, INIT,
> > > ("Failed listing cameras."),
> > > @@ -392,7 +391,7 @@ gst_libcamera_src_open(GstLibcameraSrc *self)
> > > ("libcamera::CameraMananger::cameras() is empty"));
> > > return false;
> > > }
> > > - cam = cameras[0];
> > > + cam = std::move(cameras[0]);
> > > }
> > >
> > > GST_INFO_OBJECT(self, "Using camera '%s'", cam->id().c_str());
> > > @@ -408,8 +407,8 @@ gst_libcamera_src_open(GstLibcameraSrc *self)
> > > cam->requestCompleted.connect(self->state, &GstLibcameraSrcState::requestCompleted);
> > >
> > > /* No need to lock here, we didn't start our threads yet. */
> > > - self->state->cm_ = cm;
> > > - self->state->cam_ = cam;
> > > + self->state->cm_ = std::move(cm);
> > > + self->state->cam_ = std::move(cam);
> > >
> > > return true;
> > > }
> > > diff --git a/src/libcamera/base/bound_method.cpp b/src/libcamera/base/bound_method.cpp
> > > index 322029a8..d888461e 100644
> > > --- a/src/libcamera/base/bound_method.cpp
> > > +++ b/src/libcamera/base/bound_method.cpp
> > > @@ -89,7 +89,7 @@ bool BoundMethodBase::activatePack(std::shared_ptr<BoundMethodPackBase> pack,
> > >
> > > case ConnectionTypeQueued: {
> > > std::unique_ptr<Message> msg =
> > > - std::make_unique<InvokeMessage>(this, pack, nullptr, deleteMethod);
> > > + std::make_unique<InvokeMessage>(this, std::move(pack), nullptr, deleteMethod);
> > > object_->postMessage(std::move(msg));
> > > return false;
> > > }
> > > @@ -98,7 +98,7 @@ bool BoundMethodBase::activatePack(std::shared_ptr<BoundMethodPackBase> pack,
> > > Semaphore semaphore;
> > >
> > > std::unique_ptr<Message> msg =
> > > - std::make_unique<InvokeMessage>(this, pack, &semaphore, deleteMethod);
> > > + std::make_unique<InvokeMessage>(this, std::move(pack), &semaphore, deleteMethod);
> > > object_->postMessage(std::move(msg));
> > >
> > > semaphore.acquire();
> > > diff --git a/src/libcamera/base/message.cpp b/src/libcamera/base/message.cpp
> > > index 098faac6..29ce1b21 100644
> > > --- a/src/libcamera/base/message.cpp
> > > +++ b/src/libcamera/base/message.cpp
> > > @@ -127,7 +127,7 @@ Message::Type Message::registerMessageType()
> > > InvokeMessage::InvokeMessage(BoundMethodBase *method,
> > > std::shared_ptr<BoundMethodPackBase> pack,
> > > Semaphore *semaphore, bool deleteMethod)
> > > - : Message(Message::InvokeMessage), method_(method), pack_(pack),
> > > + : Message(Message::InvokeMessage), method_(method), pack_(std::move(pack)),
> > > semaphore_(semaphore), deleteMethod_(deleteMethod)
> > > {
> > > }
> > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > > index 95a9e326..a20216a4 100644
> > > --- a/src/libcamera/camera_manager.cpp
> > > +++ b/src/libcamera/camera_manager.cpp
> > > @@ -233,7 +233,7 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
> > > MutexLocker locker(mutex_);
> > >
> > > auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> > > - [camera](std::shared_ptr<Camera> &c) {
> > > + [&camera](const std::shared_ptr<Camera> &c) {
> > > return c.get() == camera.get();
> > > });
> > > if (iter == cameras_.end())
> > > @@ -374,7 +374,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/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index 5ea2ca78..ad1fe874 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -75,7 +75,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager)
> > >
> > > PipelineHandler::~PipelineHandler()
> > > {
> > > - for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> > > + for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> > > media->release();
> > > }
> > >
> > > @@ -138,9 +138,7 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
> > > if (!media->acquire())
> > > return nullptr;
> > >
> > > - mediaDevices_.push_back(media);
> > > -
> > > - return media.get();
> > > + return mediaDevices_.emplace_back(std::move(media)).get();
> > > }
> > >
> > > /**
> > > @@ -699,7 +697,7 @@ void PipelineHandler::disconnect()
> > > continue;
> > >
> > > camera->disconnect();
> > > - manager_->_d()->removeCamera(camera);
> > > + manager_->_d()->removeCamera(std::move(camera));
> > > }
> > > }
> > >
> > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > > index bce08218..af683002 100644
> > > --- a/src/py/libcamera/py_main.cpp
> > > +++ b/src/py/libcamera/py_main.cpp
> > > @@ -65,7 +65,7 @@ public:
> > > }
> > >
> > > explicit PyCameraSmartPtr(std::shared_ptr<T> p)
> > > - : ptr_(p)
> > > + : ptr_(std::move(p))
> > > {
> > > }
> > >
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 0f3b862f..9bd89f40 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -17,7 +17,7 @@ using namespace libcamera;
> > > LOG_DECLARE_CATEGORY(V4L2Compat)
> > >
> > > V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> > > - : camera_(camera), isRunning_(false), bufferAllocator_(nullptr),
> > > + : camera_(std::move(camera)), isRunning_(false), bufferAllocator_(nullptr),
> > > efd_(-1), bufferAvailableCount_(0)
> > > {
> > > camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> > > index 6a00afb5..8d3ea8ee 100644
> > > --- a/src/v4l2/v4l2_compat_manager.cpp
> > > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > > @@ -85,7 +85,7 @@ int V4L2CompatManager::start()
> > > */
> > > auto cameras = cm_->cameras();
> > > for (auto [index, camera] : utils::enumerate(cameras)) {
> > > - V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera);
> > > + V4L2CameraProxy *proxy = new V4L2CameraProxy(index, std::move(camera));
> > > proxies_.emplace_back(proxy);
> > > }
> > >
> > > --
> > > 2.45.2
> > >
> > >
> >
More information about the libcamera-devel
mailing list