[PATCH v1] treewide: move construct/assign `shared_ptr`s where possible
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jun 18 01:12:38 CEST 2024
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 ...
>
> 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