[libcamera-devel] [RFC 06/11] libcamera: pipeline_handler: Keep track of MediaDevice
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Apr 17 12:49:15 CEST 2019
Hi Niklas,
Thank you for the patch.
On Sun, Apr 14, 2019 at 03:35:01AM +0200, Niklas Söderlund wrote:
> Instead of requiring each pipeline handle implementation to keep track
> of calling release() on its media devices upon deletion keep track of
> them in the base class. Add a helper which pipeline handlers shall use
> to acquire a media device instead of directly interacting with the
> DeviceEnumerator.
>
> Centrally keeping track of media devices will also be beneficial
> implementing pipeline exclusive access across processes.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/include/pipeline_handler.h | 4 ++++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++------------------
> src/libcamera/pipeline/uvcvideo.cpp | 24 +++++++-----------------
> src/libcamera/pipeline/vimc.cpp | 22 ++++++----------------
> src/libcamera/pipeline_handler.cpp | 22 ++++++++++++++++++++++
> 5 files changed, 41 insertions(+), 51 deletions(-)
>
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index b6cbd3bae51b08dc..d995a7e56d90f706 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -21,6 +21,7 @@ class Camera;
> class CameraConfiguration;
> class CameraManager;
> class DeviceEnumerator;
> +class DeviceMatch;
> class MediaDevice;
> class PipelineHandler;
> class Request;
> @@ -52,6 +53,8 @@ public:
> virtual ~PipelineHandler();
>
> virtual bool match(DeviceEnumerator *enumerator) = 0;
> + std::shared_ptr<MediaDevice> tryAcquire(DeviceEnumerator *enumerator,
> + const DeviceMatch &dm);
Given that this function stores a reference internally in
PipelineHandler, which is guaranteed to remain valid until the pipeline
handler is destroyed, can't we return a MediaDevice * instead of a
shared pointer ?
>
> virtual CameraConfiguration
> streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
> @@ -81,6 +84,7 @@ private:
> void mediaDeviceDisconnected(MediaDevice *media);
> virtual void disconnect();
>
> + std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
> std::vector<std::weak_ptr<Camera>> cameras_;
> std::map<const Camera *, std::unique_ptr<CameraData>> cameraData_;
> };
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index a87eff8dfec6d143..77d14533e6bceb80 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -137,7 +137,6 @@ class PipelineHandlerIPU3 : public PipelineHandler
> {
> public:
> PipelineHandlerIPU3(CameraManager *manager);
> - ~PipelineHandlerIPU3();
>
> CameraConfiguration
> streamConfiguration(Camera *camera,
> @@ -195,15 +194,6 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> {
> }
>
> -PipelineHandlerIPU3::~PipelineHandlerIPU3()
> -{
> - if (cio2MediaDev_)
> - cio2MediaDev_->release();
> -
> - if (imguMediaDev_)
> - imguMediaDev_->release();
> -}
> -
> CameraConfiguration
> PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> const std::vector<StreamUsage> &usages)
> @@ -451,20 +441,14 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> imgu_dm.add("ipu3-imgu 1 viewfinder");
> imgu_dm.add("ipu3-imgu 1 3a stat");
>
> - cio2MediaDev_ = enumerator->search(cio2_dm);
> + cio2MediaDev_ = tryAcquire(enumerator, cio2_dm);
> if (!cio2MediaDev_)
> return false;
>
> - if (!cio2MediaDev_->acquire())
> - return false;
> -
> - imguMediaDev_ = enumerator->search(imgu_dm);
> + imguMediaDev_ = tryAcquire(enumerator, imgu_dm);
> if (!imguMediaDev_)
> return false;
>
> - if (!imguMediaDev_->acquire())
> - return false;
> -
> /*
> * Disable all links that are enabled by default on CIO2, as camera
> * creation enables all valid links it finds.
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 5214bfd3097b8217..43a6d59670299376 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -24,7 +24,6 @@ class PipelineHandlerUVC : public PipelineHandler
> {
> public:
> PipelineHandlerUVC(CameraManager *manager);
> - ~PipelineHandlerUVC();
>
> CameraConfiguration
> streamConfiguration(Camera *camera,
> @@ -68,20 +67,13 @@ private:
> PipelineHandler::cameraData(camera));
> }
>
> - std::shared_ptr<MediaDevice> media_;
> };
>
> PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> - : PipelineHandler(manager), media_(nullptr)
> + : PipelineHandler(manager)
> {
> }
>
> -PipelineHandlerUVC::~PipelineHandlerUVC()
> -{
> - if (media_)
> - media_->release();
> -}
> -
> CameraConfiguration
> PipelineHandlerUVC::streamConfiguration(Camera *camera,
> const std::vector<StreamUsage> &usages)
> @@ -178,19 +170,17 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>
> bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> {
> + std::shared_ptr<MediaDevice> media;
> DeviceMatch dm("uvcvideo");
>
> - media_ = enumerator->search(dm);
> - if (!media_)
> - return false;
> -
> - if (!media_->acquire())
> + media = tryAcquire(enumerator, dm);
> + if (!media)
> return false;
>
> std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);
>
> /* Locate and open the default video node. */
> - for (MediaEntity *entity : media_->entities()) {
> + for (MediaEntity *entity : media->entities()) {
> if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> data->video_ = new V4L2Device(entity);
> break;
> @@ -209,11 +199,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>
> /* Create and register the camera. */
> std::set<Stream *> streams{ &data->stream_ };
> - std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
> + std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> registerCamera(std::move(camera), std::move(data));
>
> /* Enable hot-unplug notifications. */
> - hotplugMediaDevice(media_.get());
> + hotplugMediaDevice(media.get());
>
> return true;
> }
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index e5e78ccedd59ae66..a80331e1cd4d1e46 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -24,7 +24,6 @@ class PipelineHandlerVimc : public PipelineHandler
> {
> public:
> PipelineHandlerVimc(CameraManager *manager);
> - ~PipelineHandlerVimc();
>
> CameraConfiguration
> streamConfiguration(Camera *camera,
> @@ -67,21 +66,13 @@ private:
> return static_cast<VimcCameraData *>(
> PipelineHandler::cameraData(camera));
> }
> -
> - std::shared_ptr<MediaDevice> media_;
> };
>
> PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> - : PipelineHandler(manager), media_(nullptr)
> + : PipelineHandler(manager)
> {
> }
>
> -PipelineHandlerVimc::~PipelineHandlerVimc()
> -{
> - if (media_)
> - media_->release();
> -}
> -
> CameraConfiguration
> PipelineHandlerVimc::streamConfiguration(Camera *camera,
> const std::vector<StreamUsage> &usages)
> @@ -178,6 +169,8 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
>
> bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> {
> + std::shared_ptr<MediaDevice> media;
> +
> DeviceMatch dm("vimc");
>
> dm.add("Raw Capture 0");
> @@ -190,17 +183,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> dm.add("RGB/YUV Input");
> dm.add("Scaler");
>
> - media_ = enumerator->search(dm);
> - if (!media_)
> - return false;
> -
> - if (!media_->acquire())
> + media = tryAcquire(enumerator, dm);
> + if (!media)
> return false;
>
> std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
>
> /* Locate and open the capture video node. */
> - data->video_ = new V4L2Device(media_->getEntityByName("Raw Capture 1"));
> + data->video_ = new V4L2Device(media->getEntityByName("Raw Capture 1"));
> if (data->video_->open())
> return false;
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 43550c0e0210b7b3..7f4035c008f95f91 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -9,6 +9,7 @@
> #include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
>
> +#include "device_enumerator.h"
> #include "log.h"
> #include "media_device.h"
> #include "pipeline_handler.h"
> @@ -115,6 +116,8 @@ PipelineHandler::PipelineHandler(CameraManager *manager)
>
> PipelineHandler::~PipelineHandler()
> {
> + for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> + media->release();
> };
>
> /**
> @@ -148,6 +151,25 @@ PipelineHandler::~PipelineHandler()
> * created, or false otherwise
> */
>
> +std::shared_ptr<MediaDevice>
> +PipelineHandler::tryAcquire(DeviceEnumerator *enumerator, const DeviceMatch &dm)
Missing documentation.
> +{
> + std::shared_ptr<MediaDevice> media;
> +
> + media = enumerator->search(dm);
> + if (!media)
> + goto out;
> +
> + if (!media->acquire()) {
> + media.reset();
> + goto out;
> + }
> +
> + mediaDevices_.push_back(media);
> +out:
> + return media;
> +}
> +
> /**
> * \fn PipelineHandler::streamConfiguration()
> * \brief Retrieve a group of stream configurations for a specified camera
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list