[libcamera-devel] [PATCH 2/2] libcamera: device_enumerator: Convey device ownership through unique_ptr
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 23 15:30:14 CET 2020
Hi Kieran,
On Mon, Mar 23, 2020 at 02:08:52PM +0000, Kieran Bingham wrote:
> On 21/03/2020 18:09, Laurent Pinchart wrote:
> > Replace usage of shared_ptr with unique_ptr to convey media device
> > ownership internally in the enumerators when creating the media device.
> > Once a media device has all its dependencies met, it is converted to a
> > shared_ptr to keep the external API unchanged.
>
> Looks ok, my only worry is that it could become easy to 'forget' to move
> when needed? Or will unique_ptr fail/be loud when we need to move and
> don't ?
The compiler will be loud, brace yourself :-)
> Anyway,
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/device_enumerator.cpp | 10 +++++-----
> > src/libcamera/device_enumerator_sysfs.cpp | 8 ++++----
> > src/libcamera/device_enumerator_udev.cpp | 8 ++++----
> > src/libcamera/include/device_enumerator.h | 4 ++--
> > src/libcamera/include/device_enumerator_sysfs.h | 2 +-
> > src/libcamera/include/device_enumerator_udev.h | 8 ++++----
> > 6 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> > index 89ed67ebb115..dd17e3e32e6c 100644
> > --- a/src/libcamera/device_enumerator.cpp
> > +++ b/src/libcamera/device_enumerator.cpp
> > @@ -208,9 +208,9 @@ DeviceEnumerator::~DeviceEnumerator()
> > *
> > * \return Created media device instance on success, or nullptr otherwise
> > */
> > -std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode)
> > +std::unique_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode)
> > {
> > - std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);
> > + std::unique_ptr<MediaDevice> media = std::make_unique<MediaDevice>(deviceNode);
> >
> > int ret = media->populate();
> > if (ret < 0) {
> > @@ -236,12 +236,12 @@ std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &d
> > * This method shall be called after all members of the entities of the
> > * media graph have been confirmed to be initialized.
> > */
> > -void DeviceEnumerator::addDevice(const std::shared_ptr<MediaDevice> &media)
> > +void DeviceEnumerator::addDevice(std::unique_ptr<MediaDevice> &&media)
> > {
> > LOG(DeviceEnumerator, Debug)
> > << "Added device " << media->deviceNode() << ": " << media->driver();
> >
> > - devices_.push_back(media);
> > + devices_.push_back(std::move(media));
> > }
> >
> > /**
> > @@ -290,7 +290,7 @@ void DeviceEnumerator::removeDevice(const std::string &deviceNode)
> > */
> > std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)
> > {
> > - for (std::shared_ptr<MediaDevice> media : devices_) {
> > + for (std::shared_ptr<MediaDevice> &media : devices_) {
> > if (media->busy())
> > continue;
> >
> > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
> > index 197ca235879b..3446db59e9d4 100644
> > --- a/src/libcamera/device_enumerator_sysfs.cpp
> > +++ b/src/libcamera/device_enumerator_sysfs.cpp
> > @@ -72,11 +72,11 @@ int DeviceEnumeratorSysfs::enumerate()
> > continue;
> > }
> >
> > - std::shared_ptr<MediaDevice> media = createDevice(devnode);
> > + std::unique_ptr<MediaDevice> media = createDevice(devnode);
> > if (!media)
> > continue;
> >
> > - if (populateMediaDevice(media) < 0) {
> > + if (populateMediaDevice(media.get()) < 0) {
> > LOG(DeviceEnumerator, Warning)
> > << "Failed to populate media device "
> > << media->deviceNode()
> > @@ -84,7 +84,7 @@ int DeviceEnumeratorSysfs::enumerate()
> > continue;
> > }
> >
> > - addDevice(media);
> > + addDevice(std::move(media));
> > }
> >
> > closedir(dir);
> > @@ -92,7 +92,7 @@ int DeviceEnumeratorSysfs::enumerate()
> > return 0;
> > }
> >
> > -int DeviceEnumeratorSysfs::populateMediaDevice(const std::shared_ptr<MediaDevice> &media)
> > +int DeviceEnumeratorSysfs::populateMediaDevice(MediaDevice *media)
> > {
> > /* Associate entities to device node paths. */
> > for (MediaEntity *entity : media->entities()) {
> > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> > index ea3f6b7c9ae0..9cbc7e47d2d9 100644
> > --- a/src/libcamera/device_enumerator_udev.cpp
> > +++ b/src/libcamera/device_enumerator_udev.cpp
> > @@ -76,7 +76,7 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
> > return -ENODEV;
> >
> > if (!strcmp(subsystem, "media")) {
> > - std::shared_ptr<MediaDevice> media =
> > + std::unique_ptr<MediaDevice> media =
> > createDevice(udev_device_get_devnode(dev));
> > if (!media)
> > return -ENODEV;
> > @@ -96,7 +96,7 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
> > << "Defer media device " << media->deviceNode()
> > << " due to " << ret << " missing dependencies";
> >
> > - pending_.emplace_back(media, deps);
> > + pending_.emplace_back(std::move(media), std::move(deps));
> > MediaDeviceDeps *mediaDeps = &pending_.back();
> > for (const auto &dep : mediaDeps->deps_)
> > devMap_[dep.first] = mediaDeps;
> > @@ -104,7 +104,7 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
> > return 0;
> > }
> >
> > - addDevice(media);
> > + addDevice(std::move(media));
> > return 0;
> > }
> >
> > @@ -319,7 +319,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
> > LOG(DeviceEnumerator, Debug)
> > << "All dependencies for media device "
> > << deps->media_->deviceNode() << " found";
> > - addDevice(deps->media_);
> > + addDevice(std::move(deps->media_));
> > pending_.remove(*deps);
> > }
> >
> > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
> > index 770f42772270..433e357aebae 100644
> > --- a/src/libcamera/include/device_enumerator.h
> > +++ b/src/libcamera/include/device_enumerator.h
> > @@ -44,8 +44,8 @@ public:
> > std::shared_ptr<MediaDevice> search(const DeviceMatch &dm);
> >
> > protected:
> > - std::shared_ptr<MediaDevice> createDevice(const std::string &deviceNode);
> > - void addDevice(const std::shared_ptr<MediaDevice> &media);
> > + std::unique_ptr<MediaDevice> createDevice(const std::string &deviceNode);
> > + void addDevice(std::unique_ptr<MediaDevice> &&media);
> > void removeDevice(const std::string &deviceNode);
> >
> > private:
> > diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h
> > index 9063f6a75e11..5a5c9b0f5a31 100644
> > --- a/src/libcamera/include/device_enumerator_sysfs.h
> > +++ b/src/libcamera/include/device_enumerator_sysfs.h
> > @@ -23,7 +23,7 @@ public:
> > int enumerate();
> >
> > private:
> > - int populateMediaDevice(const std::shared_ptr<MediaDevice> &media);
> > + int populateMediaDevice(MediaDevice *media);
> > std::string lookupDeviceNode(int major, int minor);
> > };
> >
> > diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h
> > index efaefe5dc4a3..fdce4520f33a 100644
> > --- a/src/libcamera/include/device_enumerator_udev.h
> > +++ b/src/libcamera/include/device_enumerator_udev.h
> > @@ -43,9 +43,9 @@ private:
> > using DependencyMap = std::map<dev_t, std::list<MediaEntity *>>;
> >
> > struct MediaDeviceDeps {
> > - MediaDeviceDeps(const std::shared_ptr<MediaDevice> &media,
> > - const DependencyMap &deps)
> > - : media_(media), deps_(deps)
> > + MediaDeviceDeps(std::unique_ptr<MediaDevice> &&media,
> > + DependencyMap &&deps)
> > + : media_(std::move(media)), deps_(std::move(deps))
> > {
> > }
> >
> > @@ -54,7 +54,7 @@ private:
> > return media_ == other.media_;
> > }
> >
> > - std::shared_ptr<MediaDevice> media_;
> > + std::unique_ptr<MediaDevice> media_;
> > DependencyMap deps_;
> > };
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list