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

[Niklas Söderlund]

Hi Nicolas,

Thanks for your feedback.

On 2020-08-10 12:07:54 -0400, Nicolas Dufresne wrote:
> Le mercredi 29 juillet 2020 à 13:39 +0200, Niklas Söderlund a écrit :
> > Hi Kieran,
> > 
> > Thanks for your feedback.
> > 
> > On 2020-07-29 11:54:40 +0100, Kieran Bingham wrote:
> > > Hi Niklas,
> > > 
> > > 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(). 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 ?

This is a old version of this series v8 is the one that got merged and 
the structure of the ID have changed a bit since this. Also the 
interface is changed to id() instead of name(), as you point out it 
makes more sens. The Camera::name() is however removed as its true 
intent was better described by the id().

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

Agreed, we discussed this in length during this work and came to the 
conclusion that different use-cases have different needs for the 
user-friendly name.  Some cases the location of the camera (front, back) 
makes more sens then the sensor model name for example. We also thought 
about the need for users of the API to be able to localize the 
user-friendly name.

The idea is to allow applications to build a user-friendly name which 
makes sens for it's use-case. For example we have a pending series that 
does this for cam already [1].

> 
> I also dislike this waving away as it will make it looks like this
> basic information is optional, but it should not be.

I understand your point. The idea has been for a long while to work on 
the old Camera::name() interface to grantee the name is unique and 
constant in the system and then use other properties to construct 
user-friendly names. Maybe we could have done it the other way around 
tho, first constructing new user-friendly names from properties and then 
made the Camera::name() -> Camera::id() change.

1. [PATCH 0/7] libcamera: Allow for user-friendly names in applications

> 
> > 
> > > 
> > > 
> > > > 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
> > > --
> > > Kieran
> 

-- 
Regards,
Niklas Söderlund