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

[Niklas Söderlund]

Hi Laurent,

Thanks for your comments.

On 2018-12-29 02:20:41 +0200, Laurent Pinchart wrote:
> 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/

Thanks.

> 
> > + * 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) ?

Lets, I felt bool was more C++ but would be happy to switch to the 
scheme you suggest here.

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

Done.

> 
> > +	 * 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 ?

I'm open to the idea but would like to implement the configuration file 
for 'active' pipeline handlers before we make the switch.

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

Done.

> 
> > +		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/

Thanks.

> 
> > + * 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();
> 

Good catch!

> > +
> > +	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.

I also like your second idea. I added it to the task list so we can fix 
this on-top.

> 
> > +}
> > +
> > +/**
> > + * \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.

I agree, this is one limitation of this series which should be solved 
going forward.

> 
> > +{
> > +	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().

I like it!

> 
> > +
> > +} /* 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
> 
> 
> 

-- 
Regards,
Niklas Söderlund