| Message ID | 20260113000808.15395-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta: > libcamera uses std::unique_ptr<> to simplify life time management of > objects and avoid leaks. For historical reasons there are a fair number > of plain pointers with manual memory management. Replace them with > std::unique_ptr<> when the conversion is simple. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../guides/application-developer.rst | 5 +++-- > Documentation/guides/pipeline-handler.rst | 7 +++--- > .../internal/device_enumerator_udev.h | 2 +- > include/libcamera/internal/ipc_unixsocket.h | 3 ++- > include/libcamera/internal/v4l2_device.h | 2 +- > include/libcamera/internal/v4l2_videodevice.h | 12 +++++----- > src/apps/qcam/main.cpp | 5 ++--- > src/apps/qcam/main_window.cpp | 11 +++++----- > src/apps/qcam/main_window.h | 2 +- > src/libcamera/device_enumerator_udev.cpp | 6 ++--- > src/libcamera/ipc_unixsocket.cpp | 7 +++--- > src/libcamera/v4l2_device.cpp | 5 +++-- > src/libcamera/v4l2_videodevice.cpp | 22 ++++++++----------- > src/v4l2/v4l2_camera.cpp | 7 +++--- > src/v4l2/v4l2_camera.h | 2 +- > src/v4l2/v4l2_compat_manager.cpp | 18 +++++---------- > src/v4l2/v4l2_compat_manager.h | 2 +- > 17 files changed, 52 insertions(+), 66 deletions(-) > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst > index 06c07d1e9449..d981b6a4f9e7 100644 > --- a/Documentation/guides/application-developer.rst > +++ b/Documentation/guides/application-developer.rst > @@ -279,7 +279,8 @@ as the parameter of the ``FrameBufferAllocator::buffers()`` function. > > .. code:: cpp > > - FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); > + std::unique_ptr<FrameBufferAllocator> allocator = > + std::make_unique<FrameBufferAllocator>(camera); > > for (StreamConfiguration &cfg : *config) { > int ret = allocator->allocate(cfg.stream()); > @@ -539,7 +540,7 @@ uses, so needs to do the following: > > camera->stop(); > allocator->free(stream); > - delete allocator; > + allocator.reset(); > camera->release(); > camera.reset(); > cm->stop(); > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst > index 85d9cc870021..c28eccb7cf0e 100644 > --- a/Documentation/guides/pipeline-handler.rst > +++ b/Documentation/guides/pipeline-handler.rst > @@ -424,20 +424,19 @@ it will be used: > { > public: > VividCameraData(PipelineHandler *pipe, MediaDevice *media) > - : Camera::Private(pipe), media_(media), video_(nullptr) > + : Camera::Private(pipe), media_(media) > { > } > > ~VividCameraData() > { > - delete video_; > } > > int init(); > void bufferReady(FrameBuffer *buffer); > > MediaDevice *media_; > - V4L2VideoDevice *video_; > + std::unique_ptr<V4L2VideoDevice> video_; > Stream stream_; > }; > > @@ -468,7 +467,7 @@ open a single capture device named 'vivid-000-vid-cap' by the device. > > int VividCameraData::init() > { > - video_ = new V4L2VideoDevice(media_->getEntityByName("vivid-000-vid-cap")); > + video_ = std::make_unique<V4L2VideoDevice>(media_->getEntityByName("vivid-000-vid-cap")); > if (video_->open()) > return -ENODEV; > The above is addressed by https://lists.libcamera.org/pipermail/libcamera-devel/2025-December/055929.html do you think that can be merged and this rebased, or should it be done the other way around? > diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h > index 0bf78b551e75..1ce988ab84d1 100644 > --- a/include/libcamera/internal/device_enumerator_udev.h > +++ b/include/libcamera/internal/device_enumerator_udev.h > @@ -69,7 +69,7 @@ private: > > struct udev *udev_; > struct udev_monitor *monitor_; > - EventNotifier *notifier_; > + std::unique_ptr<EventNotifier> notifier_; Locally I have been testing very similar changes, but I ended up with `std::optional<>` in most cases (e.g. here), since that (1) avoids the extra memory allocation (2) provides the same interface as a pointer (so not many changes) (3) most of these allocations are long lived, so there do not appear to be many memory savings with separate allocations so I think it's worth at least some consideration. > > std::set<dev_t> orphans_; > std::unordered_set<dev_t> devices_; > [...] > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index 57db0036db6b..10367e4e178a 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -280,10 +280,10 @@ private: > enum v4l2_buf_type bufferType_; > enum v4l2_memory memoryType_; > > - V4L2BufferCache *cache_; > + std::unique_ptr<V4L2BufferCache> cache_; > std::map<unsigned int, FrameBuffer *> queuedBuffers_; > > - EventNotifier *fdBufferNotifier_; > + std::unique_ptr<EventNotifier> fdBufferNotifier_; > > State state_; > std::optional<unsigned int> firstFrame_; > @@ -301,14 +301,14 @@ public: > int open(); > void close(); > > - V4L2VideoDevice *output() { return output_; } > - V4L2VideoDevice *capture() { return capture_; } > + V4L2VideoDevice *output() { return output_.get(); } > + V4L2VideoDevice *capture() { return capture_.get(); } > > private: > std::string deviceNode_; > > - V4L2VideoDevice *output_; > - V4L2VideoDevice *capture_; > + std::unique_ptr<V4L2VideoDevice> output_; > + std::unique_ptr<V4L2VideoDevice> capture_; Here for example, directly embedding them is also an option. > }; > > } /* namespace libcamera */ > [...]
On Tue, Jan 13, 2026 at 09:57:25AM +0100, Barnabás Pőcze wrote: > 2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta: > > libcamera uses std::unique_ptr<> to simplify life time management of > > objects and avoid leaks. For historical reasons there are a fair number > > of plain pointers with manual memory management. Replace them with > > std::unique_ptr<> when the conversion is simple. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../guides/application-developer.rst | 5 +++-- > > Documentation/guides/pipeline-handler.rst | 7 +++--- > > .../internal/device_enumerator_udev.h | 2 +- > > include/libcamera/internal/ipc_unixsocket.h | 3 ++- > > include/libcamera/internal/v4l2_device.h | 2 +- > > include/libcamera/internal/v4l2_videodevice.h | 12 +++++----- > > src/apps/qcam/main.cpp | 5 ++--- > > src/apps/qcam/main_window.cpp | 11 +++++----- > > src/apps/qcam/main_window.h | 2 +- > > src/libcamera/device_enumerator_udev.cpp | 6 ++--- > > src/libcamera/ipc_unixsocket.cpp | 7 +++--- > > src/libcamera/v4l2_device.cpp | 5 +++-- > > src/libcamera/v4l2_videodevice.cpp | 22 ++++++++----------- > > src/v4l2/v4l2_camera.cpp | 7 +++--- > > src/v4l2/v4l2_camera.h | 2 +- > > src/v4l2/v4l2_compat_manager.cpp | 18 +++++---------- > > src/v4l2/v4l2_compat_manager.h | 2 +- > > 17 files changed, 52 insertions(+), 66 deletions(-) > > > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst > > index 06c07d1e9449..d981b6a4f9e7 100644 > > --- a/Documentation/guides/application-developer.rst > > +++ b/Documentation/guides/application-developer.rst > > @@ -279,7 +279,8 @@ as the parameter of the ``FrameBufferAllocator::buffers()`` function. > > > > .. code:: cpp > > > > - FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); > > + std::unique_ptr<FrameBufferAllocator> allocator = > > + std::make_unique<FrameBufferAllocator>(camera); > > > > for (StreamConfiguration &cfg : *config) { > > int ret = allocator->allocate(cfg.stream()); > > @@ -539,7 +540,7 @@ uses, so needs to do the following: > > > > camera->stop(); > > allocator->free(stream); > > - delete allocator; > > + allocator.reset(); > > camera->release(); > > camera.reset(); > > cm->stop(); > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst > > index 85d9cc870021..c28eccb7cf0e 100644 > > --- a/Documentation/guides/pipeline-handler.rst > > +++ b/Documentation/guides/pipeline-handler.rst > > @@ -424,20 +424,19 @@ it will be used: > > { > > public: > > VividCameraData(PipelineHandler *pipe, MediaDevice *media) > > - : Camera::Private(pipe), media_(media), video_(nullptr) > > + : Camera::Private(pipe), media_(media) > > { > > } > > > > ~VividCameraData() > > { > > - delete video_; > > } > > > > int init(); > > void bufferReady(FrameBuffer *buffer); > > > > MediaDevice *media_; > > - V4L2VideoDevice *video_; > > + std::unique_ptr<V4L2VideoDevice> video_; > > Stream stream_; > > }; > > > > @@ -468,7 +467,7 @@ open a single capture device named 'vivid-000-vid-cap' by the device. > > > > int VividCameraData::init() > > { > > - video_ = new V4L2VideoDevice(media_->getEntityByName("vivid-000-vid-cap")); > > + video_ = std::make_unique<V4L2VideoDevice>(media_->getEntityByName("vivid-000-vid-cap")); > > if (video_->open()) > > return -ENODEV; > > > > The above is addressed by https://lists.libcamera.org/pipermail/libcamera-devel/2025-December/055929.html > do you think that can be merged and this rebased, or should it be done the other way around? I've just reviewed that patch, please merge it and I'll rebase v2 on top. > > diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h > > index 0bf78b551e75..1ce988ab84d1 100644 > > --- a/include/libcamera/internal/device_enumerator_udev.h > > +++ b/include/libcamera/internal/device_enumerator_udev.h > > @@ -69,7 +69,7 @@ private: > > > > struct udev *udev_; > > struct udev_monitor *monitor_; > > - EventNotifier *notifier_; > > + std::unique_ptr<EventNotifier> notifier_; > > Locally I have been testing very similar changes, but I ended up with > `std::optional<>` in most cases (e.g. here), since that > > (1) avoids the extra memory allocation > (2) provides the same interface as a pointer (so not many changes) > (3) most of these allocations are long lived, so there do not appear > to be many memory savings with separate allocations > > so I think it's worth at least some consideration. I agree, but I'd like to do that separately. This patch is about consistency, as we have standardized on std::unique_ptr<>. We can then replace some instances of unique_ptr with optional, or embed member variables directly as suggested below. > > > > std::set<dev_t> orphans_; > > std::unordered_set<dev_t> devices_; > > [...] > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index 57db0036db6b..10367e4e178a 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -280,10 +280,10 @@ private: > > enum v4l2_buf_type bufferType_; > > enum v4l2_memory memoryType_; > > > > - V4L2BufferCache *cache_; > > + std::unique_ptr<V4L2BufferCache> cache_; > > std::map<unsigned int, FrameBuffer *> queuedBuffers_; > > > > - EventNotifier *fdBufferNotifier_; > > + std::unique_ptr<EventNotifier> fdBufferNotifier_; > > > > State state_; > > std::optional<unsigned int> firstFrame_; > > @@ -301,14 +301,14 @@ public: > > int open(); > > void close(); > > > > - V4L2VideoDevice *output() { return output_; } > > - V4L2VideoDevice *capture() { return capture_; } > > + V4L2VideoDevice *output() { return output_.get(); } > > + V4L2VideoDevice *capture() { return capture_.get(); } > > > > private: > > std::string deviceNode_; > > > > - V4L2VideoDevice *output_; > > - V4L2VideoDevice *capture_; > > + std::unique_ptr<V4L2VideoDevice> output_; > > + std::unique_ptr<V4L2VideoDevice> capture_; > > Here for example, directly embedding them is also an option. > > > }; > > > > } /* namespace libcamera */ > > [...]
diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst index 06c07d1e9449..d981b6a4f9e7 100644 --- a/Documentation/guides/application-developer.rst +++ b/Documentation/guides/application-developer.rst @@ -279,7 +279,8 @@ as the parameter of the ``FrameBufferAllocator::buffers()`` function. .. code:: cpp - FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); + std::unique_ptr<FrameBufferAllocator> allocator = + std::make_unique<FrameBufferAllocator>(camera); for (StreamConfiguration &cfg : *config) { int ret = allocator->allocate(cfg.stream()); @@ -539,7 +540,7 @@ uses, so needs to do the following: camera->stop(); allocator->free(stream); - delete allocator; + allocator.reset(); camera->release(); camera.reset(); cm->stop(); diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst index 85d9cc870021..c28eccb7cf0e 100644 --- a/Documentation/guides/pipeline-handler.rst +++ b/Documentation/guides/pipeline-handler.rst @@ -424,20 +424,19 @@ it will be used: { public: VividCameraData(PipelineHandler *pipe, MediaDevice *media) - : Camera::Private(pipe), media_(media), video_(nullptr) + : Camera::Private(pipe), media_(media) { } ~VividCameraData() { - delete video_; } int init(); void bufferReady(FrameBuffer *buffer); MediaDevice *media_; - V4L2VideoDevice *video_; + std::unique_ptr<V4L2VideoDevice> video_; Stream stream_; }; @@ -468,7 +467,7 @@ open a single capture device named 'vivid-000-vid-cap' by the device. int VividCameraData::init() { - video_ = new V4L2VideoDevice(media_->getEntityByName("vivid-000-vid-cap")); + video_ = std::make_unique<V4L2VideoDevice>(media_->getEntityByName("vivid-000-vid-cap")); if (video_->open()) return -ENODEV; diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h index 0bf78b551e75..1ce988ab84d1 100644 --- a/include/libcamera/internal/device_enumerator_udev.h +++ b/include/libcamera/internal/device_enumerator_udev.h @@ -69,7 +69,7 @@ private: struct udev *udev_; struct udev_monitor *monitor_; - EventNotifier *notifier_; + std::unique_ptr<EventNotifier> notifier_; std::set<dev_t> orphans_; std::unordered_set<dev_t> devices_; diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h index 48bb7a9422b5..57614fa3f0c7 100644 --- a/include/libcamera/internal/ipc_unixsocket.h +++ b/include/libcamera/internal/ipc_unixsocket.h @@ -7,6 +7,7 @@ #pragma once +#include <memory> #include <stdint.h> #include <sys/types.h> #include <vector> @@ -53,7 +54,7 @@ private: UniqueFD fd_; bool headerReceived_; struct Header header_; - EventNotifier *notifier_; + std::unique_ptr<EventNotifier> notifier_; }; } /* namespace libcamera */ diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index dbbd118abd00..8c4c59bf7773 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -89,7 +89,7 @@ private: std::string deviceNode_; UniqueFD fd_; - EventNotifier *fdEventNotifier_; + std::unique_ptr<EventNotifier> fdEventNotifier_; bool frameStartEnabled_; }; diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index 57db0036db6b..10367e4e178a 100644 --- a/include/libcamera/internal/v4l2_videodevice.h +++ b/include/libcamera/internal/v4l2_videodevice.h @@ -280,10 +280,10 @@ private: enum v4l2_buf_type bufferType_; enum v4l2_memory memoryType_; - V4L2BufferCache *cache_; + std::unique_ptr<V4L2BufferCache> cache_; std::map<unsigned int, FrameBuffer *> queuedBuffers_; - EventNotifier *fdBufferNotifier_; + std::unique_ptr<EventNotifier> fdBufferNotifier_; State state_; std::optional<unsigned int> firstFrame_; @@ -301,14 +301,14 @@ public: int open(); void close(); - V4L2VideoDevice *output() { return output_; } - V4L2VideoDevice *capture() { return capture_; } + V4L2VideoDevice *output() { return output_.get(); } + V4L2VideoDevice *capture() { return capture_.get(); } private: std::string deviceNode_; - V4L2VideoDevice *output_; - V4L2VideoDevice *capture_; + std::unique_ptr<V4L2VideoDevice> output_; + std::unique_ptr<V4L2VideoDevice> capture_; }; } /* namespace libcamera */ diff --git a/src/apps/qcam/main.cpp b/src/apps/qcam/main.cpp index d0bde14130fb..8c43d43f0dca 100644 --- a/src/apps/qcam/main.cpp +++ b/src/apps/qcam/main.cpp @@ -73,7 +73,7 @@ int main(int argc, char **argv) sa.sa_handler = &signalHandler; sigaction(SIGINT, &sa, nullptr); - CameraManager *cm = new libcamera::CameraManager(); + std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>(); ret = cm->start(); if (ret) { @@ -82,13 +82,12 @@ int main(int argc, char **argv) return EXIT_FAILURE; } - MainWindow *mainWindow = new MainWindow(cm, options); + MainWindow *mainWindow = new MainWindow(cm.get(), options); mainWindow->show(); ret = app.exec(); delete mainWindow; cm->stop(); - delete cm; return ret; } diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp index 96a2d50901b7..e84e19d70728 100644 --- a/src/apps/qcam/main_window.cpp +++ b/src/apps/qcam/main_window.cpp @@ -86,8 +86,8 @@ private: }; MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) - : saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr), - isCapturing_(false), captureRaw_(false) + : saveRaw_(nullptr), options_(options), cm_(cm), isCapturing_(false), + captureRaw_(false) { int ret; @@ -449,7 +449,7 @@ int MainWindow::startCapture() saveRaw_->setEnabled(config_->size() == 2); /* Allocate and map buffers. */ - allocator_ = new FrameBufferAllocator(camera_); + allocator_ = std::make_unique<FrameBufferAllocator>(camera_); for (StreamConfiguration &config : *config_) { Stream *stream = config.stream(); @@ -530,8 +530,7 @@ error: freeBuffers_.clear(); - delete allocator_; - allocator_ = nullptr; + allocator_.reset(); return ret; } @@ -563,7 +562,7 @@ void MainWindow::stopCapture() requests_.clear(); freeQueue_.clear(); - delete allocator_; + allocator_.reset(); isCapturing_ = false; diff --git a/src/apps/qcam/main_window.h b/src/apps/qcam/main_window.h index 81fcf915ade5..20d369ad2318 100644 --- a/src/apps/qcam/main_window.h +++ b/src/apps/qcam/main_window.h @@ -109,7 +109,7 @@ private: /* Camera manager, camera, configuration and buffers */ libcamera::CameraManager *cm_; std::shared_ptr<libcamera::Camera> camera_; - libcamera::FrameBufferAllocator *allocator_; + std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; std::unique_ptr<libcamera::CameraConfiguration> config_; std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_; diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp index 3195dd06e2fc..8b9376ca6ac4 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -28,14 +28,12 @@ namespace libcamera { LOG_DECLARE_CATEGORY(DeviceEnumerator) DeviceEnumeratorUdev::DeviceEnumeratorUdev() - : udev_(nullptr), monitor_(nullptr), notifier_(nullptr) + : udev_(nullptr), monitor_(nullptr) { } DeviceEnumeratorUdev::~DeviceEnumeratorUdev() { - delete notifier_; - if (monitor_) udev_monitor_unref(monitor_); if (udev_) @@ -212,7 +210,7 @@ done: return ret; int fd = udev_monitor_get_fd(monitor_); - notifier_ = new EventNotifier(fd, EventNotifier::Read); + notifier_ = std::make_unique<EventNotifier>(fd, EventNotifier::Read); notifier_->activated.connect(this, &DeviceEnumeratorUdev::udevNotify); return 0; diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp index 002053e35557..6ec5a5cb6595 100644 --- a/src/libcamera/ipc_unixsocket.cpp +++ b/src/libcamera/ipc_unixsocket.cpp @@ -70,7 +70,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket) */ IPCUnixSocket::IPCUnixSocket() - : headerReceived_(false), notifier_(nullptr) + : headerReceived_(false) { } @@ -130,7 +130,7 @@ int IPCUnixSocket::bind(UniqueFD fd) return -EINVAL; fd_ = std::move(fd); - notifier_ = new EventNotifier(fd_.get(), EventNotifier::Read); + notifier_ = std::make_unique<EventNotifier>(fd_.get(), EventNotifier::Read); notifier_->activated.connect(this, &IPCUnixSocket::dataNotifier); return 0; @@ -146,8 +146,7 @@ void IPCUnixSocket::close() if (!isBound()) return; - delete notifier_; - notifier_ = nullptr; + notifier_.reset(); fd_.reset(); headerReceived_ = false; diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 1582c03efacd..206efb0d951d 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -123,7 +123,8 @@ int V4L2Device::setFd(UniqueFD fd) fd_ = std::move(fd); - fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception); + fdEventNotifier_ = std::make_unique<EventNotifier>(fd_.get(), + EventNotifier::Exception); fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable); fdEventNotifier_->setEnabled(false); @@ -142,7 +143,7 @@ void V4L2Device::close() if (!isOpen()) return; - delete fdEventNotifier_; + fdEventNotifier_.reset(); fd_.reset(); } diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 25b61d049a0e..ddb757b04e59 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -535,8 +535,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2DeviceFormat &f) */ V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr), - fdBufferNotifier_(nullptr), state_(State::Stopped), - watchdogDuration_(0.0) + state_(State::Stopped), watchdogDuration_(0.0) { /* * We default to an MMAP based CAPTURE video device, however this will @@ -626,7 +625,7 @@ int V4L2VideoDevice::open() return -EINVAL; } - fdBufferNotifier_ = new EventNotifier(fd(), notifierType); + fdBufferNotifier_ = std::make_unique<EventNotifier>(fd(), notifierType); fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); fdBufferNotifier_->setEnabled(false); @@ -715,7 +714,7 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type) return -EINVAL; } - fdBufferNotifier_ = new EventNotifier(fd(), notifierType); + fdBufferNotifier_ = std::make_unique<EventNotifier>(fd(), notifierType); fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); fdBufferNotifier_->setEnabled(false); @@ -760,7 +759,7 @@ void V4L2VideoDevice::close() return; releaseBuffers(); - delete fdBufferNotifier_; + fdBufferNotifier_.reset(); formatInfo_ = nullptr; @@ -1374,7 +1373,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, if (ret < 0) return ret; - cache_ = new V4L2BufferCache(*buffers); + cache_ = std::make_unique<V4L2BufferCache>(*buffers); memoryType_ = V4L2_MEMORY_MMAP; return ret; @@ -1599,7 +1598,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count) if (ret) return ret; - cache_ = new V4L2BufferCache(count); + cache_ = std::make_unique<V4L2BufferCache>(count); LOG(V4L2, Debug) << "Prepared to import " << count << " buffers"; @@ -1621,8 +1620,7 @@ int V4L2VideoDevice::releaseBuffers() LOG(V4L2, Debug) << "Releasing buffers"; - delete cache_; - cache_ = nullptr; + cache_.reset(); return requestBuffers(0, memoryType_); } @@ -2199,14 +2197,12 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode) : deviceNode_(deviceNode) { - output_ = new V4L2VideoDevice(deviceNode); - capture_ = new V4L2VideoDevice(deviceNode); + output_ = std::make_unique<V4L2VideoDevice>(deviceNode); + capture_ = std::make_unique<V4L2VideoDevice>(deviceNode); } V4L2M2MDevice::~V4L2M2MDevice() { - delete capture_; - delete output_; } /** diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index 94d138cd5710..412ab4ea678e 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat) V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera) : camera_(camera), controls_(controls::controls), isRunning_(false), - bufferAllocator_(nullptr), efd_(-1), bufferAvailableCount_(0) + efd_(-1), bufferAvailableCount_(0) { camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete); } @@ -43,7 +43,7 @@ int V4L2Camera::open(StreamConfiguration *streamConfig) return -EINVAL; } - bufferAllocator_ = new FrameBufferAllocator(camera_); + bufferAllocator_ = std::make_unique<FrameBufferAllocator>(camera_); *streamConfig = config_->at(0); return 0; @@ -53,8 +53,7 @@ void V4L2Camera::close() { requestPool_.clear(); - delete bufferAllocator_; - bufferAllocator_ = nullptr; + bufferAllocator_.reset(); camera_->release(); } diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index 9bd161b909de..3d09ae7d6057 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -80,7 +80,7 @@ private: bool isRunning_; libcamera::Mutex bufferLock_; - libcamera::FrameBufferAllocator *bufferAllocator_; + std::unique_ptr<libcamera::FrameBufferAllocator> bufferAllocator_; std::vector<std::unique_ptr<libcamera::Request>> requestPool_; diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index f53fb300dde8..a92f2e2944fc 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -54,25 +54,21 @@ V4L2CompatManager::~V4L2CompatManager() { files_.clear(); mmaps_.clear(); + proxies_.clear(); - if (cm_) { - proxies_.clear(); + if (cm_) cm_->stop(); - delete cm_; - cm_ = nullptr; - } } int V4L2CompatManager::start() { - cm_ = new CameraManager(); + cm_ = std::make_unique<CameraManager>(); int ret = cm_->start(); if (ret) { LOG(V4L2Compat, Error) << "Failed to start camera manager: " << strerror(-ret); - delete cm_; - cm_ = nullptr; + cm_.reset(); return ret; } @@ -83,10 +79,8 @@ int V4L2CompatManager::start() * created here to wrap a camera device. */ auto cameras = cm_->cameras(); - for (auto [index, camera] : utils::enumerate(cameras)) { - V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera); - proxies_.emplace_back(proxy); - } + for (auto [index, camera] : utils::enumerate(cameras)) + proxies_.emplace_back(std::make_unique<V4L2CameraProxy>(index, camera)); return 0; } diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h index f7c6f1228282..13673d604a63 100644 --- a/src/v4l2/v4l2_compat_manager.h +++ b/src/v4l2/v4l2_compat_manager.h @@ -61,7 +61,7 @@ private: FileOperations fops_; - libcamera::CameraManager *cm_; + std::unique_ptr<libcamera::CameraManager> cm_; std::vector<std::unique_ptr<V4L2CameraProxy>> proxies_; std::map<int, std::shared_ptr<V4L2CameraFile>> files_;
libcamera uses std::unique_ptr<> to simplify life time management of objects and avoid leaks. For historical reasons there are a fair number of plain pointers with manual memory management. Replace them with std::unique_ptr<> when the conversion is simple. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../guides/application-developer.rst | 5 +++-- Documentation/guides/pipeline-handler.rst | 7 +++--- .../internal/device_enumerator_udev.h | 2 +- include/libcamera/internal/ipc_unixsocket.h | 3 ++- include/libcamera/internal/v4l2_device.h | 2 +- include/libcamera/internal/v4l2_videodevice.h | 12 +++++----- src/apps/qcam/main.cpp | 5 ++--- src/apps/qcam/main_window.cpp | 11 +++++----- src/apps/qcam/main_window.h | 2 +- src/libcamera/device_enumerator_udev.cpp | 6 ++--- src/libcamera/ipc_unixsocket.cpp | 7 +++--- src/libcamera/v4l2_device.cpp | 5 +++-- src/libcamera/v4l2_videodevice.cpp | 22 ++++++++----------- src/v4l2/v4l2_camera.cpp | 7 +++--- src/v4l2/v4l2_camera.h | 2 +- src/v4l2/v4l2_compat_manager.cpp | 18 +++++---------- src/v4l2/v4l2_compat_manager.h | 2 +- 17 files changed, 52 insertions(+), 66 deletions(-)