[libcamera-devel,v2,10/12] libcamera: camera_manager: add CameraManager class

105 diff mbox series

Provide a CameraManager class which will handle listing, instancing,
destruction and lifetime management of cameras.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/camera_manager.h |  38 +++++++
 include/libcamera/libcamera.h      |   1 +
 include/libcamera/meson.build      |   1 +
 src/libcamera/camera_manager.cpp   | 166 +++++++++++++++++++++++++++++
 src/libcamera/meson.build          |   1 +
 5 files changed, 207 insertions(+)
 create mode 100644 include/libcamera/camera_manager.h
 create mode 100644 src/libcamera/camera_manager.cpp

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

On Sat, Dec 29, 2018 at 04:28:53AM +0100, Niklas Söderlund wrote:
> Provide a CameraManager class which will handle listing, instancing,
> destruction and lifetime management of cameras.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera_manager.h |  38 +++++++
>  include/libcamera/libcamera.h      |   1 +
>  include/libcamera/meson.build      |   1 +
>  src/libcamera/camera_manager.cpp   | 166 +++++++++++++++++++++++++++++
>  src/libcamera/meson.build          |   1 +
>  5 files changed, 207 insertions(+)
>  create mode 100644 include/libcamera/camera_manager.h
>  create mode 100644 src/libcamera/camera_manager.cpp
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> new file mode 100644
> index 0000000000000000..9d032d71c4c0841a
> --- /dev/null
> +++ b/include/libcamera/camera_manager.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * camera_manager.h - Camera management
> + */
> +#ifndef __LIBCAMERA_CAMERA_MANAGER_H__
> +#define __LIBCAMERA_CAMERA_MANAGER_H__
> +
> +#include <vector>
> +#include <string>
> +
> +namespace libcamera {
> +
> +class Camera;
> +class DeviceEnumerator;
> +class PipelineHandler;
> +
> +class CameraManager
> +{
> +public:
> +	CameraManager();
> +
> +	int start();
> +	void stop();
> +
> +	std::vector<std::string> list() const;
> +	Camera *get(const std::string &name);
> +	void put(Camera *camera);
> +
> +private:
> +	DeviceEnumerator *enumerator_;
> +	std::vector<PipelineHandler *> pipes_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CAMERA_MANAGER_H__ */
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index 44c094d92feed5ba..32fb1ff741a7b97a 100644
> --- a/include/libcamera/libcamera.h
> +++ b/include/libcamera/libcamera.h
> @@ -8,6 +8,7 @@
>  #define __LIBCAMERA_LIBCAMERA_H__
>
>  #include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
>
>  namespace libcamera {
>
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 9b266ad926681db9..3e04557d66b1a8f4 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_api = files([
>      'camera.h',
> +    'camera_manager.h',
>      'libcamera.h',
>  ])
>
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> new file mode 100644
> index 0000000000000000..1160381b72a08850
> --- /dev/null
> +++ b/src/libcamera/camera_manager.cpp
> @@ -0,0 +1,166 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * camera_manager.h - Camera management
> + */
> +
> +#include <libcamera/camera_manager.h>
> +
> +#include "device_enumerator.h"
> +#include "pipeline_handler.h"
> +
> +/**
> + * \file camera_manager.h
> + * \brief Manage all cameras handled by libcamera
> + *
> + * The responsibility of the camera manager is to control the lifetime
> + * management of objects provided by libcamera.
> + *
> + * When a user wish to interact with libcamera it creates and starts a
> + * CameraManager object. Once confirmed the camera manager is running
> + * the application can list all cameras detected by the library, get
> + * one or more of the cameras and interact with them.
> + *
> + * When the user is done with the camera it should be returned to the
> + * camera manager. Once all cameras are returned to the camera manager
> + * the user is free to stop the manager.
> + *
> + * \todo Add ability to add and remove media devices based on
> + *       hot-(un)plug events coming from the device enumerator.
> + *
> + * \todo Add interface to register a notification callback to the user
> + *       to be able to inform it new cameras have been hot-plugged or
> + *       cameras have been removed due to hot-unplug.
> + */
> +
> +namespace libcamera {
> +
> +CameraManager::CameraManager()
> +	: enumerator_(nullptr)
> +{
> +}
> +
> +/**
> + * \brief Start the camera manager
> + *
> + * Start the camera manager and enumerate all devices in the system. Once
> + * the start have been confirmed the user is free to list and otherwise
> + * interact with cameras in the system until either the camera manager
> + * is stopped or the camera is unplugged from the system.
> + *
> + * \return true on successful start false otherwise
> + */
> +int CameraManager::start()
> +{
> +	std::vector<std::string> handlers;
> +
> +	if (enumerator_)
> +		return -ENODEV;

EBUSY?
Do we want the enumerator be a singleton?

> +
> +	enumerator_ = DeviceEnumerator::create();
> +
> +	if (enumerator_->enumerate())
> +		return -ENODEV;
> +
> +	/*
> +	 * TODO: Try to read handlers and order from configuration
> +	 * file and only fallback on all handlers if there is no
> +	 * configuration file.
> +	 */
> +	PipelineHandlerFactory::handlers(handlers);

I haven't commented on the PipelineHanderFactory patch, but the
'handlers()' method, even if static, is a canonical getter. Passing
'handlers' by reference make it hard to notice this actually a output
parameters. Might you consider:
1) passing handlers as pointer to make it clear is a output parameter
2) have the handlers() method return a vector

> +
> +	for (std::string const &handler : handlers) {
> +		PipelineHandler *pipe;
> +
> +		/*
> +		 * Try each pipeline handler until it exhaust
> +		 * all pipelines it can provide.
> +		 */
> +		do {
> +			pipe = PipelineHandlerFactory::create(handler, enumerator_);
> +			if (pipe)
> +				pipes_.push_back(pipe);
> +		} while (pipe);
> +	}
> +
> +	/* TODO: register hot-plug callback here */
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Stop the camera manager
> + *
> + * Before stopping the camera manger the caller is responsible for making
> + * sure all cameras provided by the manager are returned to the manager.
> + *
> + * After the manager has been stopped no resource provided by the camera
> + * manager should be consider valid or functional even if they for one
> + * reason or another have yet to be deleted.
> + */
> +void CameraManager::stop()
> +{
> +	/* TODO: unregister hot-plug callback here */
> +
> +	for (PipelineHandler *pipe : pipes_)
> +		delete pipe;
> +
> +	pipes_.clear();
> +
> +	if (enumerator_)
> +		delete enumerator_;
> +
> +	enumerator_ = nullptr;
> +}
> +
> +/**
> + * \brief List all detected cameras
> + *
> + * Before calling this function the caller is responsible to make sure
> + * the camera manger is running.
> + *
> + * \return List of names for all detected cameras
> + */
> +std::vector<std::string> CameraManager::list() const
> +{
> +	std::vector<std::string> list;
> +
> +	for (PipelineHandler *pipe : pipes_) {
> +		for (unsigned int i = 0; i < pipe->count(); i++) {
> +			Camera *cam = pipe->camera(i);
> +			list.push_back(cam->name());
> +		}
> +	}
> +
> +	return list;
> +}
> +
> +/**
> + * \brief Get a camera based on name
> + *
> + * \param[in] name Name of camera to get
> + *
> + * Before calling this function the caller is responsible to make sure

s/to make sure/of making sure/ ?
Again to be upscaled to more English language educated reviewers.

> + * the camera manger is running. A camera fetched this way should be
> + * release by the user with the put() method of the Camera object once

released

> + * it's done using the camera.
> + *
> + * \return Pointer to Camera object or nullptr if camera not found
> + */
> +Camera *CameraManager::get(const std::string &name)
> +{
> +	for (PipelineHandler *pipe : pipes_) {
> +		for (unsigned int i = 0; i < pipe->count(); i++) {
> +			Camera *cam = pipe->camera(i);
> +			if (cam->name() == name) {
> +				cam->get();
> +				return cam;
> +			}
> +		}
> +	}
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 0db648dd3e37156e..a8cb3fdc22784334 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_sources = files([
>      'camera.cpp',
> +    'camera_manager.cpp',
>      'device_enumerator.cpp',
>      'log.cpp',
>      'main.cpp',
> --
> 2.20.1

Again, minor stuff to be addressed later if you want to push this
series.

Thanks
   j

>
> _______________________________________________
> 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:46:12 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>   a few comments you might want to address before or after pushing
> this patches
> 
> On Sat, Dec 29, 2018 at 04:28:53AM +0100, Niklas Söderlund wrote:
> > Provide a CameraManager class which will handle listing, instancing,
> > destruction and lifetime management of cameras.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/camera_manager.h |  38 +++++++
> >  include/libcamera/libcamera.h      |   1 +
> >  include/libcamera/meson.build      |   1 +
> >  src/libcamera/camera_manager.cpp   | 166 +++++++++++++++++++++++++++++
> >  src/libcamera/meson.build          |   1 +
> >  5 files changed, 207 insertions(+)
> >  create mode 100644 include/libcamera/camera_manager.h
> >  create mode 100644 src/libcamera/camera_manager.cpp
> >
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > new file mode 100644
> > index 0000000000000000..9d032d71c4c0841a
> > --- /dev/null
> > +++ b/include/libcamera/camera_manager.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * camera_manager.h - Camera management
> > + */
> > +#ifndef __LIBCAMERA_CAMERA_MANAGER_H__
> > +#define __LIBCAMERA_CAMERA_MANAGER_H__
> > +
> > +#include <vector>
> > +#include <string>
> > +
> > +namespace libcamera {
> > +
> > +class Camera;
> > +class DeviceEnumerator;
> > +class PipelineHandler;
> > +
> > +class CameraManager
> > +{
> > +public:
> > +	CameraManager();
> > +
> > +	int start();
> > +	void stop();
> > +
> > +	std::vector<std::string> list() const;
> > +	Camera *get(const std::string &name);
> > +	void put(Camera *camera);
> > +
> > +private:
> > +	DeviceEnumerator *enumerator_;
> > +	std::vector<PipelineHandler *> pipes_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CAMERA_MANAGER_H__ */
> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> > index 44c094d92feed5ba..32fb1ff741a7b97a 100644
> > --- a/include/libcamera/libcamera.h
> > +++ b/include/libcamera/libcamera.h
> > @@ -8,6 +8,7 @@
> >  #define __LIBCAMERA_LIBCAMERA_H__
> >
> >  #include <libcamera/camera.h>
> > +#include <libcamera/camera_manager.h>
> >
> >  namespace libcamera {
> >
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 9b266ad926681db9..3e04557d66b1a8f4 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -1,5 +1,6 @@
> >  libcamera_api = files([
> >      'camera.h',
> > +    'camera_manager.h',
> >      'libcamera.h',
> >  ])
> >
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > new file mode 100644
> > index 0000000000000000..1160381b72a08850
> > --- /dev/null
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -0,0 +1,166 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * camera_manager.h - Camera management
> > + */
> > +
> > +#include <libcamera/camera_manager.h>
> > +
> > +#include "device_enumerator.h"
> > +#include "pipeline_handler.h"
> > +
> > +/**
> > + * \file camera_manager.h
> > + * \brief Manage all cameras handled by libcamera
> > + *
> > + * The responsibility of the camera manager is to control the lifetime
> > + * management of objects provided by libcamera.
> > + *
> > + * When a user wish to interact with libcamera it creates and starts a
> > + * CameraManager object. Once confirmed the camera manager is running
> > + * the application can list all cameras detected by the library, get
> > + * one or more of the cameras and interact with them.
> > + *
> > + * When the user is done with the camera it should be returned to the
> > + * camera manager. Once all cameras are returned to the camera manager
> > + * the user is free to stop the manager.
> > + *
> > + * \todo Add ability to add and remove media devices based on
> > + *       hot-(un)plug events coming from the device enumerator.
> > + *
> > + * \todo Add interface to register a notification callback to the user
> > + *       to be able to inform it new cameras have been hot-plugged or
> > + *       cameras have been removed due to hot-unplug.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +CameraManager::CameraManager()
> > +	: enumerator_(nullptr)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Start the camera manager
> > + *
> > + * Start the camera manager and enumerate all devices in the system. Once
> > + * the start have been confirmed the user is free to list and otherwise
> > + * interact with cameras in the system until either the camera manager
> > + * is stopped or the camera is unplugged from the system.
> > + *
> > + * \return true on successful start false otherwise
> > + */
> > +int CameraManager::start()
> > +{
> > +	std::vector<std::string> handlers;
> > +
> > +	if (enumerator_)
> > +		return -ENODEV;
> 
> EBUSY?
> Do we want the enumerator be a singleton?

Yes the CameraManager should be a singleton.

> 
> > +
> > +	enumerator_ = DeviceEnumerator::create();
> > +
> > +	if (enumerator_->enumerate())
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * TODO: Try to read handlers and order from configuration
> > +	 * file and only fallback on all handlers if there is no
> > +	 * configuration file.
> > +	 */
> > +	PipelineHandlerFactory::handlers(handlers);
> 
> I haven't commented on the PipelineHanderFactory patch, but the
> 'handlers()' method, even if static, is a canonical getter. Passing
> 'handlers' by reference make it hard to notice this actually a output
> parameters. Might you consider:

It is listed as a out parameter in the documentation.

> 1) passing handlers as pointer to make it clear is a output parameter

Would this not be just as unclear as a reference? By using a pointer 
here all we gain is that we need to add a nullptr check in 
PipelineHandlerFactory::handlers.

> 2) have the handlers() method return a vector

Sure it could be done but that would need to be a return by copy. What 
do rest of you think? I will leave it as is for now.

> 
> > +
> > +	for (std::string const &handler : handlers) {
> > +		PipelineHandler *pipe;
> > +
> > +		/*
> > +		 * Try each pipeline handler until it exhaust
> > +		 * all pipelines it can provide.
> > +		 */
> > +		do {
> > +			pipe = PipelineHandlerFactory::create(handler, enumerator_);
> > +			if (pipe)
> > +				pipes_.push_back(pipe);
> > +		} while (pipe);
> > +	}
> > +
> > +	/* TODO: register hot-plug callback here */
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \brief Stop the camera manager
> > + *
> > + * Before stopping the camera manger the caller is responsible for making
> > + * sure all cameras provided by the manager are returned to the manager.
> > + *
> > + * After the manager has been stopped no resource provided by the camera
> > + * manager should be consider valid or functional even if they for one
> > + * reason or another have yet to be deleted.
> > + */
> > +void CameraManager::stop()
> > +{
> > +	/* TODO: unregister hot-plug callback here */
> > +
> > +	for (PipelineHandler *pipe : pipes_)
> > +		delete pipe;
> > +
> > +	pipes_.clear();
> > +
> > +	if (enumerator_)
> > +		delete enumerator_;
> > +
> > +	enumerator_ = nullptr;
> > +}
> > +
> > +/**
> > + * \brief List all detected cameras
> > + *
> > + * Before calling this function the caller is responsible to make sure
> > + * the camera manger is running.
> > + *
> > + * \return List of names for all detected cameras
> > + */
> > +std::vector<std::string> CameraManager::list() const
> > +{
> > +	std::vector<std::string> list;
> > +
> > +	for (PipelineHandler *pipe : pipes_) {
> > +		for (unsigned int i = 0; i < pipe->count(); i++) {
> > +			Camera *cam = pipe->camera(i);
> > +			list.push_back(cam->name());
> > +		}
> > +	}
> > +
> > +	return list;
> > +}
> > +
> > +/**
> > + * \brief Get a camera based on name
> > + *
> > + * \param[in] name Name of camera to get
> > + *
> > + * Before calling this function the caller is responsible to make sure
> 
> s/to make sure/of making sure/ ?
> Again to be upscaled to more English language educated reviewers.
> 
> > + * the camera manger is running. A camera fetched this way should be
> > + * release by the user with the put() method of the Camera object once
> 
> released
> 

Thanks.

> > + * it's done using the camera.
> > + *
> > + * \return Pointer to Camera object or nullptr if camera not found
> > + */
> > +Camera *CameraManager::get(const std::string &name)
> > +{
> > +	for (PipelineHandler *pipe : pipes_) {
> > +		for (unsigned int i = 0; i < pipe->count(); i++) {
> > +			Camera *cam = pipe->camera(i);
> > +			if (cam->name() == name) {
> > +				cam->get();
> > +				return cam;
> > +			}
> > +		}
> > +	}
> > +
> > +	return nullptr;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 0db648dd3e37156e..a8cb3fdc22784334 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -1,5 +1,6 @@
> >  libcamera_sources = files([
> >      'camera.cpp',
> > +    'camera_manager.cpp',
> >      'device_enumerator.cpp',
> >      'log.cpp',
> >      'main.cpp',
> > --
> > 2.20.1
> 
> Again, minor stuff to be addressed later if you want to push this
> series.

With the above addressed can i add your review tag?

> 
> Thanks
>    j
> 
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Sun, Dec 30, 2018 at 09:31:09PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>

[snip]

> On 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:
> > > +	/*
> > > +	 * TODO: Try to read handlers and order from configuration
> > > +	 * file and only fallback on all handlers if there is no
> > > +	 * configuration file.
> > > +	 */
> > > +	PipelineHandlerFactory::handlers(handlers);
> >
> > I haven't commented on the PipelineHanderFactory patch, but the
> > 'handlers()' method, even if static, is a canonical getter. Passing
> > 'handlers' by reference make it hard to notice this actually a output
> > parameters. Might you consider:
>
> It is listed as a out parameter in the documentation.
>
> > 1) passing handlers as pointer to make it clear is a output parameter
>
> Would this not be just as unclear as a reference? By using a pointer
> here all we gain is that we need to add a nullptr check in
> PipelineHandlerFactory::handlers.
>

I tend to agree with what is written here:
https://google.github.io/styleguide/cppguide.html#Output_Parameters

References are good as const input arguments, but for output
parameters it's clearer to keep them pointers, to express the
intent to modify their content. To me a function that returns void
and a (&handler) parameter clearly states it is going to modify it.
The first time I read handlers(handlers) I had to go back and look at
documentation.

> > 2) have the handlers() method return a vector
>
> Sure it could be done but that would need to be a return by copy. What
> do rest of you think? I will leave it as is for now.
>
> >

That would read even more natural to me:
        handlers = PipelineHandlerFactory::handlers();

But to avoid copying the vector back I would go for the argument
passed by pointer option.

Or, could the PipelineHandlerFactory store a static vector, and return
a reference to it? I also feel is more 'natural' to return references
to statically allocated variables, such as member data, and return
pointers to instances/variables dynamically allocated. I have not
found that coded in any style guide, so that might just be my
preference only.
> > > +

[snip]

> >
> > Again, minor stuff to be addressed later if you want to push this
> > series.
>
> With the above addressed can i add your review tag?
>

For this and other patches where I forgot to do so:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

Hello,

On Sunday, 30 December 2018 23:17:15 EET Jacopo Mondi wrote:
> On Sun, Dec 30, 2018 at 09:31:09PM +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> > 
> > Thanks for your feedback.
> 
> [snip]
> 
> > On 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:
> > > > +	/*
> > > > +	 * TODO: Try to read handlers and order from configuration
> > > > +	 * file and only fallback on all handlers if there is no
> > > > +	 * configuration file.
> > > > +	 */
> > > > +	PipelineHandlerFactory::handlers(handlers);
> > > 
> > > I haven't commented on the PipelineHanderFactory patch, but the
> > > 'handlers()' method, even if static, is a canonical getter. Passing
> > > 'handlers' by reference make it hard to notice this actually a output
> > > parameters. Might you consider:
> > 
> > It is listed as a out parameter in the documentation.
> > 
> > > 1) passing handlers as pointer to make it clear is a output parameter
> > 
> > Would this not be just as unclear as a reference? By using a pointer
> > here all we gain is that we need to add a nullptr check in
> > PipelineHandlerFactory::handlers.
> 
> I tend to agree with what is written here:
> https://google.github.io/styleguide/cppguide.html#Output_Parameters
> 
> References are good as const input arguments, but for output
> parameters it's clearer to keep them pointers, to express the
> intent to modify their content. To me a function that returns void
> and a (&handler) parameter clearly states it is going to modify it.
> The first time I read handlers(handlers) I had to go back and look at
> documentation.
> 
> > > 2) have the handlers() method return a vector
> > 
> > Sure it could be done but that would need to be a return by copy. What
> > do rest of you think? I will leave it as is for now.
> 
> That would read even more natural to me:
>         handlers = PipelineHandlerFactory::handlers();
> 
> But to avoid copying the vector back I would go for the argument
> passed by pointer option.

	std::vector<std::string> handlers = PipelineHandlerFactory::handlers();

should be optimized by the compiler to avoid copies. The std::vector<> 
declared on the stack inside the handlers() function should be replaced with 
the one from the caller transparently.

https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization

> Or, could the PipelineHandlerFactory store a static vector, and return
> a reference to it? I also feel is more 'natural' to return references
> to statically allocated variables, such as member data, and return
> pointers to instances/variables dynamically allocated. I have not
> found that coded in any style guide, so that might just be my
> preference only.
> 
> > > > +
> 
> [snip]
> 
> > > Again, minor stuff to be addressed later if you want to push this
> > > series.
> > 
> > With the above addressed can i add your review tag?
> 
> For this and other patches where I forgot to do so:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Hi Niklas,

Thank you for the patch.

On Saturday, 29 December 2018 05:28:53 EET Niklas Söderlund wrote:
> Provide a CameraManager class which will handle listing, instancing,
> destruction and lifetime management of cameras.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera_manager.h |  38 +++++++
>  include/libcamera/libcamera.h      |   1 +
>  include/libcamera/meson.build      |   1 +
>  src/libcamera/camera_manager.cpp   | 166 +++++++++++++++++++++++++++++
>  src/libcamera/meson.build          |   1 +
>  5 files changed, 207 insertions(+)
>  create mode 100644 include/libcamera/camera_manager.h
>  create mode 100644 src/libcamera/camera_manager.cpp
> 
> diff --git a/include/libcamera/camera_manager.h
> b/include/libcamera/camera_manager.h new file mode 100644
> index 0000000000000000..9d032d71c4c0841a
> --- /dev/null
> +++ b/include/libcamera/camera_manager.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * camera_manager.h - Camera management
> + */
> +#ifndef __LIBCAMERA_CAMERA_MANAGER_H__
> +#define __LIBCAMERA_CAMERA_MANAGER_H__
> +
> +#include <vector>
> +#include <string>
> +
> +namespace libcamera {
> +
> +class Camera;
> +class DeviceEnumerator;
> +class PipelineHandler;
> +
> +class CameraManager
> +{
> +public:
> +	CameraManager();
> +
> +	int start();
> +	void stop();
> +
> +	std::vector<std::string> list() const;
> +	Camera *get(const std::string &name);
> +	void put(Camera *camera);
> +
> +private:
> +	DeviceEnumerator *enumerator_;
> +	std::vector<PipelineHandler *> pipes_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CAMERA_MANAGER_H__ */
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index 44c094d92feed5ba..32fb1ff741a7b97a 100644
> --- a/include/libcamera/libcamera.h
> +++ b/include/libcamera/libcamera.h
> @@ -8,6 +8,7 @@
>  #define __LIBCAMERA_LIBCAMERA_H__
> 
>  #include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> 
>  namespace libcamera {
> 
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 9b266ad926681db9..3e04557d66b1a8f4 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_api = files([
>      'camera.h',
> +    'camera_manager.h',
>      'libcamera.h',
>  ])
> 
> diff --git a/src/libcamera/camera_manager.cpp
> b/src/libcamera/camera_manager.cpp new file mode 100644
> index 0000000000000000..1160381b72a08850
> --- /dev/null
> +++ b/src/libcamera/camera_manager.cpp
> @@ -0,0 +1,166 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * camera_manager.h - Camera management
> + */
> +
> +#include <libcamera/camera_manager.h>
> +
> +#include "device_enumerator.h"
> +#include "pipeline_handler.h"
> +
> +/**
> + * \file camera_manager.h
> + * \brief Manage all cameras handled by libcamera
> + *
> + * The responsibility of the camera manager is to control the lifetime
> + * management of objects provided by libcamera.
> + *
> + * When a user wish to interact with libcamera it creates and starts a
> + * CameraManager object. Once confirmed the camera manager is running
> + * the application can list all cameras detected by the library, get
> + * one or more of the cameras and interact with them.
> + *
> + * When the user is done with the camera it should be returned to the
> + * camera manager. Once all cameras are returned to the camera manager
> + * the user is free to stop the manager.
> + *
> + * \todo Add ability to add and remove media devices based on
> + *       hot-(un)plug events coming from the device enumerator.
> + *
> + * \todo Add interface to register a notification callback to the user
> + *       to be able to inform it new cameras have been hot-plugged or
> + *       cameras have been removed due to hot-unplug.
> + */
> +
> +namespace libcamera {
> +
> +CameraManager::CameraManager()
> +	: enumerator_(nullptr)
> +{
> +}
> +
> +/**
> + * \brief Start the camera manager
> + *
> + * Start the camera manager and enumerate all devices in the system. Once
> + * the start have been confirmed the user is free to list and otherwise

s/have/has/

> + * interact with cameras in the system until either the camera manager
> + * is stopped or the camera is unplugged from the system.
> + *
> + * \return true on successful start false otherwise
> + */
> +int CameraManager::start()
> +{
> +	std::vector<std::string> handlers;
> +
> +	if (enumerator_)
> +		return -ENODEV;
> +
> +	enumerator_ = DeviceEnumerator::create();
> +

I'd remove the blank line.

> +	if (enumerator_->enumerate())
> +		return -ENODEV;
> +
> +	/*
> +	 * TODO: Try to read handlers and order from configuration
> +	 * file and only fallback on all handlers if there is no
> +	 * configuration file.
> +	 */
> +	PipelineHandlerFactory::handlers(handlers);
> +
> +	for (std::string const &handler : handlers) {
> +		PipelineHandler *pipe;
> +
> +		/*
> +		 * Try each pipeline handler until it exhaust
> +		 * all pipelines it can provide.
> +		 */
> +		do {
> +			pipe = PipelineHandlerFactory::create(handler, enumerator_);
> +			if (pipe)
> +				pipes_.push_back(pipe);
> +		} while (pipe);
> +	}
> +
> +	/* TODO: register hot-plug callback here */
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Stop the camera manager
> + *
> + * Before stopping the camera manger the caller is responsible for making
> + * sure all cameras provided by the manager are returned to the manager.
> + *
> + * After the manager has been stopped no resource provided by the camera
> + * manager should be consider valid or functional even if they for one
> + * reason or another have yet to be deleted.
> + */
> +void CameraManager::stop()
> +{
> +	/* TODO: unregister hot-plug callback here */
> +
> +	for (PipelineHandler *pipe : pipes_)
> +		delete pipe;
> +
> +	pipes_.clear();
> +
> +	if (enumerator_)
> +		delete enumerator_;
> +
> +	enumerator_ = nullptr;
> +}
> +
> +/**
> + * \brief List all detected cameras
> + *
> + * Before calling this function the caller is responsible to make sure
> + * the camera manger is running.
> + *
> + * \return List of names for all detected cameras
> + */
> +std::vector<std::string> CameraManager::list() const
> +{
> +	std::vector<std::string> list;
> +
> +	for (PipelineHandler *pipe : pipes_) {
> +		for (unsigned int i = 0; i < pipe->count(); i++) {
> +			Camera *cam = pipe->camera(i);

I still think accessing cameras from the pipeline by index isn't a good API, 
but it can be changed later.

> +			list.push_back(cam->name());
> +		}
> +	}
> +
> +	return list;
> +}
> +
> +/**
> + * \brief Get a camera based on name
> + *
> + * \param[in] name Name of camera to get
> + *
> + * Before calling this function the caller is responsible to make sure

s/to make sure/for ensuring that/

> + * the camera manger is running. A camera fetched this way should be

s/should/shall/

> + * release by the user with the put() method of the Camera object once
> + * it's done using the camera.
> + *
> + * \return Pointer to Camera object or nullptr if camera not found
> + */
> +Camera *CameraManager::get(const std::string &name)
> +{
> +	for (PipelineHandler *pipe : pipes_) {
> +		for (unsigned int i = 0; i < pipe->count(); i++) {
> +			Camera *cam = pipe->camera(i);
> +			if (cam->name() == name) {
> +				cam->get();
> +				return cam;
> +			}
> +		}
> +	}
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 0db648dd3e37156e..a8cb3fdc22784334 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_sources = files([
>      'camera.cpp',
> +    'camera_manager.cpp',
>      'device_enumerator.cpp',
>      'log.cpp',
>      'main.cpp',

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

Hi Jacopo, Laurent,

On 2018-12-31 00:39:12 +0200, Laurent Pinchart wrote:
> Hello,
> 
> On Sunday, 30 December 2018 23:17:15 EET Jacopo Mondi wrote:
> > On Sun, Dec 30, 2018 at 09:31:09PM +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > > 
> > > Thanks for your feedback.
> > 
> > [snip]
> > 
> > > On 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:
> > > > > +	/*
> > > > > +	 * TODO: Try to read handlers and order from configuration
> > > > > +	 * file and only fallback on all handlers if there is no
> > > > > +	 * configuration file.
> > > > > +	 */
> > > > > +	PipelineHandlerFactory::handlers(handlers);
> > > > 
> > > > I haven't commented on the PipelineHanderFactory patch, but the
> > > > 'handlers()' method, even if static, is a canonical getter. Passing
> > > > 'handlers' by reference make it hard to notice this actually a output
> > > > parameters. Might you consider:
> > > 
> > > It is listed as a out parameter in the documentation.
> > > 
> > > > 1) passing handlers as pointer to make it clear is a output parameter
> > > 
> > > Would this not be just as unclear as a reference? By using a pointer
> > > here all we gain is that we need to add a nullptr check in
> > > PipelineHandlerFactory::handlers.
> > 
> > I tend to agree with what is written here:
> > https://google.github.io/styleguide/cppguide.html#Output_Parameters
> > 
> > References are good as const input arguments, but for output
> > parameters it's clearer to keep them pointers, to express the
> > intent to modify their content. To me a function that returns void
> > and a (&handler) parameter clearly states it is going to modify it.
> > The first time I read handlers(handlers) I had to go back and look at
> > documentation.
> > 
> > > > 2) have the handlers() method return a vector
> > > 
> > > Sure it could be done but that would need to be a return by copy. What
> > > do rest of you think? I will leave it as is for now.
> > 
> > That would read even more natural to me:
> >         handlers = PipelineHandlerFactory::handlers();
> > 
> > But to avoid copying the vector back I would go for the argument
> > passed by pointer option.
> 
> 	std::vector<std::string> handlers = PipelineHandlerFactory::handlers();
> 
> should be optimized by the compiler to avoid copies. The std::vector<> 
> declared on the stack inside the handlers() function should be replaced with 
> the one from the caller transparently.
> 
> https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization

I yield :-)

> 
> > Or, could the PipelineHandlerFactory store a static vector, and return
> > a reference to it? I also feel is more 'natural' to return references
> > to statically allocated variables, such as member data, and return
> > pointers to instances/variables dynamically allocated. I have not
> > found that coded in any style guide, so that might just be my
> > preference only.
> > 
> > > > > +
> > 
> > [snip]
> > 
> > > > Again, minor stuff to be addressed later if you want to push this
> > > > series.
> > > 
> > > With the above addressed can i add your review tag?
> > 
> > For this and other patches where I forgot to do so:
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks Jacopo.

Hi Laurent,

Thanks for your feedback.

On 2018-12-31 01:04:08 +0200, Laurent Pinchart wrote:

[snip]

> > +/**
> > + * \brief List all detected cameras
> > + *
> > + * Before calling this function the caller is responsible to make sure
> > + * the camera manger is running.
> > + *
> > + * \return List of names for all detected cameras
> > + */
> > +std::vector<std::string> CameraManager::list() const
> > +{
> > +	std::vector<std::string> list;
> > +
> > +	for (PipelineHandler *pipe : pipes_) {
> > +		for (unsigned int i = 0; i < pipe->count(); i++) {
> > +			Camera *cam = pipe->camera(i);
> 
> I still think accessing cameras from the pipeline by index isn't a good API, 
> but it can be changed later.

I agree. As the interface between a Camera and the MediaDevice and/or 
pipeline manager is not yet defined I thought I keep this dead simple 
for now as to not create a CameraManager requirement that would take a 
lot of time to change.

As previously discussed I agree that the best solution would probably be 
for the Camera to register itself with the CameraManager. Lets see how 
it plays out.