[libcamera-devel] [PATCH 08/10] libcamera: pipeline_handler: Add camera disconnection support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 25 11:43:07 CET 2019
Hi Kieran,
On Fri, Jan 25, 2019 at 10:22:56AM +0000, Kieran Bingham wrote:
> On 24/01/2019 23:59, Laurent Pinchart wrote:
> > On Fri, Jan 25, 2019 at 12:24:49AM +0100, Jacopo Mondi wrote:
> >> On Thu, Jan 24, 2019 at 12:16:49PM +0200, Laurent Pinchart wrote:
> >>> From: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>>
> >>> Pipeline handlers are responsible for creating camera instances, but
> >>> also for destroying them when devices are unplugged. As camera objects
> >>> are reference-counted this isn't a straightforward operation and
> >>> involves the camera manager and camera object itself. Add two helper
> >>> methods in the PipelineHandler base class to register a camera and to
> >>> register a media device with the pipeline handler.
> >>>
> >>> When registering a camera, the registerCamera() helper method will add
> >>> it to the camera manager. When registering a media device, the
> >>> registerMediaDevice() helper method will listen to device disconnection
> >>> events, and disconnect all cameras created by the pipeline handler as a
> >>> response.
> >>>
> >>> Under the hood the PipelineHandler class needs to keep track of
> >>> registered cameras in order to handle disconnection. They can't be
> >>> stored as shared pointers as this would create a circular dependency
> >>> (the Camera class owns a shared pointer to the pipeline handler). Store
> >>> them as weak pointers instead. This is safe as a reference to the camera
> >>> is stored in the camera manager, and doesn't get removed until the
> >>> camera is unregistered from the manager by the PipelineHandler.
> >>>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>> src/libcamera/include/pipeline_handler.h | 10 ++++
> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-
> >>> src/libcamera/pipeline/uvcvideo.cpp | 3 +-
> >>> src/libcamera/pipeline/vimc.cpp | 3 +-
> >>> src/libcamera/pipeline_handler.cpp | 71 ++++++++++++++++++++++++
> >>> 5 files changed, 84 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> >>> index e1d6369eb0c4..804cce4807ee 100644
> >>> --- a/src/libcamera/include/pipeline_handler.h
> >>> +++ b/src/libcamera/include/pipeline_handler.h
> >>> @@ -16,6 +16,7 @@ namespace libcamera {
> >>>
> >>> class CameraManager;
> >>> class DeviceEnumerator;
> >>> +class MediaDevice;
> >>>
> >>> class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>
> >>> {
> >>> @@ -27,6 +28,15 @@ public:
> >>>
> >>> protected:
> >>> CameraManager *manager_;
> >>> +
> >>> + void registerCamera(std::shared_ptr<Camera> camera);
> >>> + void hotplugMediaDevice(MediaDevice *media);
> >>> +
> >>> +private:
> >>> + virtual void disconnect();
> >>> + void mediaDeviceDisconnected(MediaDevice *media);
> >>> +
> >>> + std::vector<std::weak_ptr<Camera>> cameras_;
> >>> };
> >>>
> >>> class PipelineHandlerFactory
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index 9831f74fe53f..3161e71420ed 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -9,7 +9,6 @@
> >>> #include <vector>
> >>>
> >>> #include <libcamera/camera.h>
> >>> -#include <libcamera/camera_manager.h>
> >>>
> >>> #include "device_enumerator.h"
> >>> #include "log.h"
> >>> @@ -169,7 +168,7 @@ void PipelineHandlerIPU3::registerCameras()
> >>>
> >>> std::string cameraName = sensor->name() + " " + std::to_string(id);
> >>> std::shared_ptr<Camera> camera = Camera::create(this, cameraName);
> >>> - manager_->addCamera(std::move(camera));
> >>> + registerCamera(std::move(camera));
> >>>
> >>> LOG(IPU3, Info)
> >>> << "Registered Camera[" << numCameras << "] \""
> >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> >>> index 73bad6714bb4..c8f1bf553bfe 100644
> >>> --- a/src/libcamera/pipeline/uvcvideo.cpp
> >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> >>> @@ -6,7 +6,6 @@
> >>> */
> >>>
> >>> #include <libcamera/camera.h>
> >>> -#include <libcamera/camera_manager.h>
> >>>
> >>> #include "device_enumerator.h"
> >>> #include "media_device.h"
> >>> @@ -49,7 +48,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >>> dev_->acquire();
> >>>
> >>> std::shared_ptr<Camera> camera = Camera::create(this, dev_->model());
> >>> - manager_->addCamera(std::move(camera));
> >>> + registerCamera(std::move(camera));
> >>>
> >>> return true;
> >>> }
> >>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> >>> index 521b20d3a120..b714a07688e9 100644
> >>> --- a/src/libcamera/pipeline/vimc.cpp
> >>> +++ b/src/libcamera/pipeline/vimc.cpp
> >>> @@ -6,7 +6,6 @@
> >>> */
> >>>
> >>> #include <libcamera/camera.h>
> >>> -#include <libcamera/camera_manager.h>
> >>>
> >>> #include "device_enumerator.h"
> >>> #include "media_device.h"
> >>> @@ -58,7 +57,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
> >>> dev_->acquire();
> >>>
> >>> std::shared_ptr<Camera> camera = Camera::create(this, "Dummy VIMC Camera");
> >>> - manager_->addCamera(std::move(camera));
> >>> + registerCamera(std::move(camera));
> >>>
> >>> return true;
> >>> }
> >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >>> index 3850ea8fadb5..f0aa2f8022c2 100644
> >>> --- a/src/libcamera/pipeline_handler.cpp
> >>> +++ b/src/libcamera/pipeline_handler.cpp
> >>> @@ -5,7 +5,11 @@
> >>> * pipeline_handler.cpp - Pipeline handler infrastructure
> >>> */
> >>>
> >>> +#include <libcamera/camera.h>
> >>> +#include <libcamera/camera_manager.h>
> >>> +
> >>> #include "log.h"
> >>> +#include "media_device.h"
> >>> #include "pipeline_handler.h"
> >>>
> >>> /**
> >>> @@ -97,6 +101,73 @@ PipelineHandler::~PipelineHandler()
> >>> * constant for the whole lifetime of the pipeline handler.
> >>> */
> >>>
> >>> +/**
> >>> + * \brief Register a camera to the camera manager and pipeline handler
> >>> + * \param[in] camera The camera to be added
> >>> + *
> >>> + * This function is called by pipeline handlers to register the cameras they
> >>> + * handle with the camera manager.
> >>> + */
> >>> +void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> >>> +{
> >>> + cameras_.push_back(camera);
> >>> + manager_->addCamera(std::move(camera));
> >>> +}
> >>> +
> >>> +/**
> >>> + * \brief Handle hotplugging of a media device
> >>
> >> "Handle" here misleaded me. What about "Enable" ?
> >
> > I'll change this to "Enable hotplug handling for a media device".
>
> To me 'hotplugging' means supporting both connect and disconnect.
>
> This function doesn't do anything regarding connect - it's just
> supporting device removal events, so the naming does seem further
> misleading.
>
> how about s/hotplugMediaDevice/registerDisconnect/ ? (and similar if
> 'hotplug' is used on other objects too)
I'm of two minds about this. I agree the naming isn't ideal, but
registerDisconnect sounds more cryptic to me. The name
hotplugMediaDevice at least refers to the hotplug mechanism, and could
also be understood as registering a media device that has been
hotplugged.
I expect this API to evolve as I'd like the acquire/release of media
devices to be handled automatically (it is otherwise quite error-prone
for pipeline handler developers). How about revisiting the function then
?
> >>> + * \param[in] media The media device
> >>> + *
> >>> + * This function enables hotplug handling, and especially hot-unplug handling,
> >>> + * of the \a media device. It shall be called by pipeline handlers for all the
> >>> + * media devices that can be disconnected.
> >>> + *
> >>> + * When a media device passed to this function is later unplugged, the pipeline
> >>> + * handler gets notified and automatically disconnects all the cameras it has
> >>> + * registered without requiring any manual intervention.
> >>> + */
> >>> +void PipelineHandler::hotplugMediaDevice(MediaDevice *media)
> >>> +{
> >>> + media->disconnected.connect(this, &PipelineHandler::mediaDeviceDisconnected);
> >>> +}
> >>> +
> >>> +/**
> >>> + * \brief Device disconnection handler
> >>> + *
> >>> + * This virtual function is called to notify the pipeline handler that the
> >>> + * device it handles has been disconnected. It notifies all cameras created by
> >>> + * the pipeline handler that they have been disconnected, and unregisters them
> >>> + * from the camera manager.
> >>> + *
> >>> + * The function can be overloaded by pipeline handlers to perform custom
> >>> + * operations at disconnection time. Any overloaded version shall call the
> >>> + * PipelineHandler::disconnect() base function for proper hot-unplug operation.
> >>> + */
> >>> +void PipelineHandler::disconnect()
> >>> +{
> >>> + for (std::weak_ptr<Camera> ptr : cameras_) {
> >>> + std::shared_ptr<Camera> camera = ptr.lock();
> >>> + if (!camera)
> >>> + continue;
> >>> +
> >>
> >> I wonder if sub-classes need a disconnectCamera(*camera) to perform
> >> per-camera operations before disconnect...
> >
> > It's a good question, and I don't have answers yet I'm afraid. I think
> > we should add that later if it turns out to be needed. Note that
> > subclasses could always connect a slot to the camera disconnected
> > signal, but that would probably be a bit cumbersome, a virtual function
> > would likely be better.
> >
> >>> + camera->disconnect();
> >>> + manager_->removeCamera(camera.get());
> >>> + }
> >>> +
> >>> + cameras_.clear();
> >>> +}
> >>> +
> >>> +/**
> >>> + * \brief Slot for the MediaDevice disconnected signal
> >>> + */
> >>> +void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
> >>> +{
> >>> + if (cameras_.empty())
> >>> + return;
> >>> +
> >>> + disconnect();
> >>> +}
> >>> +
> >>> /**
> >>> * \class PipelineHandlerFactory
> >>> * \brief Registration of PipelineHandler classes and creation of instances
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list