[libcamera-devel] [PATCH 1/2] android: Override camera is "Internal" provided if found on HAL config

[Kieran Bingham]

On 27/07/2021 16:27, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote:
>> Currently, all UVC cameras are marked with CameraLocationExternal [1]
>> property, since there is no universal information or standard, to
>> know the location of these cameras. However, in the libcamera HAL layer,
>> we can make an informed decision whether its external or internal,
>> simply by checking the presence of it, in the HAL configuration file.
>>
>> If the camera is found to be present on the HAL configuration file,
>> treat it as internal and assign its numerical id accordingly.
>>
>> [1] 76809320bb1a ("libcamera: pipeline: uvcvideo: Treat all UVC cameras
>>                   as external")
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>  src/android/camera_device.cpp      | 10 +++++++++-
>>  src/android/camera_hal_manager.cpp | 14 ++++++++++++++
>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 678cde23..ba4e2d15 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
>>  			facing_ = CAMERA_FACING_BACK;
>>  			break;
>>  		case properties::CameraLocationExternal:
>> -			facing_ = CAMERA_FACING_EXTERNAL;
>> +			/*
>> +			 * If the camera is 'internal' as found by
>> +			 * CameraHalManager, use its location from
>> +			 * HAL config file.
>> +			 */
>> +			if (id_ < 1000 && cameraConfigData)

Is this to 'prevent' setting it to cameraConfigData->facing in the event
that id >= 1000?

Will that work? won't it get set regardless in the lines after this case
statement? :

>	if (cameraConfigData && cameraConfigData->facing != -1 &&
>	    facing_ != cameraConfigData->facing) {
>		LOG(HAL, Warning)
>			<< "Camera location does not match"
>			<< " configuration file. Using " << facing_;
>	}


Also - I see that the cameraConfigData->facing is never used without
first checking that it is not set to -1, which your lines do not check
for ... I don't know if this is a critical requirement or not.




>> +				facing_ = cameraConfigData->facing;
>> +			else
>> +				facing_ = CAMERA_FACING_EXTERNAL;
>>  			break;
>>  		}
>>  
>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
>> index 4cd67544..1a9b3413 100644
>> --- a/src/android/camera_hal_manager.cpp
>> +++ b/src/android/camera_hal_manager.cpp
>> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>>  		}
>>  	}
>>  
>> +	/*
>> +	 * In some cases, particularly in UVC cameras, the camera location is defaulted
>> +	 * to 'External'. However, if the HAL config file mentions the camera in question,
>> +	 * it shall mean that the camera is integrated to the system so, override to treat
>> +	 * it correctly as a 'internal' camera.
> 
> Could you please wrap this at 80 columns ?
> 
> I wouldn't say "defaulted", but instead talk about what the libcamera
> core does. Maybe something similar to the following.
> 
> 	/*
> 	 * Some cameras whose location is reported by libcamera as external may
> 	 * actually be internal to the device. This is common with UVC cameras
> 	 * that can be integrated in a laptop, but are all considered by
> 	 * libcamera as external. The true location for those cameras is
> 	 * specified in the HAL configuration file. If the camera location is
> 	 * external and a configuration entry exists for it, override the
> 	 * location.
> 	 */
> 
>> +	 */
>> +	if (isCameraNew && isCameraExternal && halConfig_.exists()) {
> 
> Is the isCameraNew check correct ? Even if the camera has been seen
> before, we should override isCameraExternal. The id, however, should
> only be overridden when the camera is new.
> 
>> +		const CameraConfigData *configData = halConfig_.cameraConfigData(cam->id());
> 
> It would be nice to avoid the double lookup of camera config data if
> possible. The CameraDevice::create() call could be moved further down,
> just before camera->initialize(), which may give us an opportunity to
> refactor the code.
> 
>> +		if (configData && configData->facing != CAMERA_FACING_EXTERNAL) {
>> +			isCameraExternal = false;
>> +			id = numInternalCameras_;
>> +		}
>> +	}
>> +
>>  	/* Create a CameraDevice instance to wrap the libcamera Camera. */
>>  	std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam);
>>  
>