[libcamera-devel,v2,09/12] libcamera: pipeline_handler: add PipelineHandler class

103 diff mbox series

Provide a PipelineHandler which represents a handler for one or more
media devices and provider of one or more cameras.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/pipeline_handler.h |  62 ++++++++++
 src/libcamera/meson.build                |   2 +
 src/libcamera/pipeline_handler.cpp       | 138 +++++++++++++++++++++++
 3 files changed, 202 insertions(+)
 create mode 100644 src/libcamera/include/pipeline_handler.h
 create mode 100644 src/libcamera/pipeline_handler.cpp

Hi Niklas,
  a few things you might want to address before or after pushing this

On Sat, Dec 29, 2018 at 04:28:52AM +0100, Niklas Söderlund wrote:
> Provide a PipelineHandler which represents a handler for one or more
> media devices and provider of one or more cameras.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  62 ++++++++++
>  src/libcamera/meson.build                |   2 +
>  src/libcamera/pipeline_handler.cpp       | 138 +++++++++++++++++++++++
>  3 files changed, 202 insertions(+)
>  create mode 100644 src/libcamera/include/pipeline_handler.h
>  create mode 100644 src/libcamera/pipeline_handler.cpp
>
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> new file mode 100644
> index 0000000000000000..a7805a01e1c779bd
> --- /dev/null
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * pipeline_handler.h - Pipeline handler infrastructure
> + */
> +#ifndef __LIBCAMERA_PIPELINE_HANDLER_H__
> +#define __LIBCAMERA_PIPELINE_HANDLER_H__
> +
> +#include <map>
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/camera.h>
> +
> +namespace libcamera {
> +
> +class DeviceEnumerator;
> +class PipelineHandlerFactory;
> +
> +class PipelineHandler
> +{
> +public:
> +	virtual ~PipelineHandler() { };
> +
> +	virtual bool match(DeviceEnumerator *enumerator) = 0;
> +
> +	virtual unsigned int count() = 0;
> +	virtual Camera *camera(unsigned int id) = 0;
> +};
> +
> +class PipelineHandlerFactory
> +{
> +public:
> +	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 void handlers(std::vector<std::string> &handlers);
> +
> +private:
> +	static std::map<std::string, PipelineHandlerFactory *> &registry();
> +};
> +
> +#define REGISTER_PIPELINE_HANDLER(handler) \
> +	class handler##Factory : public PipelineHandlerFactory { \
> +	public: \
> +		handler##Factory() \
> +		{ \
> +			PipelineHandlerFactory::registerType(#handler, this); \
> +		} \
> +		virtual PipelineHandler *create() { \
> +			return new handler(); \
> +		} \
> +	}; \
> +	static handler##Factory global_##handler##Factory;
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PIPELINE_HANDLER_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 581da1aa78ebb3ba..0db648dd3e37156e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -3,11 +3,13 @@ libcamera_sources = files([
>      'device_enumerator.cpp',
>      'log.cpp',
>      'main.cpp',
> +    'pipeline_handler.cpp',
>  ])
>
>  libcamera_headers = files([
>      'include/device_enumerator.h',
>      'include/log.h',
> +    'include/pipeline_handler.h',
>      'include/utils.h',
>  ])
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> new file mode 100644
> index 0000000000000000..b6e28216a6636faf
> --- /dev/null
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * pipeline_handler.cpp - Pipeline handler infrastructure
> + */
> +
> +#include "device_enumerator.h"
> +#include "log.h"
> +#include "pipeline_handler.h"
> +
> +/**
> + * \file pipeline_handler.h
> + * \brief Create pipelines and cameras from one or more media device

devices

> + *
> + * Each pipeline supported by libcamera needs to be backed by a pipeline
> + * handler implementation which describes the one or many media devices
> + * needed for a pipeline to function properly.
> + *
> + * The pipeline handler is responsible to find all media devices it requires

s/to find/of finding/ ?

> + * to operate and once it retrieves them create all the camera devices
> + * it is able to support with the that set of devices.

s/the//

I would:

The pipeline handler is responsible of providing a description of the
media devices it requires to operates on, and once it gets provided wit
them by the device enumerator it creates all camera devices it is able to
support with that set of devices.

> + *
> + * To make it a bit less bit complicated to write pipe line handlers a
> + * macro REGISTER_PIPELINE_HANDLER() is provided which allows a pipeline
> + * handler implementation to register itself with the library with ease.
> + */
> +
> +namespace libcamera {
> +

nit: two empty lines
> +
> +/**
> + * \class PipelineHandler
> + * \brief Find a set of media devices and provide cameras
> + *
> + * The responsibility of a PipelineHandler is to describe all media
> + * devices it would need in order to provide cameras to the system.
> + */
> +
> +/**
> + * \class PipelineHandlerFactory
> + * \brief Keep a registry and create instances of available pipeline handlers
> + *
> + * The responsibility of the PipelineHandlerFactory is to keep a list
> + * of all pipelines in the system. Each pipeline handler should register
> + * it self with the factory using the REGISTER_PIPELINE_HANDLER() macro.
> + */
> +
> +/**
> + * \brief Add a pipeline handler to the global list
> + *
> + * \param[in] name Name of the pipeline handler to add
> + * \param[in] factory Factory to use to construct the pipeline
> + *
> + * The caller is responsible to guarantee the uniqueness of the pipeline name.
> + */
> +void PipelineHandlerFactory::registerType(const std::string &name, PipelineHandlerFactory *factory)

nit: You could break this...

> +{
> +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> +
> +	if (factories.count(name)) {
> +		LOG(Error) <<  "Registering '" << name << "' pipeline twice";
> +		return;
> +	}
> +
> +	factories[name] = factory;
> +}
> +
> +/**
> + * \brief Create a new pipeline handler and try to match it

and try to match the media devices it requires

> + *
> + * \param[in] name Name of the pipeline handler to try
> + * \param[in] enumerator  Numerator to to search for a match for the handler

s/Numerator/Device enumerator/

> + *
> + * Search \a enumerator for a match for a pipeline handler named \a name.

Try to match the media devices this pipeline requires against the ones
registered in \a enumerator.

> + *
> + * \return Pipeline handler if a match was found else nullptr
s/was found else/was found, nullptr otherwise/

> + */
> +PipelineHandler *PipelineHandlerFactory::create(const std::string &name, DeviceEnumerator *enumerator)

nit: you could break this

> +{
> +	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;

no value in breaking 80 cols here

> +		return nullptr;
> +	}
> +
> +	PipelineHandler *pipe;
> +
> +	pipe = it->second->create();

declare and initialize on the same line.
> +
> +	if (pipe->match(enumerator))
> +		return pipe;
> +
> +	delete pipe;
> +	return nullptr;
> +}
> +
> +/**
> + * \brief List all names of handlers from the global list
> + *
> + * \param[out] handlers Names of all handlers registered with the global list
> + */
> +void PipelineHandlerFactory::handlers(std::vector<std::string> &handlers)
> +{
> +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> +
> +	for (auto const &handler : factories)
> +		handlers.push_back(handler.first);
> +}
> +
> +/**
> + * \brief Static global list of pipeline handlers
> + *
> + * It might seem odd to hide the static map inside a function.
> + * This is needed to make sure the list is created before anyone
> + * tries to access it and creating problems at link time.
> + *
> + * \return Global list of pipeline handlers
> + */
> +std::map<std::string, PipelineHandlerFactory *> &PipelineHandlerFactory::registry()
> +{
> +	static std::map<std::string, PipelineHandlerFactory *> factories;
> +	return factories;
> +}
> +
> +/**
> + * \def REGISTER_PIPELINE_HANDLER
> + * \brief Register a pipeline handler with the global list
> + *
> + * \param[in] handler Class name of PipelineHandler subclass to register

nit: derived class

> + *
> + * Register a specifc pipline handler with the global list and make it
> + * avaiable to try and match devices for libcamera.

s/for libcamera//

Feel free to address this later, this is all minor stuff and I'm fine
merging code as it is.

Thanks
  j


> + */
> +
> +} /* namespace libcamera */
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thanks for your feedback.

On 2018-12-30 11:33:21 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>   a few things you might want to address before or after pushing this
> 
> On Sat, Dec 29, 2018 at 04:28:52AM +0100, Niklas Söderlund wrote:
> > Provide a PipelineHandler which represents a handler for one or more
> > media devices and provider of one or more cameras.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/include/pipeline_handler.h |  62 ++++++++++
> >  src/libcamera/meson.build                |   2 +
> >  src/libcamera/pipeline_handler.cpp       | 138 +++++++++++++++++++++++
> >  3 files changed, 202 insertions(+)
> >  create mode 100644 src/libcamera/include/pipeline_handler.h
> >  create mode 100644 src/libcamera/pipeline_handler.cpp
> >
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > new file mode 100644
> > index 0000000000000000..a7805a01e1c779bd
> > --- /dev/null
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * pipeline_handler.h - Pipeline handler infrastructure
> > + */
> > +#ifndef __LIBCAMERA_PIPELINE_HANDLER_H__
> > +#define __LIBCAMERA_PIPELINE_HANDLER_H__
> > +
> > +#include <map>
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <libcamera/camera.h>
> > +
> > +namespace libcamera {
> > +
> > +class DeviceEnumerator;
> > +class PipelineHandlerFactory;
> > +
> > +class PipelineHandler
> > +{
> > +public:
> > +	virtual ~PipelineHandler() { };
> > +
> > +	virtual bool match(DeviceEnumerator *enumerator) = 0;
> > +
> > +	virtual unsigned int count() = 0;
> > +	virtual Camera *camera(unsigned int id) = 0;
> > +};
> > +
> > +class PipelineHandlerFactory
> > +{
> > +public:
> > +	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 void handlers(std::vector<std::string> &handlers);
> > +
> > +private:
> > +	static std::map<std::string, PipelineHandlerFactory *> &registry();
> > +};
> > +
> > +#define REGISTER_PIPELINE_HANDLER(handler) \
> > +	class handler##Factory : public PipelineHandlerFactory { \
> > +	public: \
> > +		handler##Factory() \
> > +		{ \
> > +			PipelineHandlerFactory::registerType(#handler, this); \
> > +		} \
> > +		virtual PipelineHandler *create() { \
> > +			return new handler(); \
> > +		} \
> > +	}; \
> > +	static handler##Factory global_##handler##Factory;
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_PIPELINE_HANDLER_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 581da1aa78ebb3ba..0db648dd3e37156e 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -3,11 +3,13 @@ libcamera_sources = files([
> >      'device_enumerator.cpp',
> >      'log.cpp',
> >      'main.cpp',
> > +    'pipeline_handler.cpp',
> >  ])
> >
> >  libcamera_headers = files([
> >      'include/device_enumerator.h',
> >      'include/log.h',
> > +    'include/pipeline_handler.h',
> >      'include/utils.h',
> >  ])
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > new file mode 100644
> > index 0000000000000000..b6e28216a6636faf
> > --- /dev/null
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -0,0 +1,138 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * pipeline_handler.cpp - Pipeline handler infrastructure
> > + */
> > +
> > +#include "device_enumerator.h"
> > +#include "log.h"
> > +#include "pipeline_handler.h"
> > +
> > +/**
> > + * \file pipeline_handler.h
> > + * \brief Create pipelines and cameras from one or more media device
> 
> devices
> 

Thanks.

> > + *
> > + * Each pipeline supported by libcamera needs to be backed by a pipeline
> > + * handler implementation which describes the one or many media devices
> > + * needed for a pipeline to function properly.
> > + *
> > + * The pipeline handler is responsible to find all media devices it requires
> 
> s/to find/of finding/ ?
> 
> > + * to operate and once it retrieves them create all the camera devices
> > + * it is able to support with the that set of devices.
> 
> s/the//
> 
> I would:
> 
> The pipeline handler is responsible of providing a description of the
> media devices it requires to operates on, and once it gets provided wit
> them by the device enumerator it creates all camera devices it is able to
> support with that set of devices.

Some parts of your text is better then mine, I have merged to in my view 
best of both and ended up with:

 The pipeline handler is responsible of providing a description of the
 media devices it requires to operate. Once all media devices can be
 provided the pipeline handler can acquire them and create camera
 devices which utilize the acquired media devices.

> 
> > + *
> > + * To make it a bit less bit complicated to write pipe line handlers a
> > + * macro REGISTER_PIPELINE_HANDLER() is provided which allows a pipeline
> > + * handler implementation to register itself with the library with ease.
> > + */
> > +
> > +namespace libcamera {
> > +
> 
> nit: two empty lines

Thanks.

> > +
> > +/**
> > + * \class PipelineHandler
> > + * \brief Find a set of media devices and provide cameras
> > + *
> > + * The responsibility of a PipelineHandler is to describe all media
> > + * devices it would need in order to provide cameras to the system.
> > + */
> > +
> > +/**
> > + * \class PipelineHandlerFactory
> > + * \brief Keep a registry and create instances of available pipeline handlers
> > + *
> > + * The responsibility of the PipelineHandlerFactory is to keep a list
> > + * of all pipelines in the system. Each pipeline handler should register
> > + * it self with the factory using the REGISTER_PIPELINE_HANDLER() macro.
> > + */
> > +
> > +/**
> > + * \brief Add a pipeline handler to the global list
> > + *
> > + * \param[in] name Name of the pipeline handler to add
> > + * \param[in] factory Factory to use to construct the pipeline
> > + *
> > + * The caller is responsible to guarantee the uniqueness of the pipeline name.
> > + */
> > +void PipelineHandlerFactory::registerType(const std::string &name, PipelineHandlerFactory *factory)
> 
> nit: You could break this...
> 

Done.

> > +{
> > +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> > +
> > +	if (factories.count(name)) {
> > +		LOG(Error) <<  "Registering '" << name << "' pipeline twice";
> > +		return;
> > +	}
> > +
> > +	factories[name] = factory;
> > +}
> > +
> > +/**
> > + * \brief Create a new pipeline handler and try to match it
> 
> and try to match the media devices it requires
> 

Much better, thanks.

> > + *
> > + * \param[in] name Name of the pipeline handler to try
> > + * \param[in] enumerator  Numerator to to search for a match for the handler
> 
> s/Numerator/Device enumerator/

Wops :-)

> 
> > + *
> > + * Search \a enumerator for a match for a pipeline handler named \a name.
> 
> Try to match the media devices this pipeline requires against the ones
> registered in \a enumerator.

Thanks.

> 
> > + *
> > + * \return Pipeline handler if a match was found else nullptr
> s/was found else/was found, nullptr otherwise/

Thanks.

> 
> > + */
> > +PipelineHandler *PipelineHandlerFactory::create(const std::string &name, DeviceEnumerator *enumerator)
> 
> nit: you could break this

Done.

> 
> > +{
> > +	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;
> 
> no value in breaking 80 cols here

Done.

> 
> > +		return nullptr;
> > +	}
> > +
> > +	PipelineHandler *pipe;
> > +
> > +	pipe = it->second->create();
> 
> declare and initialize on the same line.

Done.

> > +
> > +	if (pipe->match(enumerator))
> > +		return pipe;
> > +
> > +	delete pipe;
> > +	return nullptr;
> > +}
> > +
> > +/**
> > + * \brief List all names of handlers from the global list
> > + *
> > + * \param[out] handlers Names of all handlers registered with the global list
> > + */
> > +void PipelineHandlerFactory::handlers(std::vector<std::string> &handlers)
> > +{
> > +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> > +
> > +	for (auto const &handler : factories)
> > +		handlers.push_back(handler.first);
> > +}
> > +
> > +/**
> > + * \brief Static global list of pipeline handlers
> > + *
> > + * It might seem odd to hide the static map inside a function.
> > + * This is needed to make sure the list is created before anyone
> > + * tries to access it and creating problems at link time.
> > + *
> > + * \return Global list of pipeline handlers
> > + */
> > +std::map<std::string, PipelineHandlerFactory *> &PipelineHandlerFactory::registry()
> > +{
> > +	static std::map<std::string, PipelineHandlerFactory *> factories;
> > +	return factories;
> > +}
> > +
> > +/**
> > + * \def REGISTER_PIPELINE_HANDLER
> > + * \brief Register a pipeline handler with the global list
> > + *
> > + * \param[in] handler Class name of PipelineHandler subclass to register
> 
> nit: derived class
> 
> > + *
> > + * Register a specifc pipline handler with the global list and make it
> > + * avaiable to try and match devices for libcamera.
> 
> s/for libcamera//

Thanks. I also fixed s/specifc/specific/ s/pipline/pipeline/ and 
s/avaiable/available/. Seems my spellchecker was not enabled at the 
time... :-)

> 
> Feel free to address this later, this is all minor stuff and I'm fine
> merging code as it is.

Does this mean I can add your review tag with all of the above fixed?

> 
> Thanks
>   j
> 
> 
> > + */
> > +
> > +} /* namespace libcamera */
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hello,

On Sunday, 30 December 2018 22:23:53 EET Niklas Söderlund wrote:
> On 2018-12-30 11:33:21 +0100, Jacopo Mondi wrote:
> > On Sat, Dec 29, 2018 at 04:28:52AM +0100, Niklas Söderlund wrote:
> > > Provide a PipelineHandler which represents a handler for one or more
> > > media devices and provider of one or more cameras.

s/provider/provides/

> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > > 
> > >  src/libcamera/include/pipeline_handler.h |  62 ++++++++++
> > >  src/libcamera/meson.build                |   2 +
> > >  src/libcamera/pipeline_handler.cpp       | 138 +++++++++++++++++++++++
> > >  3 files changed, 202 insertions(+)
> > >  create mode 100644 src/libcamera/include/pipeline_handler.h
> > >  create mode 100644 src/libcamera/pipeline_handler.cpp
> > > 
> > > diff --git a/src/libcamera/include/pipeline_handler.h
> > > b/src/libcamera/include/pipeline_handler.h new file mode 100644
> > > index 0000000000000000..a7805a01e1c779bd
> > > --- /dev/null
> > > +++ b/src/libcamera/include/pipeline_handler.h
> > > @@ -0,0 +1,62 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * pipeline_handler.h - Pipeline handler infrastructure
> > > + */
> > > +#ifndef __LIBCAMERA_PIPELINE_HANDLER_H__
> > > +#define __LIBCAMERA_PIPELINE_HANDLER_H__
> > > +
> > > +#include <map>
> > > +#include <string>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/camera.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class DeviceEnumerator;
> > > +class PipelineHandlerFactory;
> > > +
> > > +class PipelineHandler
> > > +{
> > > +public:
> > > +	virtual ~PipelineHandler() { };
> > > +
> > > +	virtual bool match(DeviceEnumerator *enumerator) = 0;
> > > +
> > > +	virtual unsigned int count() = 0;
> > > +	virtual Camera *camera(unsigned int id) = 0;
> > > +};
> > > +
> > > +class PipelineHandlerFactory
> > > +{
> > > +public:
> > > +	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 void handlers(std::vector<std::string> &handlers);
> > > +
> > > +private:
> > > +	static std::map<std::string, PipelineHandlerFactory *> &registry();
> > > +};
> > > +
> > > +#define REGISTER_PIPELINE_HANDLER(handler) \
> > > +	class handler##Factory : public PipelineHandlerFactory { \
> > > +	public: \
> > > +		handler##Factory() \
> > > +		{ \
> > > +			PipelineHandlerFactory::registerType(#handler, this); \
> > > +		} \
> > > +		virtual PipelineHandler *create() { \
> > > +			return new handler(); \
> > > +		} \
> > > +	}; \
> > > +	static handler##Factory global_##handler##Factory;
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_PIPELINE_HANDLER_H__ */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 581da1aa78ebb3ba..0db648dd3e37156e 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -3,11 +3,13 @@ libcamera_sources = files([
> > >      'device_enumerator.cpp',
> > >      'log.cpp',
> > >      'main.cpp',
> > > +    'pipeline_handler.cpp',
> > >  ])
> > >  
> > >  libcamera_headers = files([
> > >      'include/device_enumerator.h',
> > >      'include/log.h',
> > > +    'include/pipeline_handler.h',
> > >      'include/utils.h',
> > >  ])
> > > 
> > > diff --git a/src/libcamera/pipeline_handler.cpp
> > > b/src/libcamera/pipeline_handler.cpp new file mode 100644
> > > index 0000000000000000..b6e28216a6636faf
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -0,0 +1,138 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * pipeline_handler.cpp - Pipeline handler infrastructure
> > > + */
> > > +
> > > +#include "device_enumerator.h"
> > > +#include "log.h"
> > > +#include "pipeline_handler.h"
> > > +
> > > +/**
> > > + * \file pipeline_handler.h
> > > + * \brief Create pipelines and cameras from one or more media device
> > 
> > devices
> 
> Thanks.
> 
> > > + *
> > > + * Each pipeline supported by libcamera needs to be backed by a
> > > pipeline
> > > + * handler implementation which describes the one or many media devices
> > > + * needed for a pipeline to function properly.
> > > + *
> > > + * The pipeline handler is responsible to find all media devices it
> > > requires
> > 
> > s/to find/of finding/ ?
> > 
> > > + * to operate and once it retrieves them create all the camera devices
> > > + * it is able to support with the that set of devices.
> > 
> > s/the//
> > 
> > I would:
> > 
> > The pipeline handler is responsible of providing a description of the
> > media devices it requires to operates on, and once it gets provided wit
> > them by the device enumerator it creates all camera devices it is able to
> > support with that set of devices.
> 
> Some parts of your text is better then mine, I have merged to in my view
> best of both and ended up with:
> 
>  The pipeline handler is responsible of providing a description of the

s/of providing/for providing/

>  media devices it requires to operate. Once all media devices can be
>  provided the pipeline handler can acquire them and create camera
>  devices which utilize the acquired media devices.

s/which/that/

> > > + *
> > > + * To make it a bit less bit complicated to write pipe line handlers a
> > > + * macro REGISTER_PIPELINE_HANDLER() is provided which allows a
> > > pipeline
> > > + * handler implementation to register itself with the library with
> > > ease.
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > 
> > nit: two empty lines
> 
> Thanks.
> 
> > > +
> > > +/**
> > > + * \class PipelineHandler
> > > + * \brief Find a set of media devices and provide cameras
> > > + *
> > > + * The responsibility of a PipelineHandler is to describe all media
> > > + * devices it would need in order to provide cameras to the system.
> > > + */
> > > +
> > > +/**
> > > + * \class PipelineHandlerFactory
> > > + * \brief Keep a registry and create instances of available pipeline
> > > handlers + *
> > > + * The responsibility of the PipelineHandlerFactory is to keep a list
> > > + * of all pipelines in the system. Each pipeline handler should
> > > register
> > > + * it self with the factory using the REGISTER_PIPELINE_HANDLER()
> > > macro.
> > > + */
> > > +
> > > +/**
> > > + * \brief Add a pipeline handler to the global list
> > > + *
> > > + * \param[in] name Name of the pipeline handler to add
> > > + * \param[in] factory Factory to use to construct the pipeline
> > > + *
> > > + * The caller is responsible to guarantee the uniqueness of the
> > > pipeline name.
> > > + */
> > > +void PipelineHandlerFactory::registerType(const std::string &name,
> > > PipelineHandlerFactory *factory)
> > 
> > nit: You could break this...
> 
> Done.
> 
> > > +{
> > > +	std::map<std::string, PipelineHandlerFactory *> &factories =
> > > registry();
> > > +
> > > +	if (factories.count(name)) {
> > > +		LOG(Error) <<  "Registering '" << name << "' pipeline twice";
> > > +		return;
> > > +	}
> > > +
> > > +	factories[name] = factory;
> > > +}
> > > +
> > > +/**
> > > + * \brief Create a new pipeline handler and try to match it
> > 
> > and try to match the media devices it requires
> 
> Much better, thanks.
> 
> > > + *
> > > + * \param[in] name Name of the pipeline handler to try
> > > + * \param[in] enumerator  Numerator to to search for a match for the
> > > handler
> > 
> > s/Numerator/Device enumerator/
> 
> Wops :-)

And s/to to/to/

> > > + *
> > > + * Search \a enumerator for a match for a pipeline handler named \a
> > > name.
> > 
> > Try to match the media devices this pipeline requires against the ones
> > registered in \a enumerator.
> 
> Thanks.
> 
> > > + *
> > > + * \return Pipeline handler if a match was found else nullptr
> > 
> > s/was found else/was found, nullptr otherwise/
> 
> Thanks.
> 
> > > + */
> > > +PipelineHandler *PipelineHandlerFactory::create(const std::string
> > > &name, DeviceEnumerator *enumerator)
> > 
> > nit: you could break this
> 
> Done.
> 
> > > +{
> > > +	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;
> > 
> > no value in breaking 80 cols here
> 
> Done.
> 
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	PipelineHandler *pipe;
> > > +
> > > +	pipe = it->second->create();
> > 
> > declare and initialize on the same line.
> 
> Done.
> 
> > > +
> > > +	if (pipe->match(enumerator))
> > > +		return pipe;
> > > +
> > > +	delete pipe;
> > > +	return nullptr;
> > > +}
> > > +
> > > +/**
> > > + * \brief List all names of handlers from the global list
> > > + *
> > > + * \param[out] handlers Names of all handlers registered with the
> > > global list
> > > + */
> > > +void PipelineHandlerFactory::handlers(std::vector<std::string>
> > > &handlers)
> > > +{
> > > +	std::map<std::string, PipelineHandlerFactory *> &factories =
> > > registry();
> > > +
> > > +	for (auto const &handler : factories)
> > > +		handlers.push_back(handler.first);
> > > +}
> > > +
> > > +/**
> > > + * \brief Static global list of pipeline handlers
> > > + *
> > > + * It might seem odd to hide the static map inside a function.
> > > + * This is needed to make sure the list is created before anyone
> > > + * tries to access it and creating problems at link time.

No need to apologize about the oddity :-)

"The static factories map is defined inside the function to ensures it gets 
initialized on first use, without any dependency on link order."

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > + * \return Global list of pipeline handlers
> > > + */
> > > +std::map<std::string, PipelineHandlerFactory *>
> > > &PipelineHandlerFactory::registry()
> > > +{
> > > +	static std::map<std::string, PipelineHandlerFactory *> factories;
> > > +	return factories;
> > > +}
> > > +
> > > +/**
> > > + * \def REGISTER_PIPELINE_HANDLER
> > > + * \brief Register a pipeline handler with the global list
> > > + *
> > > + * \param[in] handler Class name of PipelineHandler subclass to
> > > register
> > 
> > nit: derived class
> > 
> > > + *
> > > + * Register a specifc pipline handler with the global list and make it
> > > + * avaiable to try and match devices for libcamera.
> > 
> > s/for libcamera//
> 
> Thanks. I also fixed s/specifc/specific/ s/pipline/pipeline/ and
> s/avaiable/available/. Seems my spellchecker was not enabled at the
> time... :-)
> 
> > Feel free to address this later, this is all minor stuff and I'm fine
> > merging code as it is.
> 
> Does this mean I can add your review tag with all of the above fixed?
> 
> > > + */
> > > +
> > > +} /* namespace libcamera */

Hi Laurent,

All grammar comments addressed, thanks!

On 2018-12-30 23:15:43 +0200, Laurent Pinchart wrote:

[snip]

> > > > +/**
> > > > + * \brief Static global list of pipeline handlers
> > > > + *
> > > > + * It might seem odd to hide the static map inside a function.
> > > > + * This is needed to make sure the list is created before anyone
> > > > + * tries to access it and creating problems at link time.
> 
> No need to apologize about the oddity :-)
> 
> "The static factories map is defined inside the function to ensures it gets 
> initialized on first use, without any dependency on link order."

Much better! Thanks for providing this and your tag!

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>