[libcamera-devel,v2,1/8] libcamera: pipeline_handler: Don't index factories by name

233 diff mbox series

Pipeline handler factories are register in a map indexed by their name,
and the list of names is used to expose the factories and look them up.
This is unnecessary cumbersome, we can instead store factories in a
vector and expose it directly. The pipeline factory users will still
have access to the factory names through the factory name() function.

The PipelineHandlerFactory::create() method becomes so simple that it
can be inlined in its single caller, removing the unneeded usage of the
DeviceEnumerator in the factory.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes since v1:

- ASSERT() factory name uniqueness
---
 src/libcamera/camera_manager.cpp         | 22 +++---
 src/libcamera/include/pipeline_handler.h | 29 ++++----
 src/libcamera/pipeline_handler.cpp       | 95 ++++++++----------------
 3 files changed, 57 insertions(+), 89 deletions(-)

Hi Laurent,

Thanks for your work.

On 2019-01-15 17:18:42 +0200, Laurent Pinchart wrote:
> Pipeline handler factories are register in a map indexed by their name,
> and the list of names is used to expose the factories and look them up.
> This is unnecessary cumbersome, we can instead store factories in a
> vector and expose it directly. The pipeline factory users will still
> have access to the factory names through the factory name() function.
> 
> The PipelineHandlerFactory::create() method becomes so simple that it
> can be inlined in its single caller, removing the unneeded usage of the
> DeviceEnumerator in the factory.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

This patch could have benefited from '--diff-algorithm=patience' :-)

Apart from that really nice work!

> ---
> Changes since v1:
> 
> - ASSERT() factory name uniqueness
> ---
>  src/libcamera/camera_manager.cpp         | 22 +++---
>  src/libcamera/include/pipeline_handler.h | 29 ++++----
>  src/libcamera/pipeline_handler.cpp       | 95 ++++++++----------------
>  3 files changed, 57 insertions(+), 89 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index be327f5d5638..91ef6753f405 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -74,20 +74,24 @@ int CameraManager::start()
>  	 * file and only fallback on all handlers if there is no
>  	 * configuration file.
>  	 */
> -	std::vector<std::string> handlers = PipelineHandlerFactory::handlers();
> -
> -	for (std::string const &handler : handlers) {
> -		PipelineHandler *pipe;
> +	std::vector<PipelineHandlerFactory *> &handlers = PipelineHandlerFactory::handlers();
>  
> +	for (PipelineHandlerFactory *factory : handlers) {
>  		/*
>  		 * Try each pipeline handler until it exhaust
>  		 * all pipelines it can provide.
>  		 */
> -		do {
> -			pipe = PipelineHandlerFactory::create(handler, enumerator_);
> -			if (pipe)
> -				pipes_.push_back(pipe);
> -		} while (pipe);
> +		while (1) {
> +			PipelineHandler *pipe = factory->create();
> +			if (!pipe->match(enumerator_)) {
> +				delete pipe;
> +				break;
> +			}
> +
> +			LOG(Debug) << "Pipeline handler \"" << factory->name()
> +				   << "\" matched";
> +			pipes_.push_back(pipe);
> +		}
>  	}
>  
>  	/* TODO: register hot-plug callback here */
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index fdf8b8db2e0a..764dde9ccc65 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -31,29 +31,28 @@ public:
>  class PipelineHandlerFactory
>  {
>  public:
> +	PipelineHandlerFactory(const char *name);
>  	virtual ~PipelineHandlerFactory() { };
>  
>  	virtual PipelineHandler *create() = 0;
>  
> -	static void registerType(const std::string &name, PipelineHandlerFactory *factory);
> -	static PipelineHandler *create(const std::string &name, DeviceEnumerator *enumerator);
> -	static std::vector<std::string> handlers();
> +	const std::string &name() const { return name_; }
> +
> +	static void registerType(PipelineHandlerFactory *factory);
> +	static std::vector<PipelineHandlerFactory *> &handlers();
>  
>  private:
> -	static std::map<std::string, PipelineHandlerFactory *> &registry();
> +	std::string name_;
>  };
>  
> -#define REGISTER_PIPELINE_HANDLER(handler) \
> -class handler##Factory : public PipelineHandlerFactory { \
> -public: \
> -	handler##Factory() \
> -	{ \
> -		PipelineHandlerFactory::registerType(#handler, this); \
> -	} \
> -	virtual PipelineHandler *create() { \
> -		return new handler(); \
> -	} \
> -}; \
> +#define REGISTER_PIPELINE_HANDLER(handler)				\
> +class handler##Factory : public PipelineHandlerFactory {		\
> +public:									\
> +	handler##Factory() : PipelineHandlerFactory(#handler) { }	\
> +	PipelineHandler *create() final {				\
> +		return new handler();					\
> +	}								\
> +};									\
>  static handler##Factory global_##handler##Factory;
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 1daada8653e2..08e3291e741a 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -5,7 +5,6 @@
>   * pipeline_handler.cpp - Pipeline handler infrastructure
>   */
>  
> -#include "device_enumerator.h"
>  #include "log.h"
>  #include "pipeline_handler.h"
>  
> @@ -40,7 +39,7 @@ namespace libcamera {
>   * system
>   *
>   * This function is the main entry point of the pipeline handler. It is called
> - * by the device enumerator with the \a enumerator passed as an argument. It
> + * 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 and create one or multiple Camera instances.
>   *
> @@ -88,6 +87,21 @@ namespace libcamera {
>   * static list of factories.
>   */
>  
> +/**
> + * \brief Construct a pipeline handler factory
> + * \param[in] name Name of the pipeline handler class
> + *
> + * Creating an instance of the factory registers is with the global list of
> + * factories, accessible through the handlers() function.
> + *
> + * The factory \a name is used for debug purpose and shall be unique.
> + */
> +PipelineHandlerFactory::PipelineHandlerFactory(const char *name)
> +	: name_(name)
> +{
> +	registerType(this);
> +}
> +
>  /**
>   * \fn PipelineHandlerFactory::create()
>   * \brief Create an instance of the PipelineHandler corresponding to the factory
> @@ -98,78 +112,29 @@ namespace libcamera {
>   * subclass corresponding to the factory
>   */
>  
> +/**
> + * \fn PipelineHandlerFactory::name()
> + * \brief Retrieve the factory name
> + * \return The factory name
> + */
> +
>  /**
>   * \brief Add a pipeline handler class to the registry
> - * \param[in] name Name of the pipeline handler class
>   * \param[in] factory Factory to use to construct the pipeline handler
>   *
>   * The caller is responsible to guarantee the uniqueness of the pipeline handler
>   * name.
>   */
> -void PipelineHandlerFactory::registerType(const std::string &name,
> -					  PipelineHandlerFactory *factory)
> -{
> -	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> -
> -	if (factories.count(name)) {
> -		LOG(Error) <<  "Registering '" << name << "' pipeline twice";
> -		return;
> -	}
> -
> -	LOG(Debug) << "Registered pipeline handler \"" << name << "\"";
> -	factories[name] = factory;
> -}
> -
> -/**
> - * \brief Create an instance of a pipeline handler if it matches media devices
> - * present in the system
> - * \param[in] name Name of the pipeline handler to instantiate
> - * \param[in] enumerator Device enumerator to search for a match for the handler
> - *
> - * This function matches the media devices required by pipeline \a name against
> - * the devices enumerated by \a enumerator.
> - *
> - * \return the newly created pipeline handler instance if a match was found, or
> - * nullptr otherwise
> - */
> -PipelineHandler *PipelineHandlerFactory::create(const std::string &name,
> -						DeviceEnumerator *enumerator)
> +void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)
>  {
> -	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> -
> -	auto it = factories.find(name);
> -	if (it == factories.end()) {
> -		LOG(Error) << "Trying to create non-existing pipeline handler "
> -			   << name;
> -		return nullptr;
> -	}
> -
> -	PipelineHandler *pipe = it->second->create();
> +	std::vector<PipelineHandlerFactory *> &factories = handlers();
>  
> -	if (pipe->match(enumerator)) {
> -		LOG(Debug) << "Pipeline handler \"" << name << "\" matched";
> -		return pipe;
> -	}
> -
> -	delete pipe;
> -	return nullptr;
> -}
> -
> -/**
> - * \brief Retrieve the names of all pipeline handlers registered with the
> - * factory
> - *
> - * \return a list of all registered pipeline handler names
> - */
> -std::vector<std::string> PipelineHandlerFactory::handlers()
> -{
> -	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> -	std::vector<std::string> handlers;
> +	for (PipelineHandlerFactory *f : factories)
> +		ASSERT(factory->name() != f->name());

Is this check needed? The name come from the class name passed to 
REGISTER_PIPELINE_HANDLER(). If there where to be a duplicated name 
would we not get a symbol collision at compile time?

With or without this fixed

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

>  
> -	for (auto const &handler : factories)
> -		handlers.push_back(handler.first);
> +	factories.push_back(factory);
>  
> -	return handlers;
> +	LOG(Debug) << "Registered pipeline handler \"" << factory->name() << "\"";
>  }
>  
>  /**
> @@ -180,9 +145,9 @@ std::vector<std::string> PipelineHandlerFactory::handlers()
>   *
>   * \return the list of pipeline handler factories
>   */
> -std::map<std::string, PipelineHandlerFactory *> &PipelineHandlerFactory::registry()
> +std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::handlers()
>  {
> -	static std::map<std::string, PipelineHandlerFactory *> factories;
> +	static std::vector<PipelineHandlerFactory *> factories;
>  	return factories;
>  }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Tue, Jan 15, 2019 at 11:14:00PM +0100, Niklas Söderlund wrote:
> On 2019-01-15 17:18:42 +0200, Laurent Pinchart wrote:
> > Pipeline handler factories are register in a map indexed by their name,
> > and the list of names is used to expose the factories and look them up.
> > This is unnecessary cumbersome, we can instead store factories in a
> > vector and expose it directly. The pipeline factory users will still
> > have access to the factory names through the factory name() function.
> > 
> > The PipelineHandlerFactory::create() method becomes so simple that it
> > can be inlined in its single caller, removing the unneeded usage of the
> > DeviceEnumerator in the factory.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> This patch could have benefited from '--diff-algorithm=patience' :-)
> 
> Apart from that really nice work!
> 
> > ---
> > Changes since v1:
> > 
> > - ASSERT() factory name uniqueness
> > ---
> >  src/libcamera/camera_manager.cpp         | 22 +++---
> >  src/libcamera/include/pipeline_handler.h | 29 ++++----
> >  src/libcamera/pipeline_handler.cpp       | 95 ++++++++----------------
> >  3 files changed, 57 insertions(+), 89 deletions(-)

[snip]

> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 1daada8653e2..08e3291e741a 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp

[snip]

> > +void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)
> >  {
> > -	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> > -
> > -	auto it = factories.find(name);
> > -	if (it == factories.end()) {
> > -		LOG(Error) << "Trying to create non-existing pipeline handler "
> > -			   << name;
> > -		return nullptr;
> > -	}
> > -
> > -	PipelineHandler *pipe = it->second->create();
> > +	std::vector<PipelineHandlerFactory *> &factories = handlers();
> >  
> > -	if (pipe->match(enumerator)) {
> > -		LOG(Debug) << "Pipeline handler \"" << name << "\" matched";
> > -		return pipe;
> > -	}
> > -
> > -	delete pipe;
> > -	return nullptr;
> > -}
> > -
> > -/**
> > - * \brief Retrieve the names of all pipeline handlers registered with the
> > - * factory
> > - *
> > - * \return a list of all registered pipeline handler names
> > - */
> > -std::vector<std::string> PipelineHandlerFactory::handlers()
> > -{
> > -	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> > -	std::vector<std::string> handlers;
> > +	for (PipelineHandlerFactory *f : factories)
> > +		ASSERT(factory->name() != f->name());
> 
> Is this check needed? The name come from the class name passed to 
> REGISTER_PIPELINE_HANDLER(). If there where to be a duplicated name 
> would we not get a symbol collision at compile time?

We would, unless the classes are put in an anonymous namespace. Do you
think keeping the check is worth it ?

> With or without this fixed
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> >  
> > -	for (auto const &handler : factories)
> > -		handlers.push_back(handler.first);
> > +	factories.push_back(factory);
> >  
> > -	return handlers;
> > +	LOG(Debug) << "Registered pipeline handler \"" << factory->name() << "\"";
> >  }

[snip]

Hi Laurent,

On 2019-01-16 02:05:08 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Tue, Jan 15, 2019 at 11:14:00PM +0100, Niklas Söderlund wrote:
> > On 2019-01-15 17:18:42 +0200, Laurent Pinchart wrote:
> > > Pipeline handler factories are register in a map indexed by their name,
> > > and the list of names is used to expose the factories and look them up.
> > > This is unnecessary cumbersome, we can instead store factories in a
> > > vector and expose it directly. The pipeline factory users will still
> > > have access to the factory names through the factory name() function.
> > > 
> > > The PipelineHandlerFactory::create() method becomes so simple that it
> > > can be inlined in its single caller, removing the unneeded usage of the
> > > DeviceEnumerator in the factory.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > This patch could have benefited from '--diff-algorithm=patience' :-)
> > 
> > Apart from that really nice work!
> > 
> > > ---
> > > Changes since v1:
> > > 
> > > - ASSERT() factory name uniqueness
> > > ---
> > >  src/libcamera/camera_manager.cpp         | 22 +++---
> > >  src/libcamera/include/pipeline_handler.h | 29 ++++----
> > >  src/libcamera/pipeline_handler.cpp       | 95 ++++++++----------------
> > >  3 files changed, 57 insertions(+), 89 deletions(-)
> 
> [snip]
> 
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index 1daada8653e2..08e3291e741a 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> 
> [snip]
> 
> > > +void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)
> > >  {
> > > -	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> > > -
> > > -	auto it = factories.find(name);
> > > -	if (it == factories.end()) {
> > > -		LOG(Error) << "Trying to create non-existing pipeline handler "
> > > -			   << name;
> > > -		return nullptr;
> > > -	}
> > > -
> > > -	PipelineHandler *pipe = it->second->create();
> > > +	std::vector<PipelineHandlerFactory *> &factories = handlers();
> > >  
> > > -	if (pipe->match(enumerator)) {
> > > -		LOG(Debug) << "Pipeline handler \"" << name << "\" matched";
> > > -		return pipe;
> > > -	}
> > > -
> > > -	delete pipe;
> > > -	return nullptr;
> > > -}
> > > -
> > > -/**
> > > - * \brief Retrieve the names of all pipeline handlers registered with the
> > > - * factory
> > > - *
> > > - * \return a list of all registered pipeline handler names
> > > - */
> > > -std::vector<std::string> PipelineHandlerFactory::handlers()
> > > -{
> > > -	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> > > -	std::vector<std::string> handlers;
> > > +	for (PipelineHandlerFactory *f : factories)
> > > +		ASSERT(factory->name() != f->name());
> > 
> > Is this check needed? The name come from the class name passed to 
> > REGISTER_PIPELINE_HANDLER(). If there where to be a duplicated name 
> > would we not get a symbol collision at compile time?
> 
> We would, unless the classes are put in an anonymous namespace. Do you
> think keeping the check is worth it ?

I think we can drop it. As I think it is possible if one really tries to 
create two handlers with the same name anyhow :-P

> 
> > With or without this fixed
> > 
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > 
> > >  
> > > -	for (auto const &handler : factories)
> > > -		handlers.push_back(handler.first);
> > > +	factories.push_back(factory);
> > >  
> > > -	return handlers;
> > > +	LOG(Debug) << "Registered pipeline handler \"" << factory->name() << "\"";
> > >  }
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart