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

[Kieran Bingham]

Hi All,

On 24/01/2019 23:59, Laurent Pinchart wrote:
> 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".

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)



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