[libcamera-devel] [PATCH 04/10] libcamera: device_enumerator: Reference-count MediaDevice instances
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 25 09:50:24 CET 2019
Hi Laurent,
thanks for the explanation
On Fri, Jan 25, 2019 at 01:55:08AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Jan 25, 2019 at 12:16:41AM +0100, Jacopo Mondi wrote:
> > On Thu, Jan 24, 2019 at 12:16:45PM +0200, Laurent Pinchart wrote:
> > > The MediaDevice class will be the entry point to hot-unplug, as it
> > > corresponds to the kernel devices that will report device removal
> > > events. The class will signal media device disconnection to pipeline
> > > handlers, which will clean up resources as a result.
> > >
> > > This can't be performed synchronously as references may exist to the
> > > related Camera objects in applications. The MediaDevice object thus
> > > needs to be reference-counted in order to support unplugging, as
> > > otherwise pipeline handlers would be required to drop all the references
> > > to the media device they have borrowed synchronously with the
> > > disconnection signal handler, which would be very error prone (if even
> > > possible at all in a sane way).
> > >
> > > Handle MedieDevice instances with std::shared_ptr<> to support this.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > src/libcamera/device_enumerator.cpp | 23 ++++++++++----------
> > > src/libcamera/include/device_enumerator.h | 4 ++--
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 11 ++++------
> > > src/libcamera/pipeline/uvcvideo.cpp | 4 ++--
> > > src/libcamera/pipeline/vimc.cpp | 4 ++--
> > > test/media_device/media_device_link_test.cpp | 2 +-
> > > test/pipeline/ipu3/ipu3_pipeline_test.cpp | 2 +-
> > > 7 files changed, 23 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> > > index 8100972a8d04..149ffbf9aea6 100644
> > > --- a/src/libcamera/device_enumerator.cpp
> > > +++ b/src/libcamera/device_enumerator.cpp
> > > @@ -166,12 +166,10 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()
> > >
> > > DeviceEnumerator::~DeviceEnumerator()
> > > {
> > > - for (MediaDevice *dev : devices_) {
> > > - if (dev->busy())
> > > + for (std::shared_ptr<MediaDevice> media : devices_) {
> > > + if (media->busy())
> >
> > Not sure I got this. The assignement to media in the range-based for
> > loop creates a copy, right? Does the device enumerator need to drop
> > that? They are added to a vector of shared_ptr with
> >
> > devices_.push_back(std::move(media));
> >
> > So I'm expecting this to happen. What have I missed?
>
> It helps to look at how range-based for loops are implemented. The C++
> standard explains they are equivalent to the following code.
>
> {
> auto && __range = range_expression ;
> for (auto __begin = begin_expr, __end = end_expr; __begin != __end; ++__begin) {
> range_declaration = *__begin;
> loop_statement
> }
> }
>
> which, when applied to our code, produces
>
> 1. {
> 2. auto && __range = devices_;
> 3. for (auto __begin = begin(__range), __end = end(__range); __begin != __end; ++__begin) {
> 4. std::shared_ptr<MediaDevice> media = *__begin;
> 5. if (media->busy())
> 6. LOG(DeviceEnumerator, Error)
> 7. << "Removing media device while still in use";
> 8. }
> 9. }
>
> On line 2 a reference to devices_ is created (as an rvalue reference as
> range_expression could be the return of a function, in which case using
> an lvalue would generate an lvalue reference to temporary object error).
> __range thus references devices_, it doesn't create a copy of the
> vector.
>
> On line 3 we use a classic iterator-based loop, with __begin taking the
> type std::vector<std::shared_ptr<MediaDevice>>::iterator.
>
> On line 4 the media object is created as a copy of the vector entry
> being iterated on. This creates a new std::shared_ptr<> instance,
> incrementing the reference count.
>
> On line 8 the media object gets out of scope, and is thus destroyed. The
> reference count is decremented.
>
> The for loop thus doesn't modify the reference count on the objects.
>
Thanks. What I wanted to express was "shouldn't we delete the elements
inside the container" ? But yes, when the object gets deleted, the
vector gets deleted too, as all the elements it contains, dropping the
reference to the shared object.
Thanks for the detailed explanation!
j
> At the end of the destructor all the member variables are destroyed.
> Destroying devices_ will call the destructor of each entry in turn, and
> then destroy the vector itself. The references we added to devices_ in
> DeviceEnumerator::addDevice() are thus dropped at this point, and
> everything is fine. There is thus no need to destroy the devices_
> entries manually. The "delete dev" statement was need because the
> devices_ vector contained pointers to MediaDevice objects that it owned,
> so they had to be deleted.
>
> > > LOG(DeviceEnumerator, Error)
> > > << "Removing media device while still in use";
> > > -
> > > - delete dev;
> > > }
> > > }
> > >
> > > @@ -211,7 +209,7 @@ DeviceEnumerator::~DeviceEnumerator()
> > > */
> > > int DeviceEnumerator::addDevice(const std::string &deviceNode)
> > > {
> > > - MediaDevice *media = new MediaDevice(deviceNode);
> > > + std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);
> > >
> > > int ret = media->open();
> > > if (ret < 0)
> > > @@ -243,9 +241,10 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)
> > > return ret;
> > > }
> > >
> > > - devices_.push_back(media);
> > > media->close();
> > >
> > > + devices_.push_back(std::move(media));
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -260,17 +259,17 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)
> > > *
> > > * \return pointer to the matching MediaDevice, or nullptr if no match is found
> > > */
> > > -MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm)
> > > +std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)
> > > {
> > > - for (MediaDevice *dev : devices_) {
> > > - if (dev->busy())
> > > + for (std::shared_ptr<MediaDevice> media : devices_) {
> > > + if (media->busy())
> > > continue;
> > >
> > > - if (dm.match(dev)) {
> > > + if (dm.match(media.get())) {
> > > LOG(DeviceEnumerator, Debug)
> > > << "Successful match for media device \""
> > > - << dev->driver() << "\"";
> > > - return dev;
> > > + << media->driver() << "\"";
> > > + return std::move(media);
> > > }
> > > }
> > >
> > > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
> > > index 40c7750b49fc..3f87a6255303 100644
> > > --- a/src/libcamera/include/device_enumerator.h
> > > +++ b/src/libcamera/include/device_enumerator.h
> > > @@ -42,13 +42,13 @@ public:
> > > virtual int init() = 0;
> > > virtual int enumerate() = 0;
> > >
> > > - MediaDevice *search(const DeviceMatch &dm);
> > > + std::shared_ptr<MediaDevice> search(const DeviceMatch &dm);
> > >
> > > protected:
> > > int addDevice(const std::string &deviceNode);
> > >
> > > private:
> > > - std::vector<MediaDevice *> devices_;
> > > + std::vector<std::shared_ptr<MediaDevice>> devices_;
> > >
> > > virtual std::string lookupDeviceNode(int major, int minor) = 0;
> > > };
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 13ff7da4c99e..9831f74fe53f 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -29,8 +29,8 @@ public:
> > > bool match(DeviceEnumerator *enumerator);
> > >
> > > private:
> > > - MediaDevice *cio2_;
> > > - MediaDevice *imgu_;
> > > + std::shared_ptr<MediaDevice> cio2_;
> > > + std::shared_ptr<MediaDevice> imgu_;
> > >
> > > void registerCameras();
> > > };
> > > @@ -47,9 +47,6 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()
> > >
> > > if (imgu_)
> > > imgu_->release();
> > > -
> > > - cio2_ = nullptr;
> > > - imgu_ = nullptr;
> > > }
> > >
> > > bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > > @@ -78,11 +75,11 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > > imgu_dm.add("ipu3-imgu 1 viewfinder");
> > > imgu_dm.add("ipu3-imgu 1 3a stat");
> > >
> > > - cio2_ = enumerator->search(cio2_dm);
> > > + cio2_ = std::move(enumerator->search(cio2_dm));
> > > if (!cio2_)
> > > return false;
> > >
> > > - imgu_ = enumerator->search(imgu_dm);
> > > + imgu_ = std::move(enumerator->search(imgu_dm));
> > > if (!imgu_)
> > > return false;
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > > index 3ebc074093ab..73bad6714bb4 100644
> > > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > > @@ -23,7 +23,7 @@ public:
> > > bool match(DeviceEnumerator *enumerator);
> > >
> > > private:
> > > - MediaDevice *dev_;
> > > + std::shared_ptr<MediaDevice> dev_;
> > > };
> > >
> > > PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> > > @@ -41,7 +41,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > > {
> > > DeviceMatch dm("uvcvideo");
> > >
> > > - dev_ = enumerator->search(dm);
> > > + dev_ = std::move(enumerator->search(dm));
> > >
> > > if (!dev_)
> > > return false;
> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > > index 68bfe9b12ab6..521b20d3a120 100644
> > > --- a/src/libcamera/pipeline/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc.cpp
> > > @@ -23,7 +23,7 @@ public:
> > > bool match(DeviceEnumerator *enumerator);
> > >
> > > private:
> > > - MediaDevice *dev_;
> > > + std::shared_ptr<MediaDevice> dev_;
> > > };
> > >
> > > PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager)
> > > @@ -51,7 +51,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
> > > dm.add("RGB/YUV Input");
> > > dm.add("Scaler");
> > >
> > > - dev_ = enumerator->search(dm);
> > > + dev_ = std::move(enumerator->search(dm));
> > > if (!dev_)
> > > return false;
> > >
> > > diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp
> > > index ac5b632f8ed1..58a55cdfee63 100644
> > > --- a/test/media_device/media_device_link_test.cpp
> > > +++ b/test/media_device/media_device_link_test.cpp
> > > @@ -240,7 +240,7 @@ class MediaDeviceLinkTest : public Test
> > >
> > > private:
> > > unique_ptr<DeviceEnumerator> enumerator;
> > > - MediaDevice *dev_;
> > > + shared_ptr<MediaDevice> dev_;
> > > };
> > >
> > > TEST_REGISTER(MediaDeviceLinkTest);
> > > diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> > > index 482c12499a86..953f0233cde4 100644
> > > --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> > > +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> > > @@ -65,7 +65,7 @@ int IPU3PipelineTest::init()
> > > return TestSkip;
> > > }
> > >
> > > - MediaDevice *cio2 = enumerator->search(cio2_dm);
> > > + std::shared_ptr<MediaDevice> cio2 = enumerator->search(cio2_dm);
> > > if (!cio2) {
> > > cerr << "Failed to find IPU3 CIO2: test skip" << endl;
> > > return TestSkip;
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190125/547c23e1/attachment-0001.sig>
More information about the libcamera-devel
mailing list