[libcamera-devel] [Simple-Cam: PATCH] simple-cam: Use friendly camera names

[Kieran Bingham]

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 at 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 at 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran