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

2447 diff mbox series

Expose a method to retrieve Cameras by devnum. The map of devnum to
Camera is gathered from all PipelineHandlers. This method is mainly for
the V4L2 compatibility layer, in order to match V4L2 devices to
libcamera Cameras.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
New in v3
---
 include/libcamera/camera_manager.h |  3 +++
 src/libcamera/camera_manager.cpp   | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

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@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

Thank you for the patch.

On Fri, Dec 27, 2019 at 01:58:05PM +0100, Jacopo Mondi wrote:
> 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 :)

As noted in my review of 2/6, I agree :-) It's not the end of the world
in commit messages, but interacts badly with Doxygen in the code, so we
can as well extend the good habit to commit messages.

> > Signed-off-by: Paul Elder <paul.elder@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());
> > +	}
> > +

As commented for 2/6, I think we should insert the camera in
camerasByDevnum_ when the camera is registered, bypassing the map in
PipelineHandler. This patch will then become quite small, and I think
you should squash it with 2/6.

> >  	/* 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.

You need to expand the documentation here to explain what retrieving a
camera by devnum is meant for and what it means, as commented in patch
2/6.

> > + *
> > + * \return Shared pointer to Camera object or nullptr if camera not found

Technically speaking, it's not a nullptr but an empty shared pointer.
I'd write "Shared pointer to Camera object, which is empty if the camera
is 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@jmondi.org>
> 
> >  /**
> >   * \brief Add a camera to the camera manager
> >   * \param[in] camera The camera to be added