[libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is available

[Jacopo Mondi]

Hi Laurent,

On Sun, Mar 21, 2021 at 03:47:26PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sun, Mar 21, 2021 at 02:36:22PM +0100, Jacopo Mondi wrote:
> > On Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote:
> > > On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote:
> > > > In preparation to register the Location property only if the firware
> > > > interface provides it, do not assume it is available and build the
> > > > camera name using the camera sensor model as a fallback.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/cam/main.cpp | 32 ++++++++++++++++++++------------
> > > >  1 file changed, 20 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > > index e01be63a7058..c087cdd97332 100644
> > > > --- a/src/cam/main.cpp
> > > > +++ b/src/cam/main.cpp
> > > > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const Camera *camera)
> > > >  	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;
> > > > +	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";
> > > > +			if (props.contains(properties::Model))
> > > > +				name += " '" + props.get(properties::Model) + "'";
> > > > +			break;
> > > > +		}
> > > > +	} else if (props.contains(properties::Model)) {
> > > > +		/*
> > > > +		 * If the camera location is not availble use the camera model
> > > > +		 * to build the camera name.
> > > > +		 */
> > > > +		name = "'" + props.get(properties::Model) + "'";
> > > >  	}
> > > >
> > > >  	name += " (" + camera->id() + ")";
> > >
> > > If none of the conditions above is true, there will be an extra space.
> >
> > Isn't the Model property always registered by the CameraSensor class ?
>
> Good question. I have assumed it isn't, as you call
> props.contains(Properties::Model) :-) I may have been wrong. Note that

And I call props.contains() as it was there already :)

> it's not just about the CameraSensor class, as not all pipeline handlers
> use it (most notably the UVC pipeline handler). What matters is if the
> property is documented as required or not.
>

The current implementation of the UVC pipeline handler registers
Model, but as it's not documented as mandatory I woudl stay safe and
continue checking for its presence.

I'll take your suggestion in and push!

Thanks
  j


> > > I initially thought utils::join() could be useful here, but that's not a
> > > public API :-S This could be an option, I'm sure there are other equally
> > > valid or better solutions.
> > >
> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > index e01be63a7058..3adfe2c34969 100644
> > > --- a/src/cam/main.cpp
> > > +++ b/src/cam/main.cpp
> > > @@ -377,23 +377,34 @@ int CamApp::run()
> > >  std::string const CamApp::cameraName(const Camera *camera)
> > >  {
> > >  	const ControlList &props = camera->properties();
> > > +	bool addModel = true;
> > >  	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;
> > > +	/*
> > > +	 * Construct the name from the camera location, model and ID. The model
> > > +	 * is only used if the location isn't present or is set to External.
> > > +	 */
> > > +
> > > +	if (props.contains(properties::Location)) {
> > > +		switch (props.get(properties::Location)) {
> > > +		case properties::CameraLocationFront:
> > > +			name = "Internal front camera ";
> > > +			addModel = false;
> > > +			break;
> > > +		case properties::CameraLocationBack:2
> > > +			name = "Internal back camera ";
> > > +			addModel = false;
> > > +			break;
> > > +		case properties::CameraLocationExternal:
> > > +			name = "External camera ";
> > > +			break;
> > > +		}
> > >  	}
> > >
> > > -	name += " (" + camera->id() + ")";
> > > +	if (addModel && props.contains(properties::Model))
> > > +		name += "'" + props.get(properties::Model) + "' ";
> > > +
> > > +	name += "(" + camera->id() + ")";
> > >
> > >  	return name;
> > >  }
> > >
> > > It's a detail, and regardless of what option you pick,
> >
> > Anyway, using an addModel flag would make the code more readable so
> > I'll refactor.
> >
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> --
> Regards,
>
> Laurent Pinchart