| Message ID | 20200506103346.3433-3-email@uajain.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Wed, May 06, 2020 at 10:33:53AM +0000, Umang Jain wrote: > 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 | 4 ++++ > src/libcamera/camera_manager.cpp | 20 ++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 079f848..558bb96 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 { > > @@ -42,6 +43,9 @@ public: > void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); > EventDispatcher *eventDispatcher(); > > + Signal<Camera *> cameraAdded; > + Signal<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 c75979a..6438f87 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -391,6 +391,23 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum) > return iter->second.lock(); > } > > +/** > + * \var CameraManager::cameraAdded > + * \brief Signal emitted when a new camera is added in CameraManager > + * > + * This signal is emitted when a new camera is added by the CameraManager > + * in the list of cameras it manages. A pointer to the newly-added camera > + * is passed as a parameter. I think we should detail here the relationship with CameraManager::start() and CameraManager::cameras(), and note that the signal handler should be fast (otherwise it would block operation of other cameras). How about the following ? * \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. > + */ > + > +/** > + * \var CameraManager::cameraRemoved > + * \brief Signal emitted when a camera is removed in CameraManager > + * > + * This signal is emitted when a camera is removed from the CameraManager. > + * A pointer to the removed camera is passed as a parameter. And something similar here ? * \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 > @@ -409,7 +426,9 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum) > { > ASSERT(Thread::current() == p_.get()); > > + Camera *cam = camera.get(); > p_->addCamera(camera, devnum); > + cameraAdded.emit(cam); You don't need a local cam variable as camera is a shared pointer, not a unique pointer (and if it was a unique pointer and was given to addCamera() with std::move, it would be potentially unsafe to use it afterwards). cameraAdded.emit(camera.get()); is fine. > } > > /** > @@ -427,6 +446,7 @@ void CameraManager::removeCamera(Camera *camera) > ASSERT(Thread::current() == p_.get()); > > p_->removeCamera(camera); > + cameraRemoved.emit(camera); > } > > /**
Hi Laurent, Thanks for the feedback. On Wed, May 6, 2020 at 23:53, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Umang, > > Thank you for the patch. > > On Wed, May 06, 2020 at 10:33:53AM +0000, Umang Jain wrote: >> 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 >> <mailto:email@uajain.com>> >> --- >> include/libcamera/camera_manager.h | 4 ++++ >> src/libcamera/camera_manager.cpp | 20 ++++++++++++++++++++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/include/libcamera/camera_manager.h >> b/include/libcamera/camera_manager.h >> index 079f848..558bb96 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 { >> >> @@ -42,6 +43,9 @@ public: >> void setEventDispatcher(std::unique_ptr<EventDispatcher> >> dispatcher); >> EventDispatcher *eventDispatcher(); >> >> + Signal<Camera *> cameraAdded; >> + Signal<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 c75979a..6438f87 100644 >> --- a/src/libcamera/camera_manager.cpp >> +++ b/src/libcamera/camera_manager.cpp >> @@ -391,6 +391,23 @@ std::shared_ptr<Camera> >> CameraManager::get(dev_t devnum) >> return iter->second.lock(); >> } >> >> +/** >> + * \var CameraManager::cameraAdded >> + * \brief Signal emitted when a new camera is added in >> CameraManager >> + * >> + * This signal is emitted when a new camera is added by the >> CameraManager >> + * in the list of cameras it manages. A pointer to the newly-added >> camera >> + * is passed as a parameter. > > I think we should detail here the relationship with > CameraManager::start() and CameraManager::cameras(), and note that the > signal handler should be fast (otherwise it would block operation of > other cameras). How about the following ? > > * \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. > >> + */ >> + >> +/** >> + * \var CameraManager::cameraRemoved >> + * \brief Signal emitted when a camera is removed in CameraManager >> + * >> + * This signal is emitted when a camera is removed from the >> CameraManager. >> + * A pointer to the removed camera is passed as a parameter. > > And something similar here ? > > * \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 >> @@ -409,7 +426,9 @@ void >> CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t >> devnum) >> { >> ASSERT(Thread::current() == p_.get()); >> >> + Camera *cam = camera.get(); >> p_->addCamera(camera, devnum); >> + cameraAdded.emit(cam); > > You don't need a local cam variable as camera is a shared pointer, > not a > unique pointer (and if it was a unique pointer and was given to > addCamera() with std::move, it would be potentially unsafe to use it > afterwards). > > cameraAdded.emit(camera.get()); > > is fine. Yes, camera is a shared pointer here and is std::move in p_->addCamera(camera, devnum); But I noticed that camera becomes nullptr after p_->addCamera() call and that's I would expect. I double-checked with running the codepath with your change, and plugging in a camera crashes with a `null` segfault backtrace. Am I missing something? > >> } >> >> /** >> @@ -427,6 +446,7 @@ void CameraManager::removeCamera(Camera *camera) >> ASSERT(Thread::current() == p_.get()); >> >> p_->removeCamera(camera); >> + cameraRemoved.emit(camera); >> } >> >> /** > > -- > Regards, > > Laurent Pinchart
Hi Umang, On Fri, May 08, 2020 at 01:47:15PM +0000, Umang Jain wrote: > On Wed, May 6, 2020 at 23:53, Laurent Pinchart wrote: > > On Wed, May 06, 2020 at 10:33:53AM +0000, Umang Jain wrote: > >> 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 | 4 ++++ > >> src/libcamera/camera_manager.cpp | 20 ++++++++++++++++++++ > >> 2 files changed, 24 insertions(+) > >> > >> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > >> index 079f848..558bb96 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 { > >> > >> @@ -42,6 +43,9 @@ public: > >> void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); > >> EventDispatcher *eventDispatcher(); > >> > >> + Signal<Camera *> cameraAdded; > >> + Signal<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 c75979a..6438f87 100644 > >> --- a/src/libcamera/camera_manager.cpp > >> +++ b/src/libcamera/camera_manager.cpp > >> @@ -391,6 +391,23 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum) > >> return iter->second.lock(); > >> } > >> > >> +/** > >> + * \var CameraManager::cameraAdded > >> + * \brief Signal emitted when a new camera is added in CameraManager > >> + * > >> + * This signal is emitted when a new camera is added by the CameraManager > >> + * in the list of cameras it manages. A pointer to the newly-added camera > >> + * is passed as a parameter. > > > > I think we should detail here the relationship with > > CameraManager::start() and CameraManager::cameras(), and note that the > > signal handler should be fast (otherwise it would block operation of > > other cameras). How about the following ? > > > > * \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. > > > >> + */ > >> + > >> +/** > >> + * \var CameraManager::cameraRemoved > >> + * \brief Signal emitted when a camera is removed in CameraManager > >> + * > >> + * This signal is emitted when a camera is removed from the CameraManager. > >> + * A pointer to the removed camera is passed as a parameter. > > > > And something similar here ? > > > > * \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 > >> @@ -409,7 +426,9 @@ void > >> CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t > >> devnum) > >> { > >> ASSERT(Thread::current() == p_.get()); > >> > >> + Camera *cam = camera.get(); > >> p_->addCamera(camera, devnum); > >> + cameraAdded.emit(cam); > > > > You don't need a local cam variable as camera is a shared pointer, not a > > unique pointer (and if it was a unique pointer and was given to > > addCamera() with std::move, it would be potentially unsafe to use it > > afterwards). > > > > cameraAdded.emit(camera.get()); > > > > is fine. > > Yes, camera is a shared pointer here and is std::move in p_->addCamera(camera, devnum); > But I noticed that camera becomes nullptr after p_->addCamera() call and that's > I would expect. I double-checked with running the codepath with your change, > and plugging in a camera crashes with a `null` segfault backtrace. Am I missing something? You were absolutely right, I had missed the fact that p_->addCamera() takes a non-const reference and uses std::move internally. > >> } > >> > >> /** > >> @@ -427,6 +446,7 @@ void CameraManager::removeCamera(Camera *camera) > >> ASSERT(Thread::current() == p_.get()); > >> > >> p_->removeCamera(camera); > >> + cameraRemoved.emit(camera); > >> } > >> > >> /**
diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 079f848..558bb96 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 { @@ -42,6 +43,9 @@ public: void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); EventDispatcher *eventDispatcher(); + Signal<Camera *> cameraAdded; + Signal<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 c75979a..6438f87 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -391,6 +391,23 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum) return iter->second.lock(); } +/** + * \var CameraManager::cameraAdded + * \brief Signal emitted when a new camera is added in CameraManager + * + * This signal is emitted when a new camera is added by the CameraManager + * in the list of cameras it manages. A pointer to the newly-added camera + * is passed as a parameter. + */ + +/** + * \var CameraManager::cameraRemoved + * \brief Signal emitted when a camera is removed in CameraManager + * + * This signal is emitted when a camera is removed from the CameraManager. + * A pointer to the removed camera is passed as a parameter. + */ + /** * \brief Add a camera to the camera manager * \param[in] camera The camera to be added @@ -409,7 +426,9 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum) { ASSERT(Thread::current() == p_.get()); + Camera *cam = camera.get(); p_->addCamera(camera, devnum); + cameraAdded.emit(cam); } /** @@ -427,6 +446,7 @@ void CameraManager::removeCamera(Camera *camera) ASSERT(Thread::current() == p_.get()); p_->removeCamera(camera); + cameraRemoved.emit(camera); } /**
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 | 4 ++++ src/libcamera/camera_manager.cpp | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+)