| Message ID | 20190124101651.9993-9-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Niklas a quite small things On Thu, Jan 24, 2019 at 12:16:49PM +0200, Laurent Pinchart wrote: > From: Niklas Söderlund <niklas.soderlund@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@ragnatech.se> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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" ? > + * \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... > + 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@ragnatech.se> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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
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@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@ragnatech.se> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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 >
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@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@ragnatech.se> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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
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@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@ragnatech.se> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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 >
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 + * \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; + + 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