[libcamera-devel] libcamera: pipeline_handler: Optimise factory implementation
diff mbox series

Message ID 20190605145413.9888-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: pipeline_handler: Optimise factory implementation
Related show

Commit Message

Laurent Pinchart June 5, 2019, 2:54 p.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/include/pipeline_handler.h | 11 +++-----
 src/libcamera/pipeline_handler.cpp       | 33 +++++++++++++-----------
 2 files changed, 22 insertions(+), 22 deletions(-)

Comments

Paul Elder June 5, 2019, 4:18 p.m. UTC | #1 
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@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
Laurent Pinchart June 6, 2019, 7:25 a.m. UTC | #2 
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@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
Niklas Söderlund June 10, 2019, 11:59 a.m. UTC | #3 
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@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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