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

[Laurent Pinchart]

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

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