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

[Nicolas Dufresne]

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 ?

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.

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