[libcamera-devel] [PATCH 6/7] cam: Print user-friendly camera names

[Laurent Pinchart]

Hello,

On Thu, Aug 06, 2020 at 05:06:34PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 06, 2020 at 03:46:01PM +0200, Niklas Söderlund wrote:
> > On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote:
> > > On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote:
> > > > On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote:
> > > > > On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote:
> > > > > > Instead of only printing the camera ID which is not intended for humans
> > > > > > to read and parse create a more user friendly string when prating camera
> > > > > > names. The ID is still printed as it is one option used to select camera
> > > > > > using the --camera option.
> > > > > >
> > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > > > ---
> > > > > >  src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++----
> > > > > >  1 file changed, 37 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > > > > index cc3facd5a5b22092..5af76f6965ef2387 100644
> > > > > > --- a/src/cam/main.cpp
> > > > > > +++ b/src/cam/main.cpp
> > > > > > @@ -21,6 +21,39 @@
> > > > > >
> > > > > >  using namespace libcamera;
> > > > > >
> > > > > > +std::string cameraName(const Camera *camera)
> > > > > > +{
> > > > > > +	const ControlList &props = camera->properties();
> > > > > > +	std::string name;
> > > > > > +
> > > > > > +	if (props.contains(properties::Model))
> > > > > > +		name += props.get(properties::Model) + " ";
> > > > > > +
> > > > > > +	if (props.contains(properties::Location)) {
> > > > > > +		switch (props.get(properties::Location)) {
> > > > > > +		case properties::CameraLocationFront:
> > > > > > +			name += "facing front ";
> > > > > > +			break;
> > > > > > +		case properties::CameraLocationBack:
> > > > > > +			name += "facing back ";
> > > > > > +			break;
> > > > > > +		case properties::CameraLocationExternal:
> > > > > > +			name += "external ";
> > > > > > +			break;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	if (props.contains(properties::Rotation))
> > > > > > +		name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees ";
> > > > > > +
> > > > > > +	if (!name.empty())
> > > > > > +		name += "with id ";
> > > > >
> > > > > Just a quick question while skimming through the series. cam can
> > > > > printout camera properties, do we need to make 'friendly' names
> > > > > 100 characters to repeat what's already available there ?
> > > >
> > > > I know it can print properties :-)
> > > >
> > > > I'm happy to change this to contain more or less information, my main
> > > > goal of throwing in everything here is to showcase with cam how
> > > > applications can create names.
> > > >
> > > > What properties would you like to see make up the user-friendly name?
> > >
> > > None if not useful to distinguish between cameras with the same name ?
> > >
> > > ie
> > >         1- ov5670 (front)
> > >         2- ov5670 (back)
> > >
> > > I know we could have
> > >         1- ov5670 (external)
> > >         2- ov5670 (external)
> >
> > I like this and will use it for next version.
> >
> > But I really think we need to print the id as well as it is one of two
> > possible augments to --camera to select which one is used. How about
> >
> >     1: ov5695 (front) - /base/i2c at ff160000/camera at 36
> >     2: ov2685 (back) - /base/i2c at ff160000/camera at 3c
> >
> > ?
> 
> I now wonder if an application like cam should expose machine-friendly
> id at all. But I guess it's fine, I like the above proposal

I'm happy to join the bikeshedding :-) Jokes aside, I think this is
important.

Ideally, for internal cameras, I'd like to see something along the lines
of

1: Internal front camera (/base/i2c at ff160000/camera at 36)
2. Internal back camera (/base/i2c at ff160000/camera at 3c)

reported by cam, or possibly

1: Internal front 5MP camera (/base/i2c at ff160000/camera at 36)
2. Internal back 12MP camera (/base/i2c at ff160000/camera at 3c)

if we want to make this more marketing-friendly :-) I know that
currently we'll get "Internal front camera" twice, and I think that's
OK. I believe the ID is important to print in cam, less so in qcam.

For external camera, I've been dreaming of

3: Logitech Webcam C930e on USB port 2.2 (\_SB_.PCI0.RP05.PXSX-2.2:1.0-046d:0843)
4: Logitech Webcam C930e on USB port 2.4 (\_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843)

but getting a sensible port number may be difficult (in theory we could
get the physical location from ACPI tables, but that's really the
theory). I think reporting extended physical location through properties
would be useful, but it doesn't have to be part of this series.

> > > but at that point, I think we've done the best we could
> > >
> > > > > > +
> > > > > > +	name += camera->id();
> > > > > > +
> > > > > > +	return name;
> > > > > > +}
> > > > > > +
> > > > > >  class CamApp
> > > > > >  {
> > > > > >  public:
> > > > > > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv)
> > > > > >  			return -EINVAL;
> > > > > >  		}
> > > > > >
> > > > > > -		std::cout << "Using camera " << camera_->id() << std::endl;
> > > > > > +		std::cout << "Using camera " << cameraName(camera_.get()) << std::endl;
> > > > > >
> > > > > >  		ret = prepareConfig();
> > > > > >  		if (ret) {
> > > > > > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration()
> > > > > >
> > > > > >  void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> > > > > >  {
> > > > > > -	std::cout << "Camera Added: " << cam->id() << std::endl;
> > > > > > +	std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl;
> > > > > >  }
> > > > > >
> > > > > >  void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> > > > > >  {
> > > > > > -	std::cout << "Camera Removed: " << cam->id() << std::endl;
> > > > > > +	std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl;
> > > > > >  }
> > > > > >
> > > > > >  int CamApp::run()
> > > > > > @@ -340,7 +373,7 @@ int CamApp::run()
> > > > > >
> > > > > >  		unsigned int index = 1;
> > > > > >  		for (const std::shared_ptr<Camera> &cam : cm_->cameras()) {
> > > > > > -			std::cout << index << ": " << cam->id() << std::endl;
> > > > > > +			std::cout << index << ": " << cameraName(cam.get()) << std::endl;
> > > > > >  			index++;
> > > > > >  		}
> > > > > >  	}

-- 
Regards,

Laurent Pinchart