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

Message ID 20200729092122.3765539-5-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Generate unique and stable camera names
Related show

Commit Message

Niklas Söderlund July 29, 2020, 9:21 a.m. UTC
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

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@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(-)

Comments

Kieran Bingham July 29, 2020, 10:54 a.m. UTC | #1
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'.



> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@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. */
>
Niklas Söderlund July 29, 2020, 11:39 a.m. UTC | #2
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@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@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
Nicolas Dufresne Aug. 10, 2020, 4:07 p.m. UTC | #3
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@ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo@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
Laurent Pinchart Aug. 10, 2020, 4:25 p.m. UTC | #4
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@ragnatech.se>
> > > > Reviewed-by: Jacopo Mondi <jacopo@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. */
Niklas Söderlund Aug. 10, 2020, 4:36 p.m. UTC | #5
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@ragnatech.se>
> > > > Reviewed-by: Jacopo Mondi <jacopo@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
>

Patch

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