[libcamera-devel] [PATCH 10/12] libcamera: cameramanager: add CameraManager class

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sunday, 23 December 2018 01:00:39 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 at ragnatech.se>
> ---
>  include/libcamera/cameramanager.h |  38 +++++++
>  include/libcamera/libcamera.h     |   1 +
>  include/libcamera/meson.build     |   1 +
>  src/libcamera/cameramanager.cpp   | 173 ++++++++++++++++++++++++++++++
>  src/libcamera/meson.build         |   1 +
>  5 files changed, 214 insertions(+)
>  create mode 100644 include/libcamera/cameramanager.h
>  create mode 100644 src/libcamera/cameramanager.cpp
> 
> diff --git a/include/libcamera/cameramanager.h
> b/include/libcamera/cameramanager.h new file mode 100644
> index 0000000000000000..be078a75f9e5aefc
> --- /dev/null
> +++ b/include/libcamera/cameramanager.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * cameramanager.h - Camera management
> + */
> +#ifndef __LIBCAMERA_CAMERAMANAGER_H__
> +#define __LIBCAMERA_CAMERAMANAGER_H__
> +
> +#include <vector>
> +#include <string>
> +
> +namespace libcamera {
> +
> +class Camera;
> +class DeviceEnumerator;
> +class PipelineHandler;
> +
> +class CameraManager
> +{
> +public:
> +	CameraManager();
> +
> +	bool 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_CAMERAMANAGER_H__ */
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index 44c094d92feed5ba..286b3d4d28083b01 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/cameramanager.h>
> 
>  namespace libcamera {
> 
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 9b266ad926681db9..b7ba1f124aada003 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_api = files([
>      'camera.h',
> +    'cameramanager.h',
>      'libcamera.h',
>  ])
> 
> diff --git a/src/libcamera/cameramanager.cpp
> b/src/libcamera/cameramanager.cpp new file mode 100644
> index 0000000000000000..53209e32d88e3ed3
> --- /dev/null
> +++ b/src/libcamera/cameramanager.cpp
> @@ -0,0 +1,173 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * cameramanager.h - Camera management
> + */
> +
> +#include <libcamera/cameramanager.h>
> +
> +#include "deviceenumerator.h"
> +#include "pipelinehandler.h"
> +
> +/**
> + * \file cameramanager.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_(NULL)
> +{
> +}
> +
> +/**
> + * \brief Start the camera manager
> + *
> + * Start the camera manager and enumerate all devices in the system. Once
> + * the stat have been confirmed the user is free to list and otherwise

s/stat/start/

> + * 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
> + */
> +bool CameraManager::start()

Should we standardize on integer status (0 for success and negative error code 
otherwise) ?

> +{
> +	std::vector<std::string> handlers;
> +
> +	if (enumerator_)
> +		return false;
> +
> +	enumerator_ = DeviceEnumerator::create();
> +
> +	if (enumerator_->enumerate())
> +		return false;
> +
> +	/* TODO: Try to read handlers and order from configuration

/* on its own line.

> +	 * file and only fallback on all handlers if there is no
> +	 * configuration file.
> +	 */
> +	PipelineHandler::handlers(handlers);
> +
> +	for (std::string const &handler : handlers) {

Instead of getting a list of pipeline handler names and trying to create 
pipelines by name with a lookup in PipelineHandler::create(), how about 
accessing the factories map directly ?

> +		PipelineHandler *pipe;
> +
> +		/* Try each pipeline handler until it exhaust
> +		 * all pipelines it can provide. */

Comment block format.

> +		do {
> +			pipe = PipelineHandler::create(handler, enumerator_);
> +			if (pipe)
> +				pipes_.push_back(pipe);
> +		} while (pipe);
> +	}
> +
> +	/* TODO: register hot-plug callback here */
> +
> +	return true;
> +}
> +
> +/**
> + * \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 have been stopped no resource provided my the camera

s/have/has/
s/my/by/

> + * 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_ = NULL;
> +}
> +
> +/**
> + * \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;

Would it make sense to construct the list at start() time and cache it ? 
Another option would be to make the PipelineManager register the Camera 
instances it creates with the CameraManager. I quite like the second option 
but I haven't thought it through yet.

> +}
> +
> +/**
> + * \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
> + * the camera manger is running. A camera fetched this way should be
> + * returned to the camera manger once the caller is done with it.
> + *
> + * \return Pointer to Camera object or NULL if camera not found
> + */
> +Camera *CameraManager::get(const std::string &name)

This requires camera names to be unique. While that's not a bad idea, I think 
this interface could be improved. Let's do so on top of this series.

> +{
> +	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 NULL;
> +}
> +
> +/**
> + * \brief Return camera to the camera manager
> + *
> + * \param[in] camera Camera to return to the manager
> + *
> + * After the camera have been returned to the camera manager the caller
> + * should not use the pointer to the camera object it has returned.
> + */
> +void CameraManager::put(Camera *camera)
> +{
> +	camera->put();
> +}

I don't think we need this method. The user should call camera->put() 
directly. This should be documented in CameraManager::get().

> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 0776643d3064129d..8457e57939b862ed 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_sources = files([
>      'camera.cpp',
> +    'cameramanager.cpp',
>      'deviceenumerator.cpp',
>      'log.cpp',
>      'main.cpp',

-- 
Regards,

Laurent Pinchart