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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-09-28 20:11:39 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Sep 25, 2020 at 05:07:42PM +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 printing
> > 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>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> > * Changes since v4
> > - Make cameraName() member ofr CamApp.
> > 
> > * Changes since v1
> > - Only print user-friendly names when listing cameras.
> > - Update format of user-friendly names printed.
> > - Update commit message.
> > ---
> >  src/cam/main.cpp | 31 ++++++++++++++++++++++++++++++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 244720b491f5c462..ed56d06c9d3386b9 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -45,6 +45,8 @@ private:
> >  	int infoConfiguration();
> >  	int run();
> >  
> > +	std::string cameraName(const Camera *camera);
> > +
> >  	static CamApp *app_;
> >  	OptionsParser::Options options_;
> >  	CameraManager *cm_;
> > @@ -340,7 +342,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++;
> >  		}
> >  	}
> > @@ -378,6 +380,33 @@ int CamApp::run()
> >  	return 0;
> >  }
> >  
> > +std::string CamApp::cameraName(const Camera *camera)
> > +{
> > +	const ControlList &props = camera->properties();
> > +	std::string name;
> > +
> > +	if (props.contains(properties::Location)) {
> 
> As Location is mandatory, I would consider this to be a bug in
> libcamera. Do we need to handle it explicitly ?

This is leftover from when it was not, I will drop this in next version.


> 
> > +		switch (props.get(properties::Location)) {
> > +		case properties::CameraLocationFront:
> > +			name = "Internal front camera";
> > +			break;
> > +		case properties::CameraLocationBack:
> > +			name = "Internal back camera";
> > +			break;
> > +		case properties::CameraLocationExternal:
> > +			name = "External camera";
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (props.contains(properties::Model))
> > +		name += " " + props.get(properties::Model);
> 
> For USB cameras, ideally I'd like to see something like
> 
> "External USB camera 'Logitech StreamCam' (on port 3)"

I see what you want to to, I find the sensor name the most use-full part 
:-) Mostly because I want a quick way to know which kernel driver to 
poke in to be able to fine someone else to blame when my stuff is not 
working, never works tho it's always my fault in the end ;-P

I have no strong opinion on this and was even considering dropping this 
patch for next version. You present something that can already partly be 
done so I will try one more time :-)

	const ControlList &props = camera->properties();
        std::string name;

        switch (props.get(properties::Location)) {
        case properties::CameraLocationFront:
                name = "Internal front camera";
                break;
        case properties::CameraLocationBack:
                name = "Internal back camera";
                break;
        case properties::CameraLocationExternal:
                name = "External camera";
                if (props.contains(properties::Model))
                        name += " '" + props.get(properties::Model) + "'";
                break;
        }

        name += " (" + camera->id() + ")";

	return name;

> 
> We don't have the infrastructure to know it's a USB camera yet, or to
> know the port number, so we can leave that out of now. For internal
> cameras, however, the sensor model should at best be printed under
> parentheses (or in any other way that makes it apparent it's a detail),
> if at all. As cam is more developer-oriented printing the camera sensor
> model is probably useful, but if we consider it as an example
> application, from an end-user point of view, the sensor model isn't very
> relevant.
> 
> > +
> > +	name += " (" + camera->id() + ")";
> > +
> > +	return name;
> > +}
> > +
> >  void signalHandler([[maybe_unused]] int signal)
> >  {
> >  	std::cout << "Exiting" << std::endl;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund