[libcamera-devel] [PATCH] libcamera: pipeline_handler: Optimise factory implementation

[Paul Elder]

Hi Laurent,

Thank you for the patch.

On Wed, Jun 05, 2019 at 05:54:13PM +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>
> ---
>  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;

I guess I didn't notice this when I wrote it in the first place, but
does this need to be protected rather than private?

>  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

Thanks,

Paul