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

[Jacopo Mondi]

Hi Laurent,

On Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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 ?

> 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>
>

Thanks
  j

> --
> Regards,
>
> Laurent Pinchart