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

[Jacopo Mondi]

Hi Kieran,

On Wed, Apr 14, 2021 at 07:33:26PM +0100, Kieran Bingham wrote:
> 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;
>

Uuuups, yes indeed! Thanks for spotting!

>
>
> >>  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