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

[Niklas Söderlund]

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.

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