[libcamera-devel] [PATCH 3/4] libcamera: camera_manager: Register cameras with the camera manager

[Niklas Söderlund]

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