[libcamera-devel,Simple-Cam:] simple-cam: Use friendly camera names
diff mbox series

Message ID 20201021154043.511274-1-kieran.bingham@ideasonboard.com
State Superseded
Delegated to: Kieran Bingham
Headers show
Series
  • [libcamera-devel,Simple-Cam:] simple-cam: Use friendly camera names
Related show

Commit Message

Kieran Bingham Oct. 21, 2020, 3:40 p.m. UTC
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@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(-)

Comments

Jacopo Mondi Oct. 21, 2020, 4:26 p.m. UTC | #1
Hi Kieran,

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

With the last question clarified
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> +		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;
>
>  	/*
>  	 * --------------------------------------------------------------------
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Oct. 21, 2020, 4:31 p.m. UTC | #2
On 21/10/2020 17:26, Jacopo Mondi wrote:
> Hi Kieran,
> 
> 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@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() ?

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


> With the last question clarified
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks.
k


> 
> Thanks
>   j
> 
> 
>> +		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;
>>
>>  	/*
>>  	 * --------------------------------------------------------------------
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 21, 2020, 6:52 p.m. UTC | #3
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@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@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;
> >>
> >>  	/*
> >>  	 * --------------------------------------------------------------------

Patch
diff mbox series

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)
+{
+	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;
+	}
+
+	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;
 
 	/*
 	 * --------------------------------------------------------------------