[libcamera-devel] [PATCH v3 3/4] android: Override camera as "Internal" provided found in HAL config

[Umang Jain]

Hi Laurent,

On 8/2/21 10:54 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Aug 02, 2021 at 03:21:14PM +0530, Umang Jain wrote:
>> On 8/2/21 2:16 PM, paul.elder at ideasonboard.com wrote:
>>> On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote:
>>>> Currently, all UVC cameras are reported with CameraLocationExternal [1]
>>>> by libcamera-core, since there is no universal information or standard,
>>> s/,$//
>>>
>>>> to know the location of these cameras. However, in the libcamera HAL
>>>> layer, we can make an informed decision whether its external or
>>> s/its/it's/
>>>
>>>> internal, simply by checking the presence of it in the HAL
>>> s/the presence of it/its presence/
>>>
>>>> configuration file.
>>>>
>>>> If the camera is found to be present in the HAL configuration file,
>>> s/to be present//
>>>
>>>> treat it as internal. CameraHalManager will now assign the numerical id
>>>> of the camera accordingly, based on which the facing of the camera is set
>>> I didn't think the facing of the camera affects the id?
>> Well, it should be kept in sync. If the camera is internal (id < 1000),
>> but it's facing is set to be external (as per checks in CameraDevice),
>> then these are not in sync. We need to take care of that.
>>
>> I don't think anything bad will happen if these are out of sync. But why
>> should we allow it to be out-of-sync if we handle it gracefully?
> I agree, it's best to keep them in sync.
>
>>>> as well (as per the HAL config file) while initializing the CameraDevice
>>>> wrapper.
>>> I don't quite understand this last part.
>> So in the  CameraDevice::initialize(), the facing of the camera is set.
>> If properties::Location doesn't match the location in the HAL config,
>> CameraDevice complains a warning.
>>
>> But, we know that for UVC properties:::Location won't be matching the
>> HAL config. In that case we rely on id assigned by CameraManager to see
>> if its a  internal camera or not. If yes, we just assign facing whatever
>> is mentioned in the HAL config file :)
> The last sentence of the commit message can probably be clarified a bit.
> How about this ?
>
> "The CameraHalManager will now assign the numerical id of the camera
> accordingly when initializing the CameraDevice, based on the camera
> facing value set in the HAL config file."
>
>>>> [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      | 13 ++++++++++++-
>>>>    src/android/camera_hal_manager.cpp | 17 +++++++++++++++++
>>>>    2 files changed, 29 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>>> index 678cde23..394ebc84 100644
>>>> --- a/src/android/camera_device.cpp
>>>> +++ b/src/android/camera_device.cpp
>>>> @@ -330,7 +330,18 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
>>>>    			facing_ = CAMERA_FACING_BACK;
>>>>    			break;
>>>>    		case properties::CameraLocationExternal:
>>>> -			facing_ = CAMERA_FACING_EXTERNAL;
>>>> +			/*
>>>> +			 * If the camera is reported as external, but
>>>> +			 * the CameraHalManager has overriden it, use
>>>> +			 * what is reported in the configuration file.
>>>> +			 * This typically happens for UVC cameras
>>>> +			 * reported as 'External' by libcamera but
>>>> +			 * installed in fixed position on the device.
>>>> +			 */
> This could be reflowed to 80 columns.
>
>>>> +			if (id_ < 1000 && cameraConfigData->facing != -1)
> Is the id_ check needed ? If libcamera reports the camera as external
> and the camera data overrides the facing value, the id will always be <
> 1000, won't it ?

Ah yes, we can drop the id_ check (although it helps to read the code 
tbh) and instead have

+			if (cameraConfigData && cameraConfigData->facing != -1)

Should be fine as well.
>
>>>> +			       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 b364f62a..cce98fad 100644
>>>> --- a/src/android/camera_hal_manager.cpp
>>>> +++ b/src/android/camera_hal_manager.cpp
>>>> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>>>>    	}
>>>>    
>>>>    	const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());
>>>> +
>>>> +	/*
>>>> +	 * Some cameras whose location is reported by libcamera as external may
>>>> +	 * actually be internal to the device. This is common with UVC cameras
>>>> +	 * that are integrated in a laptop. In that case the real location
>>>> +	 * should be specified in the configuration file.
>>>> +	 *
>>>> +	 * If the camera location is external and a configuration
>>>> +	 * entry exists for it, override its location.
> You could also reflow this to 80 columns.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>>>> +	 */
>>>> +	if (isCameraNew && isCameraExternal) {
>>>> +		if (cameraConfigData && cameraConfigData->facing != -1) {
>>>> +			isCameraExternal = false;
>>>> +			id = numInternalCameras_;
>>>> +		}
>>>> +	}
>>>> +
>>>>    	if (!isCameraExternal && !cameraConfigData) {
>>>>    		LOG(HAL, Error)
>>>>    			<< "HAL configuration entry for internal camera "