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

9835 diff mbox series

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@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham@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(-)

Hi Niklas,

On 9/25/20 8:37 PM, 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@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
LGTM.👍
Reviewed-by: Umang Jain <email@uajain.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)) {
> +		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);
> +
> +	name += " (" + camera->id() + ")";
> +
> +	return name;
> +}
> +
>   void signalHandler([[maybe_unused]] int signal)
>   {
>   	std::cout << "Exiting" << std::endl;

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@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham@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 ?

> +		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)"

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;

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@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham@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