[libcamera-devel] [PATCH v4 4/5] libcamera: pipeline: uvcvideo: Generate unique camera names

[Laurent Pinchart]

Hi Nicolas,

On Mon, Aug 10, 2020 at 12:07:54PM -0400, Nicolas Dufresne wrote:
> Le mercredi 29 juillet 2020 à 13:39 +0200, Niklas Söderlund a écrit :
> > On 2020-07-29 11:54:40 +0100, Kieran Bingham wrote:
> > > On 29/07/2020 10:21, Niklas Söderlund wrote:
> > > > Generate camera names that are unique and persistent between system
> > > > resets. The name is constructed from the USB vendor and product
> > > > information that is stored on USB device itself and the USB bus and
> > > > device numbers where the hardware is plugged in.
> > > > 
> > > > Before this change example of camera names:
> > > > 
> > > > Venus USB2.0 Camera: Venus USB2
> > > > Logitech Webcam C930e
> > > > 
> > > > After this change the same cameras are:
> > > > 
> > > > 0ac8:3420:3:10
> > > > 046d:0843:3:4
> > > 
> > > I don't see any update to cam or qcam in this series.
> > > 
> > > Is that really what we want to present to the users in this new-world-order?
> > > 
> > > Unique 'yes'
> > > Consistent 'yes'
> > > 
> > > Allows the 'user' to pick the right camera .... 'nope'.
> > 
> > As discussed on previous versions of this series and irc the name of 
> > cameras is not suitable to be presented to users expect for 
> > debugging/development tools. Examples of badly formatted UVC model names 
> > and serial numbers where brought up as examples on why they should not 
> > be part of the camera name.
> > 
> > The use-case for the name is for applications to be able to have a 
> > identifier that is unique and persistent.
> > 
> > To address your concern about a user-friendly string applications should 
> > construct one themself using informations and controls exposed by the 
> > Camera. The string could contain information about where in the system a 
> > camera is located and the string might need to be localized and other 
> > such concerns.
> > 
> > I agree work is needed on cam and qcam to support this. That is however 
> > a different problem this series do not address. I hope to have shot at 
> > working on that as soon as we have this work merged.
> 
> I believe the fact it's called name() makes that API dedicated to
> something human readable. I was expecting a new API called id() or
> identifier().

Fully agreed. v4 isn't the latest version of this series. The code that
has been merged calls this id().

> At least all other systems seems to make an effort to
> keep it human recognizable, perhaps you should have a look at
> pulseaudio unique name scheme ?

It's a good idea. Any pointer ?

> I don't think it's great to wave away the ability to display something
> slightly human readable, even for debugging having just string of
> hexadecimal (UVC cases) will make things much harder.
> 
> I also dislike this waving away as it will make it looks like this
> basic information is optional, but it should not be.

Niklas has submitted another series aimed at solving the human-readable
name issue, in a properly designed way this time. The overall idea is to
expose all the information needed to construct a name, and let
applications construct it. I think we should also have a helper function
to provide a default constructed name.

> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > > * Changes since v3
> > > > - Switch argument to generateName() to UVCCameraData pointer.
> > > > ---
> > > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 35 +++++++++++++++++++-
> > > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > index 93e3dc17e3a7105e..f51529ea519f5aee 100644
> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > @@ -6,6 +6,7 @@
> > > >   */
> > > >  
> > > >  #include <algorithm>
> > > > +#include <fstream>
> > > >  #include <iomanip>
> > > >  #include <math.h>
> > > >  #include <tuple>
> > > > @@ -81,6 +82,8 @@ public:
> > > >  	bool match(DeviceEnumerator *enumerator) override;
> > > >  
> > > >  private:
> > > > +	std::string generateName(const UVCCameraData *data);
> > > > +
> > > >  	int processControl(ControlList *controls, unsigned int id,
> > > >  			   const ControlValue &value);
> > > >  	int processControls(UVCCameraData *data, Request *request);
> > > > @@ -379,6 +382,30 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +std::string PipelineHandlerUVC::generateName(const UVCCameraData *data)
> > > > +{
> > > > +
> > > > +	static const std::vector<std::string> files
> > > > +		= { "idVendor", "idProduct", "busnum", "devnum" };
> > > > +	std::string path = data->video_->devicePath();
> > > > +	std::vector<std::string> values;
> > > > +	std::string value;
> > > > +
> > > > +	for (const std::string &name : files) {
> > > > +		std::ifstream file(path + "/../" + name);
> > > > +
> > > > +		if (!file.is_open())
> > > > +			return "";
> > > > +
> > > > +		std::getline(file, value);
> > > > +		file.close();
> > > > +
> > > > +		values.push_back(value);
> > > > +	}
> > > > +
> > > > +	return utils::join(values, ":");
> > > > +}
> > > > +
> > > >  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > > >  {
> > > >  	MediaDevice *media;
> > > > @@ -405,8 +432,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > > >  		return false;
> > > >  
> > > >  	/* Create and register the camera. */
> > > > +	std::string name = generateName(data.get());
> > > > +	if (name.empty()) {
> > > > +		LOG(UVC, Error) << "Failed to generate camera name";
> > > > +		return false;
> > > > +	}
> > > > +
> > > >  	std::set<Stream *> streams{ &data->stream_ };
> > > > -	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> > > > +	std::shared_ptr<Camera> camera = Camera::create(this, name, streams);
> > > >  	registerCamera(std::move(camera), std::move(data));
> > > >  
> > > >  	/* Enable hot-unplug notifications. */

-- 
Regards,

Laurent Pinchart