[libcamera-devel,v2,1/3] cam: Do not assume Location is available
diff mbox series

Message ID 20210319130120.141563-2-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: Do not default the location property
Related show

Commit Message

Jacopo Mondi March 19, 2021, 1:01 p.m. UTC
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@jmondi.org>
---
 src/cam/main.cpp | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart March 20, 2021, 8:55 p.m. UTC | #1
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@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.
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:
+			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,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Jacopo Mondi March 21, 2021, 1:36 p.m. UTC | #2
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@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@ideasonboard.com>
>

Thanks
  j

> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart March 21, 2021, 1:47 p.m. UTC | #3
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@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@ideasonboard.com>
Jacopo Mondi March 21, 2021, 2:09 p.m. UTC | #4
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@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@ideasonboard.com>
>
> --
> Regards,
>
> Laurent Pinchart
Paul Elder March 22, 2021, 5:47 a.m. UTC | #5
Hi Jacopo,

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@jmondi.org>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  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() + ")";
> -- 
> 2.30.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series

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