[libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager: add CameraManager class
Jacopo Mondi
jacopo at jmondi.org
Sun Dec 30 11:46:12 CET 2018
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181230/d10ab51e/attachment-0001.sig>
More information about the libcamera-devel
mailing list