[libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager: add CameraManager class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Dec 30 21:31:09 CET 2018
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 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?
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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list