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

[Umang Jain]

Hi Kieran?

On 7/27/21 9:09 PM, Kieran Bingham wrote:
> 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?

Not sure, if I understand it correctly, but this is the check for the 
situation where libcamera-core reports camera's properties::Location as 
CameraLocationExternal. See 76809320bb1a. So, before setting the 
facing_, we just check whether the CameraHalManager has classified the 
camera as internal(id < 1000) or external(remove-able, id >= 1000) to 
the system. If it's internal, we grab/set the facing_ from the hal 
config file.

Does it make sense?

>
> Will that work? won't it get set regardless in the lines after this case
> statement? :
which lines? The switch-case block is inside a if block, followed by 
else if / else.
>
>> 	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);
>>>