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

[Laurent Pinchart]

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 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());
> > +	}
> > +

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 at jmondi.org>
> 
> >  /**
> >   * \brief Add a camera to the camera manager
> >   * \param[in] camera The camera to be added

-- 
Regards,

Laurent Pinchart