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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart