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

[Jacopo Mondi]

Hi Umang,

On Wed, Jul 28, 2021 at 01:07:59PM +0530, Umang Jain wrote:
> Currently, all UVC cameras are reported with CameraLocationExternal [1]
> by libcamera-core, 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. CameraHalManager will now assign the numerical id
> of the camera accordingly, based on which the facing of the camera is set
> as well (as per the HAL config file) while initializing the CameraDevice
> wrapper.
>
> [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 | 18 ++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
Oh my, the logic to handle location is the most complex part of the
HAL :)

I think the code is fine, I would swap comments though.

> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 678cde23..c7f5fc4e 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.

or at least mention the UVC camera used as internal cameras here.

			/*
			 * 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.
                         */


> +			 */
> +			if (id_ < 1000 && cameraConfigData->facing != -1)
> +			       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..4950bd75 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -145,6 +145,24 @@ 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 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.
> +	 */

	/*
	 * 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. 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.
	 */

Just suggestions, take whatever you consider appropriate.

> +	if (isCameraNew && isCameraExternal) {
> +		if (cameraConfigData && cameraConfigData->facing != -1 &&
> +		    cameraConfigData->facing != CAMERA_FACING_EXTERNAL) {

Actually I don't think location external should be specified for
cameras listed in the config file. We should probably prohibit it in
the configuration file parser, something we allow at the moment. It
could be done on top and the check removed here.

Code is good!
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j


> +			isCameraExternal = false;
> +			id = numInternalCameras_;
> +		}
> +	}
> +



>  	if (!isCameraExternal && !cameraConfigData) {
>  		LOG(HAL, Error)
>  			<< "HAL configuration entry for internal camera "
> --
> 2.31.0
>