[libcamera-devel,v2,4/5] libcamera: pipeline: Allows more expressive names
diff mbox series

Message ID 20190116135949.2097-5-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: pipeline: Add Intel IPU3 pipeline handler
Related show

Commit Message

Jacopo Mondi Jan. 16, 2019, 1:59 p.m. UTC 
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"

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/pipeline_handler.h | 4 ++--
 src/libcamera/pipeline/vimc.cpp          | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Niklas Söderlund Jan. 16, 2019, 3:24 p.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

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.

> 
> Signed-off-by: Jacopo Mondi <jacopo@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 */
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 18, 2019, 12:51 a.m. UTC | #2 
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@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 */

Patch
diff mbox series 

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