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

[Niklas Söderlund]

Hi Jacopo,

Thanks for your comments.

On 2020-07-29 10:14:59 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Jul 29, 2020 at 01:42:24AM +0200, 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
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 34 +++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 93e3dc17e3a7105e..72f3f5d9f7c414d0 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,9 @@ public:
> >  	bool match(DeviceEnumerator *enumerator) override;
> >
> >  private:
> > +	std::string generateName(const MediaDevice *media,
> > +				 const std::string path);
> > +
> >  	int processControl(ControlList *controls, unsigned int id,
> >  			   const ControlValue &value);
> >  	int processControls(UVCCameraData *data, Request *request);
> > @@ -379,6 +383,28 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> >  	return 0;
> >  }
> >
> > +std::string PipelineHandlerUVC::generateName(const MediaDevice *media,
> 
> Is media used ?
> 
> > +					     const std::string path)
> 
> I would have passed the CameraData instance and got the path from
> there.

I have no strong preference so I will make it so in v4.


> 
> > +{
> > +	static const std::vector<std::string> files = { "idVendor", "idProduct", "busnum", "devnum" };
> 
> less than 120 cols ?
> 
> > +	std::vector<std::string> values;
> > +	std::string value;
> > +
> > +	for (const std::string &name : files) {
> > +		std::ifstream file(path + "/../" + name);
> > +
> > +		if (!file.is_open())
> > +			return "";
> 
> Return or just skip that file ? If you want to return is this worth an
> error message ?

All files are mandatory so its all pass or we should fail.

> > +
> > +		std::getline(file, value);
> > +		file.close();
> 
> This applies to the other patches as well, reading
> std::ifstream::close documentation I see:
> 
> "Note that any open file is automatically closed when the ifstream object is destroyed."
> 
> Do you need to call close ? I got suspicious as it felt weird that an
> STL library opens a file on construction but requires manual closure..

I like to explicitly close the file, specially here as we loop I think 
it makes the code more readable.

> 
> > +
> > +		values.push_back(value);
> > +	}
> > +
> > +	return utils::join(values, ":");
> > +}
> > +
> >  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >  {
> >  	MediaDevice *media;
> > @@ -405,8 +431,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >  		return false;
> >
> >  	/* Create and register the camera. */
> > +	std::string name = generateName(media, data->video_->devicePath());
> > +	if (name.empty()) {
> > +		LOG(UVC, Error) << "Failed to generate camera name";
> > +		return false;
> > +	}
> 
> Oh the error is here, and is better here than in the function. But
> then you have to fail if you don't find one of the files... I think
> what you have here is right then...
> 
> > +
> >  	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);
> 
> nits apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks.

> 
> Thanks
>   j
> 
> >  	registerCamera(std::move(camera), std::move(data));
> >
> >  	/* Enable hot-unplug notifications. */
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund