| Message ID | 20190124101651.9993-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2019-01-24 12:16:42 +0200, Laurent Pinchart wrote: > Instead of passing the camera manager pointer to the match() function, > and later to more PipelineHandler functions, store it in the > PipelineHandler::manager_ member variable at construction time and > access it from there. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera_manager.cpp | 4 +-- > src/libcamera/include/pipeline_handler.h | 20 ++++++++----- > src/libcamera/pipeline/ipu3/ipu3.cpp | 18 ++++++------ > src/libcamera/pipeline/uvcvideo.cpp | 12 ++++---- > src/libcamera/pipeline/vimc.cpp | 12 ++++---- > src/libcamera/pipeline_handler.cpp | 36 ++++++++++++++++++++---- > 6 files changed, 66 insertions(+), 36 deletions(-) > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 4ea7ed44cc31..14410d4dcda7 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -98,8 +98,8 @@ int CameraManager::start() > * all pipelines it can provide. > */ > while (1) { > - PipelineHandler *pipe = factory->create(); > - if (!pipe->match(this, enumerator_.get())) { > + PipelineHandler *pipe = factory->create(this); > + if (!pipe->match(enumerator_.get())) { > delete pipe; > break; > } > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index b03217d5bff8..7bb07d1ec5c7 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -19,9 +19,13 @@ class DeviceEnumerator; > class PipelineHandler > { > public: > - virtual ~PipelineHandler() { }; > + PipelineHandler(CameraManager *manager); > + virtual ~PipelineHandler(); > > - virtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0; > + virtual bool match(DeviceEnumerator *enumerator) = 0; > + > +protected: > + CameraManager *manager_; > }; > > class PipelineHandlerFactory > @@ -30,7 +34,7 @@ public: > PipelineHandlerFactory(const char *name); > virtual ~PipelineHandlerFactory() { }; > > - virtual PipelineHandler *create() = 0; > + virtual PipelineHandler *create(CameraManager *manager) = 0; > > const std::string &name() const { return name_; } > > @@ -42,11 +46,13 @@ private: > }; > > #define REGISTER_PIPELINE_HANDLER(handler) \ > -class handler##Factory : public PipelineHandlerFactory { \ > +class handler##Factory : public PipelineHandlerFactory \ > +{ \ > public: \ > - handler##Factory() : PipelineHandlerFactory(#handler) { } \ > - PipelineHandler *create() final { \ > - return new handler(); \ > + handler##Factory() : PipelineHandlerFactory(#handler) {} \ > + PipelineHandler *create(CameraManager *manager) final \ > + { \ > + return new handler(manager); \ > } \ > }; \ > static handler##Factory global_##handler##Factory; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8cbc10acfbb5..589b3078f301 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -23,20 +23,20 @@ LOG_DEFINE_CATEGORY(IPU3) > class PipelineHandlerIPU3 : public PipelineHandler > { > public: > - PipelineHandlerIPU3(); > + PipelineHandlerIPU3(CameraManager *manager); > ~PipelineHandlerIPU3(); > > - bool match(CameraManager *manager, DeviceEnumerator *enumerator); > + bool match(DeviceEnumerator *enumerator); > > private: > MediaDevice *cio2_; > MediaDevice *imgu_; > > - void registerCameras(CameraManager *manager); > + void registerCameras(); > }; > > -PipelineHandlerIPU3::PipelineHandlerIPU3() > - : cio2_(nullptr), imgu_(nullptr) > +PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager) > + : PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr) > { > } > > @@ -52,7 +52,7 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3() > imgu_ = nullptr; > } > > -bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumerator) > +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > { > DeviceMatch cio2_dm("ipu3-cio2"); > cio2_dm.add("ipu3-csi2 0"); > @@ -106,7 +106,7 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > if (cio2_->disableLinks()) > goto error_close_cio2; > > - registerCameras(manager); > + registerCameras(); > > cio2_->close(); > > @@ -127,7 +127,7 @@ error_release_mdev: > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > * CIO2 CSI-2 receivers. > */ > -void PipelineHandlerIPU3::registerCameras(CameraManager *manager) > +void PipelineHandlerIPU3::registerCameras() > { > /* > * For each CSI-2 receiver on the IPU3, create a Camera if an > @@ -172,7 +172,7 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager) > > std::string cameraName = sensor->name() + " " + std::to_string(id); > std::shared_ptr<Camera> camera = Camera::create(cameraName); > - manager->addCamera(std::move(camera)); > + manager_->addCamera(std::move(camera)); > > LOG(IPU3, Info) > << "Registered Camera[" << numCameras << "] \"" > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index 3ba69da8b775..92b23da73901 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -17,17 +17,17 @@ namespace libcamera { > class PipelineHandlerUVC : public PipelineHandler > { > public: > - PipelineHandlerUVC(); > + PipelineHandlerUVC(CameraManager *manager); > ~PipelineHandlerUVC(); > > - bool match(CameraManager *manager, DeviceEnumerator *enumerator); > + bool match(DeviceEnumerator *enumerator); > > private: > MediaDevice *dev_; > }; > > -PipelineHandlerUVC::PipelineHandlerUVC() > - : dev_(nullptr) > +PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) > + : PipelineHandler(manager), dev_(nullptr) > { > } > > @@ -37,7 +37,7 @@ PipelineHandlerUVC::~PipelineHandlerUVC() > dev_->release(); > } > > -bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumerator) > +bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > { > DeviceMatch dm("uvcvideo"); > > @@ -49,7 +49,7 @@ bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumera > dev_->acquire(); > > std::shared_ptr<Camera> camera = Camera::create(dev_->model()); > - manager->addCamera(std::move(camera)); > + manager_->addCamera(std::move(camera)); > > return true; > } > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 82b9237a3d7d..f12d007cd956 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -17,17 +17,17 @@ namespace libcamera { > class PipeHandlerVimc : public PipelineHandler > { > public: > - PipeHandlerVimc(); > + PipeHandlerVimc(CameraManager *manager); > ~PipeHandlerVimc(); > > - bool match(CameraManager *manager, DeviceEnumerator *enumerator); > + bool match(DeviceEnumerator *enumerator); > > private: > MediaDevice *dev_; > }; > > -PipeHandlerVimc::PipeHandlerVimc() > - : dev_(nullptr) > +PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager) > + : PipelineHandler(manager), dev_(nullptr) > { > } > > @@ -37,7 +37,7 @@ PipeHandlerVimc::~PipeHandlerVimc() > dev_->release(); > } > > -bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator) > +bool PipeHandlerVimc::match(DeviceEnumerator *enumerator) > { > DeviceMatch dm("vimc"); > > @@ -65,7 +65,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator > * object is modeled. > */ > std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera"); > - manager->addCamera(std::move(camera)); > + manager_->addCamera(std::move(camera)); > > return true; > } > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index c24feeafc503..45788487b5c0 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -34,18 +34,30 @@ LOG_DEFINE_CATEGORY(Pipeline) > * with the pipelines it supports and creates corresponding Camera devices. > */ > > +/** > + * \brief Construct a PipelineHandler instance > + * \param[in] manager The camera manager > + */ > +PipelineHandler::PipelineHandler(CameraManager *manager) > + : manager_(manager) > +{ > +} > + > +PipelineHandler::~PipelineHandler() > +{ > +} > + > /** > * \fn PipelineHandler::match(DeviceEnumerator *enumerator) > * \brief Match media devices and create camera instances > - * \param manager The camera manager > * \param enumerator The enumerator providing all media devices found in the > * system > * > * This function is the main entry point of the pipeline handler. It is called > - * by the camera manager with the \a manager and \a enumerator passed as > - * arguments. It shall acquire from the \a enumerator all the media devices it > - * needs for a single pipeline, create one or multiple Camera instances and > - * register them with the \a manager. > + * by the camera manager with the \a enumerator passed as an argument. It shall > + * acquire from the \a enumerator all the media devices it needs for a single > + * pipeline, create one or multiple Camera instances and register them with the > + * camera manager. > * > * If all media devices needed by the pipeline handler are found, they must all > * be acquired by a call to MediaDevice::acquire(). This function shall then > @@ -66,6 +78,15 @@ LOG_DEFINE_CATEGORY(Pipeline) > * created, or false otherwise > */ > > +/** > + * \var PipelineHandler::manager_ > + * \brief The Camera manager associated with the pipeline handler > + * > + * The camera manager pointer is stored in the pipeline handler for the > + * convenience of pipeline handler implementations. It remains valid and > + * constant for the whole lifetime of the pipeline handler. > + */ > + > /** > * \class PipelineHandlerFactory > * \brief Registration of PipelineHandler classes and creation of instances > @@ -96,8 +117,11 @@ PipelineHandlerFactory::PipelineHandlerFactory(const char *name) > /** > * \fn PipelineHandlerFactory::create() > * \brief Create an instance of the PipelineHandler corresponding to the factory > + * \param[in] manager The camera manager > * > - * This virtual function is implemented by the REGISTER_PIPELINE_HANDLER() macro. > + * This virtual function is implemented by the REGISTER_PIPELINE_HANDLER() > + * macro. It creates a pipeline handler instance associated with the camera > + * \a manager. > * > * \return a pointer to a newly constructed instance of the PipelineHandler > * subclass corresponding to the factory > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 4ea7ed44cc31..14410d4dcda7 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -98,8 +98,8 @@ int CameraManager::start() * all pipelines it can provide. */ while (1) { - PipelineHandler *pipe = factory->create(); - if (!pipe->match(this, enumerator_.get())) { + PipelineHandler *pipe = factory->create(this); + if (!pipe->match(enumerator_.get())) { delete pipe; break; } diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index b03217d5bff8..7bb07d1ec5c7 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -19,9 +19,13 @@ class DeviceEnumerator; class PipelineHandler { public: - virtual ~PipelineHandler() { }; + PipelineHandler(CameraManager *manager); + virtual ~PipelineHandler(); - virtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0; + virtual bool match(DeviceEnumerator *enumerator) = 0; + +protected: + CameraManager *manager_; }; class PipelineHandlerFactory @@ -30,7 +34,7 @@ public: PipelineHandlerFactory(const char *name); virtual ~PipelineHandlerFactory() { }; - virtual PipelineHandler *create() = 0; + virtual PipelineHandler *create(CameraManager *manager) = 0; const std::string &name() const { return name_; } @@ -42,11 +46,13 @@ private: }; #define REGISTER_PIPELINE_HANDLER(handler) \ -class handler##Factory : public PipelineHandlerFactory { \ +class handler##Factory : public PipelineHandlerFactory \ +{ \ public: \ - handler##Factory() : PipelineHandlerFactory(#handler) { } \ - PipelineHandler *create() final { \ - return new handler(); \ + handler##Factory() : PipelineHandlerFactory(#handler) {} \ + PipelineHandler *create(CameraManager *manager) final \ + { \ + return new handler(manager); \ } \ }; \ static handler##Factory global_##handler##Factory; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 8cbc10acfbb5..589b3078f301 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -23,20 +23,20 @@ LOG_DEFINE_CATEGORY(IPU3) class PipelineHandlerIPU3 : public PipelineHandler { public: - PipelineHandlerIPU3(); + PipelineHandlerIPU3(CameraManager *manager); ~PipelineHandlerIPU3(); - bool match(CameraManager *manager, DeviceEnumerator *enumerator); + bool match(DeviceEnumerator *enumerator); private: MediaDevice *cio2_; MediaDevice *imgu_; - void registerCameras(CameraManager *manager); + void registerCameras(); }; -PipelineHandlerIPU3::PipelineHandlerIPU3() - : cio2_(nullptr), imgu_(nullptr) +PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager) + : PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr) { } @@ -52,7 +52,7 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3() imgu_ = nullptr; } -bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumerator) +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) { DeviceMatch cio2_dm("ipu3-cio2"); cio2_dm.add("ipu3-csi2 0"); @@ -106,7 +106,7 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer if (cio2_->disableLinks()) goto error_close_cio2; - registerCameras(manager); + registerCameras(); cio2_->close(); @@ -127,7 +127,7 @@ error_release_mdev: * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four * CIO2 CSI-2 receivers. */ -void PipelineHandlerIPU3::registerCameras(CameraManager *manager) +void PipelineHandlerIPU3::registerCameras() { /* * For each CSI-2 receiver on the IPU3, create a Camera if an @@ -172,7 +172,7 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager) std::string cameraName = sensor->name() + " " + std::to_string(id); std::shared_ptr<Camera> camera = Camera::create(cameraName); - manager->addCamera(std::move(camera)); + manager_->addCamera(std::move(camera)); LOG(IPU3, Info) << "Registered Camera[" << numCameras << "] \"" diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index 3ba69da8b775..92b23da73901 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -17,17 +17,17 @@ namespace libcamera { class PipelineHandlerUVC : public PipelineHandler { public: - PipelineHandlerUVC(); + PipelineHandlerUVC(CameraManager *manager); ~PipelineHandlerUVC(); - bool match(CameraManager *manager, DeviceEnumerator *enumerator); + bool match(DeviceEnumerator *enumerator); private: MediaDevice *dev_; }; -PipelineHandlerUVC::PipelineHandlerUVC() - : dev_(nullptr) +PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) + : PipelineHandler(manager), dev_(nullptr) { } @@ -37,7 +37,7 @@ PipelineHandlerUVC::~PipelineHandlerUVC() dev_->release(); } -bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumerator) +bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) { DeviceMatch dm("uvcvideo"); @@ -49,7 +49,7 @@ bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumera dev_->acquire(); std::shared_ptr<Camera> camera = Camera::create(dev_->model()); - manager->addCamera(std::move(camera)); + manager_->addCamera(std::move(camera)); return true; } diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 82b9237a3d7d..f12d007cd956 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -17,17 +17,17 @@ namespace libcamera { class PipeHandlerVimc : public PipelineHandler { public: - PipeHandlerVimc(); + PipeHandlerVimc(CameraManager *manager); ~PipeHandlerVimc(); - bool match(CameraManager *manager, DeviceEnumerator *enumerator); + bool match(DeviceEnumerator *enumerator); private: MediaDevice *dev_; }; -PipeHandlerVimc::PipeHandlerVimc() - : dev_(nullptr) +PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager) + : PipelineHandler(manager), dev_(nullptr) { } @@ -37,7 +37,7 @@ PipeHandlerVimc::~PipeHandlerVimc() dev_->release(); } -bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator) +bool PipeHandlerVimc::match(DeviceEnumerator *enumerator) { DeviceMatch dm("vimc"); @@ -65,7 +65,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator * object is modeled. */ std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera"); - manager->addCamera(std::move(camera)); + manager_->addCamera(std::move(camera)); return true; } diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index c24feeafc503..45788487b5c0 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -34,18 +34,30 @@ LOG_DEFINE_CATEGORY(Pipeline) * with the pipelines it supports and creates corresponding Camera devices. */ +/** + * \brief Construct a PipelineHandler instance + * \param[in] manager The camera manager + */ +PipelineHandler::PipelineHandler(CameraManager *manager) + : manager_(manager) +{ +} + +PipelineHandler::~PipelineHandler() +{ +} + /** * \fn PipelineHandler::match(DeviceEnumerator *enumerator) * \brief Match media devices and create camera instances - * \param manager The camera manager * \param enumerator The enumerator providing all media devices found in the * system * * This function is the main entry point of the pipeline handler. It is called - * by the camera manager with the \a manager and \a enumerator passed as - * arguments. It shall acquire from the \a enumerator all the media devices it - * needs for a single pipeline, create one or multiple Camera instances and - * register them with the \a manager. + * by the camera manager with the \a enumerator passed as an argument. It shall + * acquire from the \a enumerator all the media devices it needs for a single + * pipeline, create one or multiple Camera instances and register them with the + * camera manager. * * If all media devices needed by the pipeline handler are found, they must all * be acquired by a call to MediaDevice::acquire(). This function shall then @@ -66,6 +78,15 @@ LOG_DEFINE_CATEGORY(Pipeline) * created, or false otherwise */ +/** + * \var PipelineHandler::manager_ + * \brief The Camera manager associated with the pipeline handler + * + * The camera manager pointer is stored in the pipeline handler for the + * convenience of pipeline handler implementations. It remains valid and + * constant for the whole lifetime of the pipeline handler. + */ + /** * \class PipelineHandlerFactory * \brief Registration of PipelineHandler classes and creation of instances @@ -96,8 +117,11 @@ PipelineHandlerFactory::PipelineHandlerFactory(const char *name) /** * \fn PipelineHandlerFactory::create() * \brief Create an instance of the PipelineHandler corresponding to the factory + * \param[in] manager The camera manager * - * This virtual function is implemented by the REGISTER_PIPELINE_HANDLER() macro. + * This virtual function is implemented by the REGISTER_PIPELINE_HANDLER() + * macro. It creates a pipeline handler instance associated with the camera + * \a manager. * * \return a pointer to a newly constructed instance of the PipelineHandler * subclass corresponding to the factory
Instead of passing the camera manager pointer to the match() function, and later to more PipelineHandler functions, store it in the PipelineHandler::manager_ member variable at construction time and access it from there. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/camera_manager.cpp | 4 +-- src/libcamera/include/pipeline_handler.h | 20 ++++++++----- src/libcamera/pipeline/ipu3/ipu3.cpp | 18 ++++++------ src/libcamera/pipeline/uvcvideo.cpp | 12 ++++---- src/libcamera/pipeline/vimc.cpp | 12 ++++---- src/libcamera/pipeline_handler.cpp | 36 ++++++++++++++++++++---- 6 files changed, 66 insertions(+), 36 deletions(-)