[libcamera-devel] [PATCH 08/10] libcamera: pipeline_handler: Add camera disconnection support

[Laurent Pinchart]

Hi Jacopo,

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".

> > + * \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