[libcamera-devel,v2,5/6] android: camera_device: Get location from config
diff mbox series

Message ID 20210329152807.28331-5-jacopo@jmondi.org
State Superseded
Delegated to: Jacopo Mondi
Headers show
Series
  • [libcamera-devel,v2,1/6] libcamera: List dependency for Android Camera3 HAL
Related show

Commit Message

Jacopo Mondi March 29, 2021, 3:28 p.m. UTC 
Create the CameraDevice with a reference to the HAL configuration
file and use it to retrieve the camera location if not available
from the Camera.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp      | 18 +++++++++---------
 src/android/camera_device.h        |  8 ++++++--
 src/android/camera_hal_manager.cpp |  3 ++-
 3 files changed, 17 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart March 30, 2021, 12:17 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Mar 29, 2021 at 05:28:06PM +0200, Jacopo Mondi wrote:
> Create the CameraDevice with a reference to the HAL configuration
> file and use it to retrieve the camera location if not available
> from the Camera.

Overall this looks good, but I still think we should pass the camera
configuration to the CameraDevice class, not the full CameraHalConfig.
I'll comment further on this topic in the review of 4/6.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp      | 18 +++++++++---------
>  src/android/camera_device.h        |  8 ++++++--
>  src/android/camera_hal_manager.cpp |  3 ++-
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae6936647660..1731fe166887 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -309,9 +309,10 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
>   * back to the framework using the designated callbacks.
>   */
>  
> -CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera,
> +			   CameraHalConfig &halConfig)
>  	: id_(id), running_(false), camera_(std::move(camera)),
> -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> +	  halConfig_(halConfig), facing_(CAMERA_FACING_FRONT), orientation_(0)
>  {
>  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
>  
> @@ -341,10 +342,11 @@ CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
>  CameraDevice::~CameraDevice() = default;
>  
>  std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> -						   std::shared_ptr<Camera> cam)
> +						   std::shared_ptr<Camera> cam,
> +						   CameraHalConfig &halConfig)
>  {
>  	return std::unique_ptr<CameraDevice>(
> -		new CameraDevice(id, std::move(cam)));
> +		new CameraDevice(id, std::move(cam), halConfig));
>  }
>  
>  /*
> @@ -370,11 +372,9 @@ int CameraDevice::initialize()
>  			break;
>  		}
>  	} else {
> -		/*
> -		 * \todo Retrieve the camera location from configuration file
> -		 * if not available from the library.
> -		 */
> -		facing_ = CAMERA_FACING_FRONT;
> +		facing_ = halConfig_.cameraLocation(camera_->id());
> +		if (facing_ < 0)
> +			return facing_;
>  	}
>  
>  	/*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 11bdfec8d587..6355e8d8c26a 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"
>  #include "camera_metadata.h"
>  #include "camera_stream.h"
>  #include "camera_worker.h"
> @@ -33,7 +34,8 @@ class CameraDevice : protected libcamera::Loggable
>  {
>  public:
>  	static std::unique_ptr<CameraDevice> create(unsigned int id,
> -						    std::shared_ptr<libcamera::Camera> cam);
> +						    std::shared_ptr<libcamera::Camera> cam,
> +						    CameraHalConfig &halConfig);
>  	~CameraDevice();
>  
>  	int initialize();
> @@ -66,7 +68,8 @@ protected:
>  	std::string logPrefix() const override;
>  
>  private:
> -	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> +	CameraDevice(unsigned int id, 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_;
> +	const CameraHalConfig &halConfig_;
>  
>  	std::unique_ptr<CameraMetadata> staticMetadata_;
>  	std::map<unsigned int, std::unique_ptr<CameraMetadata>> requestTemplates_;
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index f79789b5bfb8..9ff7534a16f3 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -120,7 +120,8 @@ 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);
> +	std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam,
> +								    halConfig_);
>  	int ret = camera->initialize();
>  	if (ret) {
>  		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
Laurent Pinchart March 30, 2021, 12:17 a.m. UTC | #2 
On Tue, Mar 30, 2021 at 03:17:21AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Mar 29, 2021 at 05:28:06PM +0200, Jacopo Mondi wrote:
> > Create the CameraDevice with a reference to the HAL configuration
> > file and use it to retrieve the camera location if not available
> > from the Camera.
> 
> Overall this looks good, but I still think we should pass the camera
> configuration to the CameraDevice class, not the full CameraHalConfig.
> I'll comment further on this topic in the review of 4/6.

Sorry, I meant 3/6.

> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp      | 18 +++++++++---------
> >  src/android/camera_device.h        |  8 ++++++--
> >  src/android/camera_hal_manager.cpp |  3 ++-
> >  3 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ae6936647660..1731fe166887 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -309,9 +309,10 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> >   * back to the framework using the designated callbacks.
> >   */
> >  
> > -CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> > +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera,
> > +			   CameraHalConfig &halConfig)
> >  	: id_(id), running_(false), camera_(std::move(camera)),
> > -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> > +	  halConfig_(halConfig), facing_(CAMERA_FACING_FRONT), orientation_(0)
> >  {
> >  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> >  
> > @@ -341,10 +342,11 @@ CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> >  CameraDevice::~CameraDevice() = default;
> >  
> >  std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> > -						   std::shared_ptr<Camera> cam)
> > +						   std::shared_ptr<Camera> cam,
> > +						   CameraHalConfig &halConfig)
> >  {
> >  	return std::unique_ptr<CameraDevice>(
> > -		new CameraDevice(id, std::move(cam)));
> > +		new CameraDevice(id, std::move(cam), halConfig));
> >  }
> >  
> >  /*
> > @@ -370,11 +372,9 @@ int CameraDevice::initialize()
> >  			break;
> >  		}
> >  	} else {
> > -		/*
> > -		 * \todo Retrieve the camera location from configuration file
> > -		 * if not available from the library.
> > -		 */
> > -		facing_ = CAMERA_FACING_FRONT;
> > +		facing_ = halConfig_.cameraLocation(camera_->id());
> > +		if (facing_ < 0)
> > +			return facing_;
> >  	}
> >  
> >  	/*
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 11bdfec8d587..6355e8d8c26a 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"
> >  #include "camera_metadata.h"
> >  #include "camera_stream.h"
> >  #include "camera_worker.h"
> > @@ -33,7 +34,8 @@ class CameraDevice : protected libcamera::Loggable
> >  {
> >  public:
> >  	static std::unique_ptr<CameraDevice> create(unsigned int id,
> > -						    std::shared_ptr<libcamera::Camera> cam);
> > +						    std::shared_ptr<libcamera::Camera> cam,
> > +						    CameraHalConfig &halConfig);
> >  	~CameraDevice();
> >  
> >  	int initialize();
> > @@ -66,7 +68,8 @@ protected:
> >  	std::string logPrefix() const override;
> >  
> >  private:
> > -	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> > +	CameraDevice(unsigned int id, 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_;
> > +	const CameraHalConfig &halConfig_;
> >  
> >  	std::unique_ptr<CameraMetadata> staticMetadata_;
> >  	std::map<unsigned int, std::unique_ptr<CameraMetadata>> requestTemplates_;
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > index f79789b5bfb8..9ff7534a16f3 100644
> > --- a/src/android/camera_hal_manager.cpp
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -120,7 +120,8 @@ 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);
> > +	std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam,
> > +								    halConfig_);
> >  	int ret = camera->initialize();
> >  	if (ret) {
> >  		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index ae6936647660..1731fe166887 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -309,9 +309,10 @@  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
  * back to the framework using the designated callbacks.
  */
 
-CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
+CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera,
+			   CameraHalConfig &halConfig)
 	: id_(id), running_(false), camera_(std::move(camera)),
-	  facing_(CAMERA_FACING_FRONT), orientation_(0)
+	  halConfig_(halConfig), facing_(CAMERA_FACING_FRONT), orientation_(0)
 {
 	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
 
@@ -341,10 +342,11 @@  CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
 CameraDevice::~CameraDevice() = default;
 
 std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
-						   std::shared_ptr<Camera> cam)
+						   std::shared_ptr<Camera> cam,
+						   CameraHalConfig &halConfig)
 {
 	return std::unique_ptr<CameraDevice>(
-		new CameraDevice(id, std::move(cam)));
+		new CameraDevice(id, std::move(cam), halConfig));
 }
 
 /*
@@ -370,11 +372,9 @@  int CameraDevice::initialize()
 			break;
 		}
 	} else {
-		/*
-		 * \todo Retrieve the camera location from configuration file
-		 * if not available from the library.
-		 */
-		facing_ = CAMERA_FACING_FRONT;
+		facing_ = halConfig_.cameraLocation(camera_->id());
+		if (facing_ < 0)
+			return facing_;
 	}
 
 	/*
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 11bdfec8d587..6355e8d8c26a 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"
 #include "camera_metadata.h"
 #include "camera_stream.h"
 #include "camera_worker.h"
@@ -33,7 +34,8 @@  class CameraDevice : protected libcamera::Loggable
 {
 public:
 	static std::unique_ptr<CameraDevice> create(unsigned int id,
-						    std::shared_ptr<libcamera::Camera> cam);
+						    std::shared_ptr<libcamera::Camera> cam,
+						    CameraHalConfig &halConfig);
 	~CameraDevice();
 
 	int initialize();
@@ -66,7 +68,8 @@  protected:
 	std::string logPrefix() const override;
 
 private:
-	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
+	CameraDevice(unsigned int id, 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_;
+	const CameraHalConfig &halConfig_;
 
 	std::unique_ptr<CameraMetadata> staticMetadata_;
 	std::map<unsigned int, std::unique_ptr<CameraMetadata>> requestTemplates_;
diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
index f79789b5bfb8..9ff7534a16f3 100644
--- a/src/android/camera_hal_manager.cpp
+++ b/src/android/camera_hal_manager.cpp
@@ -120,7 +120,8 @@  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);
+	std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam,
+								    halConfig_);
 	int ret = camera->initialize();
 	if (ret) {
 		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();