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

[Laurent Pinchart]

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)
> +				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);
>  

-- 
Regards,

Laurent Pinchart