[libcamera-devel] [PATCH 08/10] libcamera: pipeline_handler: Add camera disconnection support
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jan 25 11:50:59 CET 2019
Hi Laurent,
On 25/01/2019 10:43, Laurent Pinchart wrote:
> 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.
Otherwise, registerDisconnectHandler(MediaDevice)?
> 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
> ?
As you wish,
--
Kieran
>>>>> + * \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
More information about the libcamera-devel
mailing list