[libcamera-devel] [PATCH v3 4/6] libcamera: camera_manager: allow retrieving cameras by devnum

[Jacopo Mondi]

Hi Paul,

On Mon, Dec 23, 2019 at 01:26:18AM -0600, Paul Elder wrote:
> Expose a method to retrieve Cameras by devnum. The map of devnum to

Camera instances by device number ?

> Camera is gathered from all PipelineHandlers. This method is mainly for

ipeline handlers

> the V4L2 compatibility layer, in order to match V4L2 devices to
> libcamera Cameras.

Camera

Do not pluralize class names :)

>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> New in v3
> ---
>  include/libcamera/camera_manager.h |  3 +++
>  src/libcamera/camera_manager.cpp   | 24 ++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 8331898c..6ce63912 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
>  #define __LIBCAMERA_CAMERA_MANAGER_H__
>
> +#include <map>
>  #include <memory>
>  #include <string>
>  #include <vector>
> @@ -33,6 +34,7 @@ public:
>
>  	const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
>  	std::shared_ptr<Camera> get(const std::string &name);
> +	std::shared_ptr<Camera> get(dev_t devnum);
>
>  	void addCamera(std::shared_ptr<Camera> camera);
>  	void removeCamera(Camera *camera);
> @@ -46,6 +48,7 @@ private:
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>  	std::vector<std::shared_ptr<Camera>> cameras_;
> +	std::map<dev_t, std::shared_ptr<Camera>> camerasByDevnum_;

Do you need to store these as shared pointers ?
I don't think it's a big deal, but CameraManager already has a vector
of Camera shared_ptr, adding one reference (for some cameras only, the
ones created with a devnum associated) might not be needed. I would
store them as weak_ptr and return iter->second.lock() in the get()
implementation.

The implications are minor, so it's really up to you.

>
>  	static const std::string version_;
>  	static CameraManager *self_;
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 7c6f72bb..1354df4c 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -121,6 +121,11 @@ int CameraManager::start()
>  		}
>  	}
>
> +	for (std::shared_ptr<PipelineHandler> pipe : pipes_) {
> +		const std::map<dev_t, std::weak_ptr<Camera>> devMap = pipe->camerasByDevnum();

&devMap

> +		camerasByDevnum_.insert(devMap.begin(), devMap.end());
> +	}
> +
>  	/* TODO: register hot-plug callback here */
>
>  	return 0;
> @@ -180,6 +185,25 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &name)
>  	return nullptr;
>  }
>
> +/**
> + * \brief Get a camera based on devnum

"Retrieve a camera" ? Even if the other get() impmentation has this
very same documentation

> + * \param[in] devnum Device number of camera to get
> + *
> + * Before calling this function the caller is responsible for ensuring that
> + * the camera manager is running.
> + *
> + * \return Shared pointer to Camera object or nullptr if camera not found
> + */
> +std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> +{
> +	auto iter = camerasByDevnum_.find(devnum);
> +
> +	if (iter == camerasByDevnum_.end())
> +		return nullptr;
> +
> +	return iter->second;
> +}
> +

Minors apart:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

>  /**
>   * \brief Add a camera to the camera manager
>   * \param[in] camera The camera to be added
> --
> 2.23.0
>
> _______________________________________________
> 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/20191227/81783be9/attachment.sig>