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

[Laurent Pinchart]

Hi Paul,

On Wed, Jun 05, 2019 at 12:18:06PM -0400, Paul Elder wrote:
> 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?

setInfo() needed to be protected as it was called by derived classes,
but createInstance() can be private indeed. I'll fix this.

> >  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

-- 
Regards,

Laurent Pinchart