[libcamera-devel] [PATCH v2 4/5] libcamera: pipeline: Allows more expressive names

[Laurent Pinchart]

Hello,

On Wed, Jan 16, 2019 at 04:24:47PM +0100, Niklas Söderlund wrote:
> On 2019-01-16 14:59:48 +0100, Jacopo Mondi wrote:
> > Allow the registration of pipeline handlers with more expressive names
> > than the pipeline handler factory class name.
> > 
> > Result is the following:
> > -DBG pipeline_handler.cpp:119 Registered pipeline handler "PipeHandlerVimc"
> > -DBG pipeline_handler.cpp:119 Registered pipeline handler "PipelineHandlerIPU3"
> > +DBG pipeline_handler.cpp:119 Registered pipeline handler "VIMC virtual driver pipeline handler"
> > +DBG pipeline_handler.cpp:119 Registered pipeline handler "Intel IPU3 pipeline handler"
> 
> What is the intended usage of this name? Would it ever be visible to 
> users other then in this debug message? A application will deal with a 
> list of cameras not pipeline handlers.
> 
> If it's only for debug messages it would prefers to keep the class name 
> as it would save a 'git grep' to find which decorative name corresponds 
> to a file implementing it.

Furthermore the name will also be used to filter, sort or otherwise
configure pipeline handlers based on the libcamera configuration file,
so I think the class name is better.

This being said, we could add a description argument to the
REGISTER_PIPELINE_HANDLER() macro and a description_ field to the
factory if there's a need for a pipeline handler description. If it is
just for the debug message I'm not sure it's worth it.

> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/include/pipeline_handler.h | 4 ++--
> >  src/libcamera/pipeline/vimc.cpp          | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index fdf8b8d..180f599 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -43,12 +43,12 @@ private:
> >  	static std::map<std::string, PipelineHandlerFactory *> &registry();
> >  };
> >  
> > -#define REGISTER_PIPELINE_HANDLER(handler) \
> > +#define REGISTER_PIPELINE_HANDLER(handler, name) \
> >  class handler##Factory : public PipelineHandlerFactory { \
> >  public: \
> >  	handler##Factory() \
> >  	{ \
> > -		PipelineHandlerFactory::registerType(#handler, this); \
> > +		PipelineHandlerFactory::registerType(name, this); \
> >  	} \
> >  	virtual PipelineHandler *create() { \
> >  		return new handler(); \
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 720d9c2..21af902 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -88,6 +88,6 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
> >  	return true;
> >  }
> >  
> > -REGISTER_PIPELINE_HANDLER(PipeHandlerVimc);
> > +REGISTER_PIPELINE_HANDLER(PipeHandlerVimc, "VIMC virtual driver pipeline handler");
> >  
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart