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

[Laurent Pinchart]

On Tue, Jul 27, 2021 at 11:18:58PM +0530, Umang Jain wrote:
> On 7/27/21 8:57 PM, Laurent Pinchart wrote:
> > 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 ?
>
> oops!
>
> > 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.
> 
> I didn't understand the part ... "if camera has been seen before, we 
> should override isCameraExternal" part.
>
> I think this check holds. If the internal UVC camera is seen before, it 
> will be re-assigned same numeric-id (which, as per this patch, will be < 
> 1000). Hence, I do not think we need to over-ride anything here, because 
> CameraHalManager already knows that it is an internal camera (based on 
> previous-id re-assign at the start of the function). Am I missing 
> something?

You're right, isCameraExternal is only used when isCameraNew == true.

> >> +		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.
>
> Yeah makes sense, I'll see how can I refactor a bit to avoid double-lookup.
>
> >> +		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