| Message ID | 20190106023328.10989-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2019-01-06 04:33:21 +0200, Laurent Pinchart wrote: > There can only be a single camera manager instance in the application. > Creating it as a singleton helps avoiding mistakes. It also allows the > camera manager to be used as a storage of global data, such as the > future event dispatcher. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/camera_manager.h | 6 ++++-- > src/libcamera/camera_manager.cpp | 15 +++++++++++++++ > test/list.cpp | 4 +--- > 3 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 2768a5bd2384..56a3f32d8b6f 100644 > --- a/include/libcamera/camera_manager.h > +++ b/include/libcamera/camera_manager.h > @@ -19,15 +19,17 @@ class PipelineHandler; > class CameraManager > { > public: > - CameraManager(); > - > int start(); > void stop(); > > std::vector<std::string> list() const; > Camera *get(const std::string &name); > > + static CameraManager *instance(); > + > private: > + CameraManager(); > + > DeviceEnumerator *enumerator_; > std::vector<PipelineHandler *> pipes_; > }; > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 50a805fc665c..db2bc4b7424e 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -161,4 +161,19 @@ Camera *CameraManager::get(const std::string &name) > return nullptr; > } > > +/** > + * \brief Retrieve the camera manager instance > + * > + * The CameraManager is a singleton and can't be constructed manually. This > + * function can instead be used to retrieve the single global instance of the > + * manager. > + * > + * \return The camera manager instance > + */ > +CameraManager *CameraManager::instance() > +{ > + static CameraManager manager; > + return &manager; > +} > + > } /* namespace libcamera */ > diff --git a/test/list.cpp b/test/list.cpp > index 39b8a41d1fef..49610c697c63 100644 > --- a/test/list.cpp > +++ b/test/list.cpp > @@ -19,7 +19,7 @@ class ListTest : public Test > protected: > int init() > { > - cm = new CameraManager(); > + cm = CameraManager::instance(); > if (!cm) > return -ENOMEM; > > @@ -43,8 +43,6 @@ protected: > void cleanup() > { > cm->stop(); > - > - delete cm; > } > > private: > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Sun, Jan 06, 2019 at 04:33:21AM +0200, Laurent Pinchart wrote: > There can only be a single camera manager instance in the application. > Creating it as a singleton helps avoiding mistakes. It also allows the > camera manager to be used as a storage of global data, such as the > future event dispatcher. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/camera_manager.h | 6 ++++-- > src/libcamera/camera_manager.cpp | 15 +++++++++++++++ > test/list.cpp | 4 +--- > 3 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 2768a5bd2384..56a3f32d8b6f 100644 > --- a/include/libcamera/camera_manager.h > +++ b/include/libcamera/camera_manager.h > @@ -19,15 +19,17 @@ class PipelineHandler; > class CameraManager > { > public: > - CameraManager(); > - > int start(); > void stop(); > > std::vector<std::string> list() const; > Camera *get(const std::string &name); > > + static CameraManager *instance(); > + > private: > + CameraManager(); > + > DeviceEnumerator *enumerator_; > std::vector<PipelineHandler *> pipes_; > }; > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 50a805fc665c..db2bc4b7424e 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -161,4 +161,19 @@ Camera *CameraManager::get(const std::string &name) > return nullptr; > } > > +/** > + * \brief Retrieve the camera manager instance > + * > + * The CameraManager is a singleton and can't be constructed manually. This > + * function can instead be used to retrieve the single global instance of the nit: s/can/shall Apart from this minor thing, one other thing I wonder about is copy constructors, should we set them to "= delete" ? With this clarified: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + * manager. > + * > + * \return The camera manager instance > + */ > +CameraManager *CameraManager::instance() > +{ > + static CameraManager manager; > + return &manager; > +} > + > } /* namespace libcamera */ > diff --git a/test/list.cpp b/test/list.cpp > index 39b8a41d1fef..49610c697c63 100644 > --- a/test/list.cpp > +++ b/test/list.cpp > @@ -19,7 +19,7 @@ class ListTest : public Test > protected: > int init() > { > - cm = new CameraManager(); > + cm = CameraManager::instance(); > if (!cm) > return -ENOMEM; > > @@ -43,8 +43,6 @@ protected: > void cleanup() > { > cm->stop(); > - > - delete cm; > } > > private: > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Monday, 7 January 2019 12:34:21 EET Jacopo Mondi wrote: > On Sun, Jan 06, 2019 at 04:33:21AM +0200, Laurent Pinchart wrote: > > There can only be a single camera manager instance in the application. > > Creating it as a singleton helps avoiding mistakes. It also allows the > > camera manager to be used as a storage of global data, such as the > > future event dispatcher. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > include/libcamera/camera_manager.h | 6 ++++-- > > src/libcamera/camera_manager.cpp | 15 +++++++++++++++ > > test/list.cpp | 4 +--- > > 3 files changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/include/libcamera/camera_manager.h > > b/include/libcamera/camera_manager.h index 2768a5bd2384..56a3f32d8b6f > > 100644 > > --- a/include/libcamera/camera_manager.h > > +++ b/include/libcamera/camera_manager.h > > @@ -19,15 +19,17 @@ class PipelineHandler; > > class CameraManager > > { > > public: > > - CameraManager(); > > - > > int start(); > > void stop(); > > > > std::vector<std::string> list() const; > > Camera *get(const std::string &name); > > > > + static CameraManager *instance(); > > + > > private: > > + CameraManager(); > > + > > DeviceEnumerator *enumerator_; > > std::vector<PipelineHandler *> pipes_; > > > > }; > > diff --git a/src/libcamera/camera_manager.cpp > > b/src/libcamera/camera_manager.cpp index 50a805fc665c..db2bc4b7424e > > 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -161,4 +161,19 @@ Camera *CameraManager::get(const std::string &name) > > return nullptr; > > } > > > > +/** > > + * \brief Retrieve the camera manager instance > > + * > > + * The CameraManager is a singleton and can't be constructed manually. > > This + * function can instead be used to retrieve the single global > > instance of the > > nit: s/can/shall Fixed. > Apart from this minor thing, one other thing I wonder about is copy > constructors, should we set them to "= delete" ? Yes, and the operator= too. Fixed. > With this clarified: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thank you. > > + * manager. > > + * > > + * \return The camera manager instance > > + */ > > +CameraManager *CameraManager::instance() > > +{ > > + static CameraManager manager; > > + return &manager; > > +} > > + > > > > } /* namespace libcamera */ > > > > diff --git a/test/list.cpp b/test/list.cpp > > index 39b8a41d1fef..49610c697c63 100644 > > --- a/test/list.cpp > > +++ b/test/list.cpp > > @@ -19,7 +19,7 @@ class ListTest : public Test > > protected: > > int init() > > { > > - cm = new CameraManager(); > > + cm = CameraManager::instance(); > > if (!cm) > > return -ENOMEM; > > > > @@ -43,8 +43,6 @@ protected: > > void cleanup() > > { > > cm->stop(); > > - > > - delete cm; > > } > > > > private:
diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 2768a5bd2384..56a3f32d8b6f 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -19,15 +19,17 @@ class PipelineHandler; class CameraManager { public: - CameraManager(); - int start(); void stop(); std::vector<std::string> list() const; Camera *get(const std::string &name); + static CameraManager *instance(); + private: + CameraManager(); + DeviceEnumerator *enumerator_; std::vector<PipelineHandler *> pipes_; }; diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 50a805fc665c..db2bc4b7424e 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -161,4 +161,19 @@ Camera *CameraManager::get(const std::string &name) return nullptr; } +/** + * \brief Retrieve the camera manager instance + * + * The CameraManager is a singleton and can't be constructed manually. This + * function can instead be used to retrieve the single global instance of the + * manager. + * + * \return The camera manager instance + */ +CameraManager *CameraManager::instance() +{ + static CameraManager manager; + return &manager; +} + } /* namespace libcamera */ diff --git a/test/list.cpp b/test/list.cpp index 39b8a41d1fef..49610c697c63 100644 --- a/test/list.cpp +++ b/test/list.cpp @@ -19,7 +19,7 @@ class ListTest : public Test protected: int init() { - cm = new CameraManager(); + cm = CameraManager::instance(); if (!cm) return -ENOMEM; @@ -43,8 +43,6 @@ protected: void cleanup() { cm->stop(); - - delete cm; } private:
There can only be a single camera manager instance in the application. Creating it as a singleton helps avoiding mistakes. It also allows the camera manager to be used as a storage of global data, such as the future event dispatcher. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/camera_manager.h | 6 ++++-- src/libcamera/camera_manager.cpp | 15 +++++++++++++++ test/list.cpp | 4 +--- 3 files changed, 20 insertions(+), 5 deletions(-)