[libcamera-devel] [PATCH] libcamera: pipeline_handler: Optimise factory implementation
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Jun 10 13:59:20 CEST 2019
Hi Laurent,
Thanks for your work.
On 2019-06-05 17:54:13 +0300, Laurent Pinchart wrote:
> The REGISTER_PIPELINE_HANDLER() macro defines and instantiates a
> subclass of the PipelineHandlerFactory class that specialises the
> virtual create() method. Now that create() does more than just creating
> an instance, boilerplate code gets duplicated between different
> factories.
>
> Factor out the instance creation code to a new virtual createInstance()
> method, and turn create() into a non-virtual method of the base class
> that can then be defined in the .cpp file.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
I like that this both becomes simpler to read and less duplication. With
the protected/private comment Paul points out,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/include/pipeline_handler.h | 11 +++-----
> src/libcamera/pipeline_handler.cpp | 33 +++++++++++++-----------
> 2 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 84307e408772..36f0b84c0d6c 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -107,7 +107,7 @@ public:
> PipelineHandlerFactory(const char *name);
> virtual ~PipelineHandlerFactory() { };
>
> - virtual std::shared_ptr<PipelineHandler> create(CameraManager *manager) = 0;
> + std::shared_ptr<PipelineHandler> create(CameraManager *manager);
>
> const std::string &name() const { return name_; }
>
> @@ -115,7 +115,7 @@ public:
> static std::vector<PipelineHandlerFactory *> &factories();
>
> protected:
> - void setInfo(PipelineHandler *handler, const char *name);
> + virtual PipelineHandler *createInstance(CameraManager *manager) = 0;
>
> private:
> std::string name_;
> @@ -126,12 +126,9 @@ class handler##Factory final : public PipelineHandlerFactory \
> { \
> public: \
> handler##Factory() : PipelineHandlerFactory(#handler) {} \
> - std::shared_ptr<PipelineHandler> create(CameraManager *manager) \
> + PipelineHandler *createInstance(CameraManager *manager) \
> { \
> - std::shared_ptr<handler> h = \
> - std::make_shared<handler>(manager); \
> - setInfo(h.get(), #handler); \
> - return h; \
> + return new handler(manager); \
> } \
> }; \
> static handler##Factory global_##handler##Factory;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 800931d81337..af19f4a33187 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -539,17 +539,18 @@ 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. 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
> + * \return A shared pointer to a new instance of the PipelineHandler subclass
> + * corresponding to the factory
> */
> +std::shared_ptr<PipelineHandler> PipelineHandlerFactory::create(CameraManager *manager)
> +{
> + PipelineHandler *handler = createInstance(manager);
> + handler->name_ = name_.c_str();
> + return std::shared_ptr<PipelineHandler>(handler);
> +}
>
> /**
> * \fn PipelineHandlerFactory::name()
> @@ -589,15 +590,17 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories()
> }
>
> /**
> - * \brief Set the information of a given pipeline handler
> - * \param[in] handler The handler whose info is to be set
> - * \param[in] name The name of the pipeline handler
> + * \fn PipelineHandlerFactory::createInstance()
> + * \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. 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
> */
> -void PipelineHandlerFactory::setInfo(PipelineHandler *handler,
> - const char *name)
> -{
> - handler->name_ = name;
> -}
>
> /**
> * \def REGISTER_PIPELINE_HANDLER
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list