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

[Niklas Söderlund]

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 at ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo at 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 at 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

-- 
Regards,
Niklas Söderlund