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

85 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/pipelinehandler.h |  71 ++++++++++++++
 src/libcamera/meson.build               |   2 +
 src/libcamera/pipelinehandler.cpp       | 122 ++++++++++++++++++++++++
 3 files changed, 195 insertions(+)
 create mode 100644 src/libcamera/include/pipelinehandler.h
 create mode 100644 src/libcamera/pipelinehandler.cpp

Hi Niklas,

Thank you for the patch.

On Sunday, 23 December 2018 01:00:38 EET 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/pipelinehandler.h |  71 ++++++++++++++
>  src/libcamera/meson.build               |   2 +
>  src/libcamera/pipelinehandler.cpp       | 122 ++++++++++++++++++++++++
>  3 files changed, 195 insertions(+)
>  create mode 100644 src/libcamera/include/pipelinehandler.h
>  create mode 100644 src/libcamera/pipelinehandler.cpp
> 
> diff --git a/src/libcamera/include/pipelinehandler.h
> b/src/libcamera/include/pipelinehandler.h new file mode 100644
> index 0000000000000000..0e2f497a4fda3caa
> --- /dev/null
> +++ b/src/libcamera/include/pipelinehandler.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * pipelinehandler.h - Pipeline handler infrastructure
> + */
> +#ifndef __LIBCAMERA_PIPELINEHANDLER_H__
> +#define __LIBCAMERA_PIPELINEHANDLER_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;
> +
> +	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);

Don't those three methods belong to PipelineHandlerFactory ? It seems more 
logical to me to have creation handled by the factory.

> +private:
> +	static std::map<std::string, PipelineHandlerFactory *> &registry();
> +};
> +
> +class PipelineHandlerFactory
> +{
> +public:
> +

No need for a blank line.

> +	virtual ~PipelineHandlerFactory() { };
> +
> +	virtual PipelineHandler *create() = 0;
> +};
> +
> +/**
> + * \brief Register a pipeline hander with the global list
> + *
> + * \param[in] handler Class name of PipelineHandler subclass to register
> + *
> + * Register a specifc pipline handler with the global list and make it
> + * avaiable to try and match devices for libcamera.
> + */

Let's move the documentation to the .cpp file.

> +#define REGISTER_PIPELINE(handler) \

REGISTER_PIPELINE_HANDLER ?

> +	class handler##Factory : public PipelineHandlerFactory { \
> +	public: \
> +		handler##Factory() \
> +		{ \
> +			PipelineHandler::registerType(#handler, this); \
> +		} \
> +		virtual PipelineHandler *create() { \
> +			return new handler(); \
> +		} \
> +	}; \
> +	static handler##Factory global_##handler##Factory;
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PIPELINEHANDLER_H__ */

[snip]

> diff --git a/src/libcamera/pipelinehandler.cpp
> b/src/libcamera/pipelinehandler.cpp new file mode 100644
> index 0000000000000000..3c47f1ceb72eb6f6
> --- /dev/null
> +++ b/src/libcamera/pipelinehandler.cpp
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * pipelinehandler.cpp - Pipeline handler infrastructure
> + */
> +
> +#include "deviceenumerator.h"
> +#include "log.h"
> +#include "pipelinehandler.h"
> +
> +/**
> + * \file pipelinehandler.h
> + * \brief Create pipelines and cameras from one or more media device
> + *
> + * 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 + * to operate and once it retrieves them create all the camera
> devices + * it is able to support with the that set of devices.
> + *
> + * To make it a bit less bit complicated to write pipe line handlers a
> + * important macro REGISTER_PIPELINE() is provided which allows a pipeline
> + * hander implementation to register itself with the library with ease.
> + *
> + * \todo Figure out how and if the PipelineHandler should be involved in
> + *       controlling cameras with resource dependencies and if the handler
> + *       should provide helpers for cameras to control media graph links.
> + */
> +
> +namespace libcamera {
> +
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \brief Add a pipeline hander 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 PipelineHandler::registerType(const std::string &name,
> PipelineHandlerFactory *factory)
> +{
> +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();

How about adding a name member variable to the PipelineHandlerFactory class 
and storing factories in an std::vector ? You could then return the full list 
of factories to CameraManager and pass a factory pointer to 
PipelineHandler::create(), avoiding the lookup by name operation.

> +	if (factories.count(name)) {
> +		LOG(Error) <<  "Registering '" << name << "' pipeline twice";
> +		return;
> +	}
> +
> +	factories[name] = factory;
> +}
> +
> +/**
> + * \brief Create a new pipeline hander and try to match it

s/hander/handler/

> + *
> + * \param[in] name Name of the pipeline handler to try
> + * \param[in] enumerator  Numerator to to search for a match for the
> handler
> + *
> + * Search \a enumerator for a match for a pipeline handler named \a name.
> + *
> + * \return Pipeline handler if a match was found else NULL
> + */
> +PipelineHandler *PipelineHandler::create(const std::string &name,
> DeviceEnumerator *enumerator)
> +{
> +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> +
> +	if (!factories.count(name)) {
> +		LOG(Error) << "Trying to create non-existing pipeline handler " << name;
> +		return NULL;
> +	}
> +
> +	PipelineHandler *pipe;
> +
> +	pipe = factories[name]->create();

Use an iterator to lookup in one operation, removing the need to count() and 
[] separately.

> +
> +	if (pipe->match(enumerator))
> +		return pipe;
> +
> +	delete pipe;
> +	return NULL;

I wonder whether REGISTER_PIPELINE should register a new instance of the 
PipelineHandler instead of the PipelineHandlerFactory. This would avoid 
creating and deleting instance in a tight loop during matching, and would 
allow for storing data shared by multiple pipelines from the same pipeline 
handler (I'm not sure we'll have a use case for this though). The pipeline 
handler would create one pipeline per match, and pipelines would be handled 
internally in the pipeline handler and not exposed to the camera manager. The 
pipeline handler would then need to register the camera instances it creates 
with the camera manager, as the camera manager wouldn't have a list of pipes 
anymore.

> +}
> +
> +/**
> + * \brief List all names of handlers from the global list
> + *
> + * \param[out] handlers Names of all handlers registered with the global
> list + */
> +void PipelineHandler::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 *>
> &PipelineHandler::registry()
> +{
> +	static std::map<std::string, PipelineHandlerFactory *> factories;
> +	return factories;
> +}
> +
> +} /* namespace libcamera */

Hi Laurent,

Thanks for your comments.

On 2018-12-29 02:37:38 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sunday, 23 December 2018 01:00:38 EET 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/pipelinehandler.h |  71 ++++++++++++++
> >  src/libcamera/meson.build               |   2 +
> >  src/libcamera/pipelinehandler.cpp       | 122 ++++++++++++++++++++++++
> >  3 files changed, 195 insertions(+)
> >  create mode 100644 src/libcamera/include/pipelinehandler.h
> >  create mode 100644 src/libcamera/pipelinehandler.cpp
> > 
> > diff --git a/src/libcamera/include/pipelinehandler.h
> > b/src/libcamera/include/pipelinehandler.h new file mode 100644
> > index 0000000000000000..0e2f497a4fda3caa
> > --- /dev/null
> > +++ b/src/libcamera/include/pipelinehandler.h
> > @@ -0,0 +1,71 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * pipelinehandler.h - Pipeline handler infrastructure
> > + */
> > +#ifndef __LIBCAMERA_PIPELINEHANDLER_H__
> > +#define __LIBCAMERA_PIPELINEHANDLER_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;
> > +
> > +	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);
> 
> Don't those three methods belong to PipelineHandlerFactory ? It seems more 
> logical to me to have creation handled by the factory.

Agreed, they are moved for v2.


> 
> > +private:
> > +	static std::map<std::string, PipelineHandlerFactory *> &registry();
> > +};
> > +
> > +class PipelineHandlerFactory
> > +{
> > +public:
> > +
> 
> No need for a blank line.

Thanks.

> 
> > +	virtual ~PipelineHandlerFactory() { };
> > +
> > +	virtual PipelineHandler *create() = 0;
> > +};
> > +
> > +/**
> > + * \brief Register a pipeline hander with the global list
> > + *
> > + * \param[in] handler Class name of PipelineHandler subclass to register
> > + *
> > + * Register a specifc pipline handler with the global list and make it
> > + * avaiable to try and match devices for libcamera.
> > + */
> 
> Let's move the documentation to the .cpp file.

Done.

> 
> > +#define REGISTER_PIPELINE(handler) \
> 
> REGISTER_PIPELINE_HANDLER ?

Yes.

> 
> > +	class handler##Factory : public PipelineHandlerFactory { \
> > +	public: \
> > +		handler##Factory() \
> > +		{ \
> > +			PipelineHandler::registerType(#handler, this); \
> > +		} \
> > +		virtual PipelineHandler *create() { \
> > +			return new handler(); \
> > +		} \
> > +	}; \
> > +	static handler##Factory global_##handler##Factory;
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_PIPELINEHANDLER_H__ */
> 
> [snip]
> 
> > diff --git a/src/libcamera/pipelinehandler.cpp
> > b/src/libcamera/pipelinehandler.cpp new file mode 100644
> > index 0000000000000000..3c47f1ceb72eb6f6
> > --- /dev/null
> > +++ b/src/libcamera/pipelinehandler.cpp
> > @@ -0,0 +1,122 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * pipelinehandler.cpp - Pipeline handler infrastructure
> > + */
> > +
> > +#include "deviceenumerator.h"
> > +#include "log.h"
> > +#include "pipelinehandler.h"
> > +
> > +/**
> > + * \file pipelinehandler.h
> > + * \brief Create pipelines and cameras from one or more media device
> > + *
> > + * 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 + * to operate and once it retrieves them create all the camera
> > devices + * it is able to support with the that set of devices.
> > + *
> > + * To make it a bit less bit complicated to write pipe line handlers a
> > + * important macro REGISTER_PIPELINE() is provided which allows a pipeline
> > + * hander implementation to register itself with the library with ease.
> > + *
> > + * \todo Figure out how and if the PipelineHandler should be involved in
> > + *       controlling cameras with resource dependencies and if the handler
> > + *       should provide helpers for cameras to control media graph links.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +
> > +/**
> > + * \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.
> > + */
> > +
> > +/**
> > + * \brief Add a pipeline hander 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 PipelineHandler::registerType(const std::string &name,
> > PipelineHandlerFactory *factory)
> > +{
> > +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> 
> How about adding a name member variable to the PipelineHandlerFactory class 
> and storing factories in an std::vector ? You could then return the full list 
> of factories to CameraManager and pass a factory pointer to 
> PipelineHandler::create(), avoiding the lookup by name operation.

This is a nice idea, I would like to implement the configuration file 
which allows the user to specify which pipeline handlers should be used 
or if all available should be used. I had this in mind when implementing 
this in the first place. I made a note of it and will think of it when I 
add the configuration feature.

> 
> > +	if (factories.count(name)) {
> > +		LOG(Error) <<  "Registering '" << name << "' pipeline twice";
> > +		return;
> > +	}
> > +
> > +	factories[name] = factory;
> > +}
> > +
> > +/**
> > + * \brief Create a new pipeline hander and try to match it
> 
> s/hander/handler/

Wops, I manage to misspell this in quiet a few locations.

> 
> > + *
> > + * \param[in] name Name of the pipeline handler to try
> > + * \param[in] enumerator  Numerator to to search for a match for the
> > handler
> > + *
> > + * Search \a enumerator for a match for a pipeline handler named \a name.
> > + *
> > + * \return Pipeline handler if a match was found else NULL
> > + */
> > +PipelineHandler *PipelineHandler::create(const std::string &name,
> > DeviceEnumerator *enumerator)
> > +{
> > +	std::map<std::string, PipelineHandlerFactory *> &factories = registry();
> > +
> > +	if (!factories.count(name)) {
> > +		LOG(Error) << "Trying to create non-existing pipeline handler " << name;
> > +		return NULL;
> > +	}
> > +
> > +	PipelineHandler *pipe;
> > +
> > +	pipe = factories[name]->create();
> 
> Use an iterator to lookup in one operation, removing the need to count() and 
> [] separately.

Thanks.

> 
> > +
> > +	if (pipe->match(enumerator))
> > +		return pipe;
> > +
> > +	delete pipe;
> > +	return NULL;
> 
> I wonder whether REGISTER_PIPELINE should register a new instance of the 
> PipelineHandler instead of the PipelineHandlerFactory. This would avoid 
> creating and deleting instance in a tight loop during matching, and would 
> allow for storing data shared by multiple pipelines from the same pipeline 
> handler (I'm not sure we'll have a use case for this though). The pipeline 
> handler would create one pipeline per match, and pipelines would be handled 
> internally in the pipeline handler and not exposed to the camera manager. The 
> pipeline handler would then need to register the camera instances it creates 
> with the camera manager, as the camera manager wouldn't have a list of pipes 
> anymore.

The current design is that each pipeline handler is fully independent.  
If a pipeline requires more then one media device where resources are 
shard between two semi independent video sources, the pipeline handler 
should still request all media devices needed and provide two or more 
cameras. This is supported in the design in this series and for now I 
feel this approach is simpler.

I do see your point that there is waste in creating and destroying 
objects in the matching loop. I will meditate a bit over this and see 
what can be done.

> 
> > +}
> > +
> > +/**
> > + * \brief List all names of handlers from the global list
> > + *
> > + * \param[out] handlers Names of all handlers registered with the global
> > list + */
> > +void PipelineHandler::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 *>
> > &PipelineHandler::registry()
> > +{
> > +	static std::map<std::string, PipelineHandlerFactory *> factories;
> > +	return factories;
> > +}
> > +
> > +} /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
>