[libcamera-devel] [PATCH 3/4] libcamera: camera_manager: Register cameras with the camera manager
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Jan 18 17:06:04 CET 2019
Hi Laurent,
Thanks for your work.
On 2019-01-18 01:59:15 +0200, Laurent Pinchart wrote:
> Cameras are listed through a double indirection, first iterating over
> all available pipeline handlers, and then listing the cameras they each
> support. To simplify the API make the pipeline handlers register the
> cameras with the camera manager directly, which lets the camera manager
> easily expose the list of all available cameras.
>
> The PipelineHandler API gets simplified as the handlers don't need to
> expose the list of cameras they have created.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/camera_manager.h | 5 ++-
> src/libcamera/camera_manager.cpp | 44 +++++++++++-------------
> src/libcamera/include/pipeline_handler.h | 6 ++--
> src/libcamera/pipeline/vimc.cpp | 30 ++++------------
> src/libcamera/pipeline_handler.cpp | 21 +++--------
> test/list-cameras.cpp | 5 +--
> 6 files changed, 40 insertions(+), 71 deletions(-)
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index b82a8ce95b9f..4b941fd9183b 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -24,9 +24,11 @@ public:
> int start();
> void stop();
>
> - std::vector<std::string> list() const;
> + const std::vector<Camera *> &cameras() const { return cameras_; }
> Camera *get(const std::string &name);
>
> + void addCamera(Camera *camera);
> +
> static CameraManager *instance();
>
> void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
> @@ -40,6 +42,7 @@ private:
>
> std::unique_ptr<DeviceEnumerator> enumerator_;
> std::vector<PipelineHandler *> pipes_;
> + std::vector<Camera *> cameras_;
>
> std::unique_ptr<EventDispatcher> dispatcher_;
> };
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 9806e399f92b..852e5ed70fe3 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -93,7 +93,7 @@ int CameraManager::start()
> */
> while (1) {
> PipelineHandler *pipe = factory->create();
> - if (!pipe->match(enumerator_.get())) {
> + if (!pipe->match(this, enumerator_.get())) {
> delete pipe;
> break;
> }
> @@ -132,26 +132,14 @@ void CameraManager::stop()
> }
>
> /**
> - * \brief List all detected cameras
> + * \fn CameraManager::cameras()
> + * \brief Retrieve all available cameras
> *
> * Before calling this function the caller is responsible for ensuring that
> * the camera manger is running.
> *
> - * \return List of names for all detected cameras
> + * \return List of all available 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
> @@ -167,19 +155,27 @@ std::vector<std::string> CameraManager::list() const
> */
> 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;
> - }
> - }
> + for (Camera *camera : cameras_) {
> + if (camera->name() == name)
> + return camera;
> }
>
> return nullptr;
> }
>
> +/**
> + * \brief Add a camera to the camera manager
> + * \param[in] camera The camera to be added
> + *
> + * This function is called by pipeline handlers to register the cameras they
> + * handle with the camera manager. Registered cameras are immediately made
> + * available to the system.
> + */
> +void CameraManager::addCamera(Camera *camera)
> +{
> + cameras_.push_back(camera);
I wonder if we should not add a runtime check here to make sure the
camera name is unique before adding it to the vector here? The get()
function retrieves a Camera object by name there might be odd
consequences otherwise :-)
With this addressed,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> +}
> +
> /**
> * \brief Retrieve the camera manager instance
> *
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index e976aaa13546..f05f201f7ca8 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -15,6 +15,7 @@
>
> namespace libcamera {
>
> +class CameraManager;
> class DeviceEnumerator;
>
> class PipelineHandler
> @@ -22,10 +23,7 @@ class PipelineHandler
> public:
> virtual ~PipelineHandler() { };
>
> - virtual bool match(DeviceEnumerator *enumerator) = 0;
> -
> - virtual unsigned int count() = 0;
> - virtual Camera *camera(unsigned int id) = 0;
> + virtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0;
> };
>
> class PipelineHandlerFactory
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 720d9c2031c9..8742e0bae9a8 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -6,6 +6,7 @@
> */
>
> #include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
>
> #include "device_enumerator.h"
> #include "media_device.h"
> @@ -19,44 +20,24 @@ public:
> PipeHandlerVimc();
> ~PipeHandlerVimc();
>
> - bool match(DeviceEnumerator *enumerator);
> -
> - unsigned int count();
> - Camera *camera(unsigned int id) final;
> + bool match(CameraManager *manager, DeviceEnumerator *enumerator);
>
> private:
> MediaDevice *dev_;
> - Camera *camera_;
> };
>
> PipeHandlerVimc::PipeHandlerVimc()
> - : dev_(nullptr), camera_(nullptr)
> + : dev_(nullptr)
> {
> }
>
> PipeHandlerVimc::~PipeHandlerVimc()
> {
> - if (camera_)
> - camera_->put();
> -
> if (dev_)
> dev_->release();
> }
>
> -unsigned int PipeHandlerVimc::count()
> -{
> - return 1;
> -}
> -
> -Camera *PipeHandlerVimc::camera(unsigned int id)
> -{
> - if (id != 0)
> - return nullptr;
> -
> - return camera_;
> -}
> -
> -bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
> +bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator)
> {
> DeviceMatch dm("vimc");
>
> @@ -83,7 +64,8 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
> * will be chosen depends on how the Camera
> * object is modeled.
> */
> - camera_ = new Camera("Dummy VIMC Camera");
> + Camera *camera = new Camera("Dummy VIMC Camera");
> + manager->addCamera(camera);
>
> return true;
> }
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index c19ab94f1774..f2e08a6a7315 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -35,13 +35,15 @@ namespace libcamera {
> /**
> * \fn PipelineHandler::match(DeviceEnumerator *enumerator)
> * \brief Match media devices and create camera instances
> + * \param manager The camera manager
> * \param enumerator The enumerator providing all media devices found in the
> * system
> *
> * This function is the main entry point of the pipeline handler. It is called
> - * by the camera manager with the \a enumerator passed as an argument. It
> - * shall acquire from the \a enumerator all the media devices it needs for a
> - * single pipeline and create one or multiple Camera instances.
> + * by the camera manager with the \a manager and \a enumerator passed as
> + * arguments. It shall acquire from the \a enumerator all the media devices it
> + * needs for a single pipeline, create one or multiple Camera instances and
> + * register them with the \a manager.
> *
> * If all media devices needed by the pipeline handler are found, they must all
> * be acquired by a call to MediaDevice::acquire(). This function shall then
> @@ -62,19 +64,6 @@ namespace libcamera {
> * created, or false otherwise
> */
>
> -/**
> - * \fn PipelineHandler::count()
> - * \brief Retrieve the number of cameras handled by this pipeline handler
> - * \return the number of cameras that were created by the match() function
> - */
> -
> -/**
> - * \fn PipelineHandler::camera(unsigned int id)
> - * \brief Retrieve one of the cameras handled by this pipeline handler
> - * \param[in] id the camera index
> - * \return a pointer to the Camera identified by \a id
> - */
> -
> /**
> * \class PipelineHandlerFactory
> * \brief Registration of PipelineHandler classes and creation of instances
> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> index e2026c99c5b8..fdbbda0957b2 100644
> --- a/test/list-cameras.cpp
> +++ b/test/list-cameras.cpp
> @@ -7,6 +7,7 @@
>
> #include <iostream>
>
> +#include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
>
> #include "test.h"
> @@ -29,8 +30,8 @@ protected:
> {
> unsigned int count = 0;
>
> - for (auto name : cm->list()) {
> - cout << "- " << name << endl;
> + for (Camera *camera : cm->cameras()) {
> + cout << "- " << camera->name() << endl;
> count++;
> }
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> 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