[libcamera-devel] [PATCH 04/10] libcamera: device_enumerator: Reference-count MediaDevice instances
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 25 00:16:41 CET 2019
Hi Laurent,
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?
Thanks
j
> 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/9e9d15e6/attachment-0001.sig>
More information about the libcamera-devel
mailing list