[libcamera-devel] [PATCH 6/7] android: camera_device: Get properties from config

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 25, 2021 at 11:39:09PM +0100, Niklas Söderlund wrote:
> On 2021-03-24 12:25:26 +0100, Jacopo Mondi wrote:
> > Create the CameraDevice with a reference to the HAL configuration
> > file and use it to retrieve device and camera properties.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp      | 71 ++++++++++++++++--------------
> >  src/android/camera_device.h        |  8 +++-
> >  src/android/camera_hal_manager.cpp |  3 +-
> >  3 files changed, 47 insertions(+), 35 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 72a89258386d..403d149e4f68 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -312,33 +312,22 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> >   * back to the framework using the designated callbacks.
> >   */
> >  
> > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
> > -	: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
> > -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> > +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera,
> > +			   CameraHalConfig &halConfig)
> > +	: id_(id), running_(false), camera_(camera), halConfig_(&halConfig),
> > +	  staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0)
> >  {
> > -	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> > -
> > -	maker_ = "libcamera";
> > -	model_ = "cameraModel";
> > -
> > -	/* \todo Support getting properties on Android */
> > -	std::ifstream fstream("/var/cache/camera/camera.prop");
> > -	if (!fstream.is_open())
> > -		return;
> > -
> > -	std::string line;
> > -	while (std::getline(fstream, line)) {
> > -		std::string::size_type delimPos = line.find("=");
> > -		if (delimPos == std::string::npos)
> > -			continue;
> > -		std::string key = line.substr(0, delimPos);
> > -		std::string val = line.substr(delimPos + 1);
> > -
> > -		if (!key.compare("ro.product.model"))
> > -			model_ = val;
> > -		else if (!key.compare("ro.product.manufacturer"))
> > -			maker_ = val;
> > +	maker_ = halConfig_->deviceMaker();
> > +	model_ = halConfig_->deviceModel();
> > +	if (maker_.empty() || model_.empty()) {
> > +		maker_ = "libcamera";
> > +		model_ = "cameraModel";
> > +		LOG(HAL, Warning)
> > +			<< "Cannot find manufacturer information. "
> > +			<< "Default it to '" << maker_ << " - " << model_;
> >  	}

Should we do this ? The existing code handles Chrome OS natively, isn't
it better than duplicating information in our configuration file ?

> > +
> > +	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> >  }
> >  
> >  CameraDevice::~CameraDevice()
> > @@ -351,9 +340,10 @@ CameraDevice::~CameraDevice()
> >  }
> >  
> >  std::shared_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> > -						   const std::shared_ptr<Camera> &cam)
> > +						   const std::shared_ptr<Camera> &cam,
> > +						   CameraHalConfig &halConfig)
> >  {
> > -	CameraDevice *camera = new CameraDevice(id, cam);
> > +	CameraDevice *camera = new CameraDevice(id, cam, halConfig);
> >  	return std::shared_ptr<CameraDevice>(camera);
> >  }
> >  
> > @@ -380,11 +370,28 @@ int CameraDevice::initialize()
> >  			break;
> >  		}
> >  	} else {
> > -		/*
> > -		 * \todo Retrieve the camera location from configuration file
> > -		 * if not available from the library.
> > -		 */
> > -		facing_ = CAMERA_FACING_FRONT;
> > +		std::string location = halConfig_->cameraLocation(camera_->id());
> > +		if (location.empty()) {
> > +			LOG(HAL, Error) << "Location for camera "
> > +					<< camera_->id() << " not reported";
> > +			return -EINVAL;
> > +		}
> 
> Would it make sens to cameraLocation() to translate from the string to 
> the enum value?

Seems like a good idea.

> > +
> > +		if (location == "front") {
> > +			facing_ = CAMERA_FACING_FRONT;
> > +		} else if (location == "back") {
> > +			facing_ = CAMERA_FACING_BACK;
> > +		} else if (location == "external") {
> > +			facing_ = CAMERA_FACING_EXTERNAL;
> > +		} else {
> > +			LOG(HAL, Error) << "Unsupported camera location "
> > +					<< location;
> > +			return -EINVAL;
> > +		}
> > +
> > +		LOG(HAL, Debug)
> > +			<< "Camera location retrieved from configration file: "
> > +			<< location;
> >  	}
> >  
> >  	/*
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 823d561cc295..33bfd115a703 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -24,6 +24,7 @@
> >  #include "libcamera/internal/log.h"
> >  #include "libcamera/internal/message.h"
> >  
> > +#include "camera_hal_config.h"

A forward declaratio of CameraHalConfig should be enough in this file.

> >  #include "camera_metadata.h"
> >  #include "camera_stream.h"
> >  #include "camera_worker.h"
> > @@ -33,7 +34,8 @@ class CameraDevice : protected libcamera::Loggable
> >  {
> >  public:
> >  	static std::shared_ptr<CameraDevice> create(unsigned int id,
> > -						    const std::shared_ptr<libcamera::Camera> &cam);
> > +						    const std::shared_ptr<libcamera::Camera> &cam,
> > +						    CameraHalConfig &halConfig);

const CameraHalConfig, as the camera device shouldn't change the
configuration ?

> >  	~CameraDevice();
> >  
> >  	int initialize();
> > @@ -66,7 +68,8 @@ protected:
> >  	std::string logPrefix() const override;
> >  
> >  private:
> > -	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> > +	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera,
> > +		     CameraHalConfig &halConfig);
> >  
> >  	struct Camera3RequestDescriptor {
> >  		Camera3RequestDescriptor(libcamera::Camera *camera,
> > @@ -113,6 +116,7 @@ private:
> >  	bool running_;
> >  	std::shared_ptr<libcamera::Camera> camera_;
> >  	std::unique_ptr<libcamera::CameraConfiguration> config_;
> > +	CameraHalConfig *halConfig_;

As this should never change during the lifetime of the CameraDevice, you
can make it a reference.

> >  
> >  	CameraMetadata *staticMetadata_;
> >  	std::map<unsigned int, const CameraMetadata *> requestTemplates_;
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > index a19f80edede8..4fb5c87e2a68 100644
> > --- a/src/android/camera_hal_manager.cpp
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -129,7 +129,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> >  	}
> >  
> >  	/* Create a CameraDevice instance to wrap the libcamera Camera. */
> > -	std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> > +	std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam),
> > +								    halConfig_);
> >  	int ret = camera->initialize();
> >  	if (ret) {
> >  		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();

-- 
Regards,

Laurent Pinchart