| Message ID | 20200513172950.72685-4-email@uajain.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 13/05/2020 18:29, Umang Jain wrote: > Emit 'cameraAdded' and 'cameraRemoved' from CameraManager to enable > hotplug and hot-unplug support in appplications like QCam. s/appplications/applications/ Hrm ... seeing cameraAdded and cameraRemoved, makes me think back to my earlier signal name bikeshedding ;-) Perhaps that one should be deviceAdded and deviceRemoved (was newDevicesFound?) Anyway, let the bikeshedding happen on that patch instead ;-) > Signed-off-by: Umang Jain <email@uajain.com> > --- > include/libcamera/camera_manager.h | 6 +++++- > src/libcamera/camera_manager.cpp | 34 +++++++++++++++++++++++++++--- > src/libcamera/pipeline_handler.cpp | 2 +- > 3 files changed, 37 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 079f848..366fa07 100644 > --- a/include/libcamera/camera_manager.h > +++ b/include/libcamera/camera_manager.h > @@ -13,6 +13,7 @@ > #include <vector> > > #include <libcamera/object.h> > +#include <libcamera/signal.h> > > namespace libcamera { > > @@ -35,13 +36,16 @@ public: > std::shared_ptr<Camera> get(dev_t devnum); > > void addCamera(std::shared_ptr<Camera> camera, dev_t devnum); > - void removeCamera(Camera *camera); > + void removeCamera(std::shared_ptr<Camera> camera); > > static const std::string &version() { return version_; } > > void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); > EventDispatcher *eventDispatcher(); > > + Signal<std::shared_ptr<Camera>> cameraAdded; > + Signal<std::shared_ptr<Camera>> cameraRemoved; > + > private: > static const std::string version_; > static CameraManager *self_; > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index a13cfe1..b9d2496 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -186,7 +186,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, > } > } > > - cameras_.push_back(std::move(camera)); > + cameras_.push_back(camera); > > if (devnum) { > unsigned int index = cameras_.size() - 1; > @@ -376,6 +376,32 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum) > return iter->second.lock(); > } > > +/** > + * \brief Notify of a new camera added to the system Missing the \var to reference the signal. > + * > + * This signal is emitted when a new camera is detected and successfully handled > + * by the camera manager. The notification occurs alike for cameras detected > + * when the manager is started with start() or when new cameras are later > + * connected to the system. When the signal is emitted the new camera is already > + * available from the list of cameras(). > + * > + * The signal is emitted from the CameraManager thread. Applications shall > + * minimize the time spent in the signal handler and shall in particular not > + * perform any blocking operation. > + */ > + > +/** And missing here too. > + * \brief Notify of a new camera removed from the system > + * > + * This signal is emitted when a camera is removed from the system. When the > + * signal is emitted the camera is not available from the list of cameras() > + * anymore. > + * > + * The signal is emitted from the CameraManager thread. Applications shall > + * minimize the time spent in the signal handler and shall in particular not > + * perform any blocking operation. > + */ > + > /** > * \brief Add a camera to the camera manager > * \param[in] camera The camera to be added > @@ -395,6 +421,7 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum) > ASSERT(Thread::current() == p_.get()); > > p_->addCamera(camera, devnum); > + cameraAdded.emit(camera); > } > > /** > @@ -407,11 +434,12 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum) > * > * \context This function shall be called from the CameraManager thread. > */ > -void CameraManager::removeCamera(Camera *camera) > +void CameraManager::removeCamera(std::shared_ptr<Camera> camera) > { > ASSERT(Thread::current() == p_.get()); > > - p_->removeCamera(camera); > + p_->removeCamera(camera.get()); > + cameraRemoved.emit(camera); Is there any race here, or potential problem emitting the signal with the camera, after it's been removed from the CameraManager? (I hope/expect not as it's using a shared_ptr ... But I haven't checked to see if there are any implications caused by the removeCamera call) > } > > /** > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 254d341..a9187e1 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -555,7 +555,7 @@ void PipelineHandler::disconnect() > continue; > > camera->disconnect(); > - manager_->removeCamera(camera.get()); > + manager_->removeCamera(camera); > } > > cameras_.clear(); >
Hi Kieran, On Mon, 2020-05-18 at 16:44 +0100, Kieran Bingham wrote: > Hi Umang, > > On 13/05/2020 18:29, Umang Jain wrote: > > Emit 'cameraAdded' and 'cameraRemoved' from CameraManager to enable > > hotplug and hot-unplug support in appplications like QCam. > > s/appplications/applications/ > > > Hrm ... seeing cameraAdded and cameraRemoved, makes me think back to > my > earlier signal name bikeshedding ;-) Perhaps that one should be > deviceAdded and deviceRemoved (was newDevicesFound?) > > Anyway, let the bikeshedding happen on that patch instead ;-) The DeviceEnumerator::newDevicesFound is somewhat internal to libcamera (i.e. not meant to be used by applications - only by CameraManager). 'cameraAdded' and 'cameraRemoved' are application facing signal-probes that denote hotplugging. Maybe using 'newCameraAdded' and 'cameraRemoved' for CameraManager will make more things clear? As far as DeviceEnumerator:newDevicesFound is concerned, I think: s/newDeviceFound/deviceAdded/ should do too. > > > Signed-off-by: Umang Jain <email@uajain.com> > > --- > > include/libcamera/camera_manager.h | 6 +++++- > > src/libcamera/camera_manager.cpp | 34 > > +++++++++++++++++++++++++++--- > > src/libcamera/pipeline_handler.cpp | 2 +- > > 3 files changed, 37 insertions(+), 5 deletions(-) > > > > diff --git a/include/libcamera/camera_manager.h > > b/include/libcamera/camera_manager.h > > index 079f848..366fa07 100644 > > --- a/include/libcamera/camera_manager.h > > +++ b/include/libcamera/camera_manager.h > > @@ -13,6 +13,7 @@ > > #include <vector> > > > > #include <libcamera/object.h> > > +#include <libcamera/signal.h> > > > > namespace libcamera { > > > > @@ -35,13 +36,16 @@ public: > > std::shared_ptr<Camera> get(dev_t devnum); > > > > void addCamera(std::shared_ptr<Camera> camera, dev_t devnum); > > - void removeCamera(Camera *camera); > > + void removeCamera(std::shared_ptr<Camera> camera); > > > > static const std::string &version() { return version_; } > > > > void setEventDispatcher(std::unique_ptr<EventDispatcher> > > dispatcher); > > EventDispatcher *eventDispatcher(); > > > > + Signal<std::shared_ptr<Camera>> cameraAdded; > > + Signal<std::shared_ptr<Camera>> cameraRemoved; > > + > > private: > > static const std::string version_; > > static CameraManager *self_; > > diff --git a/src/libcamera/camera_manager.cpp > > b/src/libcamera/camera_manager.cpp > > index a13cfe1..b9d2496 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -186,7 +186,7 @@ void > > CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, > > } > > } > > > > - cameras_.push_back(std::move(camera)); > > + cameras_.push_back(camera); > > > > if (devnum) { > > unsigned int index = cameras_.size() - 1; > > @@ -376,6 +376,32 @@ std::shared_ptr<Camera> > > CameraManager::get(dev_t devnum) > > return iter->second.lock(); > > } > > > > +/** > > + * \brief Notify of a new camera added to the system > > Missing the \var to reference the signal. > > > + * > > + * This signal is emitted when a new camera is detected and > > successfully handled > > + * by the camera manager. The notification occurs alike for > > cameras detected > > + * when the manager is started with start() or when new cameras > > are later > > + * connected to the system. When the signal is emitted the new > > camera is already > > + * available from the list of cameras(). > > + * > > + * The signal is emitted from the CameraManager thread. > > Applications shall > > + * minimize the time spent in the signal handler and shall in > > particular not > > + * perform any blocking operation. > > + */ > > + > > +/** > > And missing here too. > > > + * \brief Notify of a new camera removed from the system > > + * > > + * This signal is emitted when a camera is removed from the > > system. When the > > + * signal is emitted the camera is not available from the list of > > cameras() > > + * anymore. > > + * > > + * The signal is emitted from the CameraManager thread. > > Applications shall > > + * minimize the time spent in the signal handler and shall in > > particular not > > + * perform any blocking operation. > > + */ > > + > > /** > > * \brief Add a camera to the camera manager > > * \param[in] camera The camera to be added > > @@ -395,6 +421,7 @@ void > > CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t > > devnum) > > ASSERT(Thread::current() == p_.get()); > > > > p_->addCamera(camera, devnum); > > + cameraAdded.emit(camera); > > } > > > > /** > > @@ -407,11 +434,12 @@ void > > CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t > > devnum) > > * > > * \context This function shall be called from the CameraManager > > thread. > > */ > > -void CameraManager::removeCamera(Camera *camera) > > +void CameraManager::removeCamera(std::shared_ptr<Camera> camera) > > { > > ASSERT(Thread::current() == p_.get()); > > > > - p_->removeCamera(camera); > > + p_->removeCamera(camera.get()); > > + cameraRemoved.emit(camera); > > Is there any race here, or potential problem emitting the signal with > the camera, after it's been removed from the CameraManager? (I > hope/expect not as it's using a shared_ptr ... But I haven't checked > to > see if there are any implications caused by the removeCamera call) > > > > } > > > > /** > > diff --git a/src/libcamera/pipeline_handler.cpp > > b/src/libcamera/pipeline_handler.cpp > > index 254d341..a9187e1 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -555,7 +555,7 @@ void PipelineHandler::disconnect() > > continue; > > > > camera->disconnect(); > > - manager_->removeCamera(camera.get()); > > + manager_->removeCamera(camera); > > } > > > > cameras_.clear(); > >
diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 079f848..366fa07 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -13,6 +13,7 @@ #include <vector> #include <libcamera/object.h> +#include <libcamera/signal.h> namespace libcamera { @@ -35,13 +36,16 @@ public: std::shared_ptr<Camera> get(dev_t devnum); void addCamera(std::shared_ptr<Camera> camera, dev_t devnum); - void removeCamera(Camera *camera); + void removeCamera(std::shared_ptr<Camera> camera); static const std::string &version() { return version_; } void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); EventDispatcher *eventDispatcher(); + Signal<std::shared_ptr<Camera>> cameraAdded; + Signal<std::shared_ptr<Camera>> cameraRemoved; + private: static const std::string version_; static CameraManager *self_; diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index a13cfe1..b9d2496 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -186,7 +186,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, } } - cameras_.push_back(std::move(camera)); + cameras_.push_back(camera); if (devnum) { unsigned int index = cameras_.size() - 1; @@ -376,6 +376,32 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum) return iter->second.lock(); } +/** + * \brief Notify of a new camera added to the system + * + * This signal is emitted when a new camera is detected and successfully handled + * by the camera manager. The notification occurs alike for cameras detected + * when the manager is started with start() or when new cameras are later + * connected to the system. When the signal is emitted the new camera is already + * available from the list of cameras(). + * + * The signal is emitted from the CameraManager thread. Applications shall + * minimize the time spent in the signal handler and shall in particular not + * perform any blocking operation. + */ + +/** + * \brief Notify of a new camera removed from the system + * + * This signal is emitted when a camera is removed from the system. When the + * signal is emitted the camera is not available from the list of cameras() + * anymore. + * + * The signal is emitted from the CameraManager thread. Applications shall + * minimize the time spent in the signal handler and shall in particular not + * perform any blocking operation. + */ + /** * \brief Add a camera to the camera manager * \param[in] camera The camera to be added @@ -395,6 +421,7 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum) ASSERT(Thread::current() == p_.get()); p_->addCamera(camera, devnum); + cameraAdded.emit(camera); } /** @@ -407,11 +434,12 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum) * * \context This function shall be called from the CameraManager thread. */ -void CameraManager::removeCamera(Camera *camera) +void CameraManager::removeCamera(std::shared_ptr<Camera> camera) { ASSERT(Thread::current() == p_.get()); - p_->removeCamera(camera); + p_->removeCamera(camera.get()); + cameraRemoved.emit(camera); } /** diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 254d341..a9187e1 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -555,7 +555,7 @@ void PipelineHandler::disconnect() continue; camera->disconnect(); - manager_->removeCamera(camera.get()); + manager_->removeCamera(camera); } cameras_.clear();
Emit 'cameraAdded' and 'cameraRemoved' from CameraManager to enable hotplug and hot-unplug support in appplications like QCam. Signed-off-by: Umang Jain <email@uajain.com> --- include/libcamera/camera_manager.h | 6 +++++- src/libcamera/camera_manager.cpp | 34 +++++++++++++++++++++++++++--- src/libcamera/pipeline_handler.cpp | 2 +- 3 files changed, 37 insertions(+), 5 deletions(-)