[libcamera-devel] [PATCH 4/5] android: camera_device: Get properties from configuration

[Kieran Bingham]

Hi Jacopo,

On 13/04/2021 21:00, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Tue, Apr 13, 2021 at 04:50:41PM +0200, Jacopo Mondi wrote:
>> Open the HAL configuration file in the Camera HAL manager and get
>> the camera properties for each created CameraDevice and initialize it
>> with them.
>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>  src/android/camera_device.cpp      | 56 +++++++++++++++++++++++++-----
>>  src/android/camera_device.h        |  3 +-
>>  src/android/camera_hal_manager.cpp | 37 +++++++++++++++++++-
>>  src/android/camera_hal_manager.h   |  3 ++
>>  4 files changed, 89 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 89044efa7ebe..2e93936fdb4b 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -6,6 +6,7 @@
>>   */
>>  
>>  #include "camera_device.h"
>> +#include "camera_hal_config.h"
>>  #include "camera_ops.h"
>>  #include "post_processor.h"
>>  
>> @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
>>  }
>>  
>>  /*
>> - * Initialize the camera static information.
>> + * Initialize the camera static information retrieved from the
>> + * Camera::properties or from the cameraPros.
>> + *
>> + * cameraProps is optional for external camera devices and is defaulted to
>> + * nullptr.
>> + *
>>   * This method is called before the camera device is opened.
>>   */
>> -int CameraDevice::initialize()
>> +int CameraDevice::initialize(const CameraProps *cameraProps)
>>  {
>> -	/* Initialize orientation and facing side of the camera. */
>> +	/*
>> +	 * Initialize orientation and facing side of the camera.
>> +	 *
>> +	 * If the libcamera::Camera provides those information as retrieved
>> +	 * from firmware use them, otherwise fallback to values parsed from
>> +	 * the configuration file. If the configuration file is not available
>> +	 * the camera is external so its location and rotation can be safely
>> +	 * defaulted.
>> +	 */
>>  	const ControlList &properties = camera_->properties();
>>  
>>  	if (properties.contains(properties::Location)) {
>> @@ -464,12 +478,22 @@ int CameraDevice::initialize()
>>  			facing_ = CAMERA_FACING_EXTERNAL;
>>  			break;
>>  		}
>> +
>> +		if (cameraProps && cameraProps->facing != -1 &&
>> +		    facing_ != cameraProps->facing) {
>> +			LOG(HAL, Warning)
>> +				<< "Camera location does not match"
>> +				<< " configuration file. Using " << facing_;
>> +		}
>> +	} else if (cameraProps) {
>> +		if (cameraProps->facing == -1) {
>> +			LOG(HAL, Error)
>> +				<< "Camera facing not in configuration file";
>> +			return -EINVAL;
>> +		}
>> +		facing_ = cameraProps->facing;
>>  	} else {
>> -		/*
>> -		 * \todo Retrieve the camera location from configuration file
>> -		 * if not available from the library.
>> -		 */
>> -		facing_ = CAMERA_FACING_FRONT;
>> +		facing_ = CAMERA_FACING_EXTERNAL;
>>  	}
>>  
>>  	/*
>> @@ -483,8 +507,24 @@ int CameraDevice::initialize()
>>  	if (properties.contains(properties::Rotation)) {
>>  		int rotation = properties.get(properties::Rotation);
>>  		orientation_ = (360 - rotation) % 360;
>> +		if (cameraProps && cameraProps->rotation != -1 &&
>> +		    orientation_ != cameraProps->rotation) {
>> +			LOG(HAL, Warning)
>> +				<< "Camera orientation does not match"
>> +				<< " configuration file. Using " << orientation_;
>> +		}
>> +	} else if (cameraProps) {
>> +		if (cameraProps->rotation == -1) {
>> +			LOG(HAL, Error)
>> +				<< "Camera rotation not in configuration file";
>> +			return -EINVAL;
>> +		}
>> +		orientation_ = cameraProps->rotation;
>> +	} else {
>> +		orientation_ = 0;
>>  	}
>>  
>> +	/* Acquire the camera and initialize available stream configurations. */
>>  	int ret = camera_->acquire();
>>  	if (ret) {
>>  		LOG(HAL, Error) << "Failed to temporarily acquire the camera";
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 11bdfec8d587..598d89f1cff0 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -29,6 +29,7 @@
>>  #include "camera_worker.h"
>>  #include "jpeg/encoder.h"
>>  
>> +class CameraProps;

This triggers a compiler warning on GCC

In file included from ../src/android/camera_device.cpp:9:
../src/android/camera_hal_config.h:16:1: error: 'CameraProps' defined as
a struct here but previously declared as a class; this is valid, but may
result in linker errors under the Microsoft C++ ABI
[-Werror,-Wmismatched-tags]
struct CameraProps {
^
../src/android/camera_device.h:32:1: note: did you mean struct here?
class CameraProps;



>>  class CameraDevice : protected libcamera::Loggable
>>  {
>>  public:
>> @@ -36,7 +37,7 @@ public:
>>  						    std::shared_ptr<libcamera::Camera> cam);
>>  	~CameraDevice();
>>  
>> -	int initialize();
>> +	int initialize(const CameraProps *cameraProps = nullptr);
> 
> You can drop the = nullptr if you agree with the change below.
> 
>>  
>>  	int open(const hw_module_t *hardwareModule);
>>  	void close();
>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
>> index bf3fcda75237..1defc3f9c5bd 100644
>> --- a/src/android/camera_hal_manager.cpp
>> +++ b/src/android/camera_hal_manager.cpp
>> @@ -41,6 +41,16 @@ int CameraHalManager::init()
>>  {
>>  	cameraManager_ = std::make_unique<CameraManager>();
>>  
>> +	/*
>> +	 * Open and parse configuration file.
>> +	 *
>> +	 * If the configuration file is not available the HAL only supports
>> +	 * external cameras. If it exists but it's not valid then error out.
>> +	 */
>> +	halConfig_.open();
>> +	if (halConfig_.exists() && !halConfig_.valid())
>> +		return -EINVAL;
>> +
>>  	/* Support camera hotplug. */
>>  	cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);
>>  	cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);
>> @@ -100,6 +110,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>>  	auto iter = cameraIdsMap_.find(cam->id());
>>  	if (iter != cameraIdsMap_.end()) {
>>  		id = iter->second;
>> +		if (id >= firstExternalCameraId_)
>> +			isCameraExternal = true;
>>  	} else {
>>  		isCameraNew = true;
>>  
>> @@ -117,7 +129,30 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>>  
>>  	/* Create a CameraDevice instance to wrap the libcamera Camera. */
>>  	std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam);
>> -	int ret = camera->initialize();
>> +
>> +	/*
>> +	 * The configuration file must be valid for internal cameras.
>> +	 * External cameras can be initialized without configuration file.
>> +	 */
>> +	int ret = -EINVAL;
>> +	if (!halConfig_.exists()) {
>> +		if (isCameraExternal)
>> +			ret = camera->initialize();
>> +	} else {
>> +		/*
>> +		 * Get camera properties from the configuration file which
>> +		 * exists and is valid.
>> +		 *
>> +		 * Internal cameras are required to have a corresponding entry
>> +		 * in the configuration file. External cameras are not required
>> +		 * to.
>> +		 */
>> +		const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id());
>> +		if (cameraProps->valid)
>> +			ret = camera->initialize(cameraProps);
>> +		else if (isCameraExternal)
>> +			ret = camera->initialize();
>> +	}
>>  	if (ret) {
>>  		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
>>  		return;
> 
> If the configuration file does not exist and the camera is internal,
> we'll only print "Failed to initialize camera", which will be confusing.
> How about the following ?
> 
> 	/*
> 	 * The configuration file must be valid, and contain a corresponding
> 	 * entry for internal cameras. External cameras can be initialized
> 	 * without configuration file.
> 	 */
> 	if (!isCameraExternal && !halConfig_.exists()) {
> 		LOG(HALConfig, Error)
> 			<< "HAL configuration file is mandatory for internal cameras";
> 		return -EINVAL;
> 	}
> 
> 	const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id());
> 	if (!isCameraExternal && !cameraProps) {
> 		LOG(HALConfig, Error)
> 			<< "HAL configuration entry for camera " << cam->id()
> 			<< " is missing";
> 		return -EINVAL;
> 	}
> 
> 	ret = camera->initialize(cameraProps);
> 	if (ret) {
> 		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
> 		return;
> 	}
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> 
>> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
>> index d9bf27989965..af1581da6579 100644
>> --- a/src/android/camera_hal_manager.h
>> +++ b/src/android/camera_hal_manager.h
>> @@ -19,6 +19,8 @@
>>  
>>  #include <libcamera/camera_manager.h>
>>  
>> +#include "camera_hal_config.h"
>> +
>>  class CameraDevice;
>>  
>>  class CameraHalManager
>> @@ -50,6 +52,7 @@ private:
>>  	CameraDevice *cameraDeviceFromHalId(unsigned int id);
>>  
>>  	std::unique_ptr<libcamera::CameraManager> cameraManager_;
>> +	CameraHalConfig halConfig_;
>>  
>>  	const camera_module_callbacks_t *callbacks_;
>>  	std::vector<std::unique_ptr<CameraDevice>> cameras_;
> 

-- 
Regards
--
Kieran