[libcamera-devel] [Simple-Cam: PATCH] simple-cam: Use friendly camera names

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Oct 21, 2020 at 05:31:18PM +0100, Kieran Bingham wrote:
> On 21/10/2020 17:26, Jacopo Mondi wrote:
> > On Wed, Oct 21, 2020 at 04:40:43PM +0100, Kieran Bingham wrote:
> >> Take the example code for generating a camera name from 'cam' and use
> >> it when reporting cameras within the simple-cam application.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>
> >> I wonder if this cameraName() might be something that
> >> should go into libcamera::utils as a public API helper?
> >>
> >> I know we want to allow applications to decide their own naming too, but
> >> I think we've discussed in the past about having a helper library on top
> >> to make things easier for application developers ?
> >>
> >>  simple-cam.cpp | 30 +++++++++++++++++++++++++++---
> >>  1 file changed, 27 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/simple-cam.cpp b/simple-cam.cpp
> >> index 727bb6d86480..1b6fd3939bf6 100644
> >> --- a/simple-cam.cpp
> >> +++ b/simple-cam.cpp
> >> @@ -60,6 +60,30 @@ static void requestComplete(Request *request)
> >>  	camera->queueRequest(request);
> >>  }
> >>
> >> +std::string cameraName(std::shared_ptr<libcamera::Camera> camera)
> > 
> > nit: camera can be a reference, or just a raw pointer. No need to
> 
> I started with a raw pointer, but got a compile error, indicating that
> the type available in :
> 
>   	for (auto const &camera : cm->cameras())
> 
> was a shared pointer. So that's what I put in and it compiled ;-)
> 
> I presume I could also do a camera.get() ?

Yes, I think that's better to avoid constructing/destroying a shared
pointer for little gain.

> > increase/decrease the usage count just for the scope of this function.
> > 
> >> +{
> >> +	const ControlList &props = camera->properties();
> >> +	std::string name;
> >> +
> >> +	switch (props.get(properties::Location)) {
> >> +	case properties::CameraLocationFront:
> >> +		name = "Internal front camera";
> >> +		break;
> > 
> > I wonder if defaulting to "Front" in CameraSensor is still a good idea
> > or we should let applications deals with that case...
> > 
> >> +	case properties::CameraLocationBack:
> >> +		name = "Internal back camera";
> >> +		break;
> >> +	case properties::CameraLocationExternal:
> >> +		name = "External camera";
> >> +		if (props.contains(properties::Model))
> >> +			name += " '" + props.get(properties::Model) + "'";
> > 
> > Why model is only considered for external cameras ?
> 
> The code there is taken verbatim from src/cam/
> 
> IMO, any change here should have a corresponding change there - or ...
> all of this should go in to some helper library (libcamera::utils?) so
> it can be handled as a common use case.

A libcamera utility library is needed (and we'll have to discuss whether
it should be a separate library, or part of the same binary). I'm
however not convinced this particular feature belongs there, as the idea
behind our camera naming scheme is to let applications construct the
names in the way that best fits their use cases, including localizing
them if needed. We have code duplication between cam and simplecam
because they're both demo applications, but it doesn't meant it would be
a candidate for libcamera itself.

Additionally, I think the application guide should be updated to explain
how applications are supposed to construct a camera name.

> > With the last question clarified
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > 
> >> +		break;
> >> +	}
> >> +
> >> +	name += " (" + camera->id() + ")";
> >> +
> >> +	return name;
> >> +}
> >> +
> >>  int main()
> >>  {
> >>  	/*
> >> @@ -77,11 +101,11 @@ int main()
> >>  	cm->start();
> >>
> >>  	/*
> >> -	 * Just as a test, list all id's of the Camera registered in the
> >> -	 * system. They are indexed by name by the CameraManager.
> >> +	 * Just as a test, generate names of the Cameras registered in the
> >> +	 * system, and list them.
> >>  	 */
> >>  	for (auto const &camera : cm->cameras())
> >> -		std::cout << camera->id() << std::endl;
> >> +		std::cout << " - " << cameraName(camera) << std::endl;
> >>
> >>  	/*
> >>  	 * --------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart