[libcamera-devel,v2,1/1] android: CameraHalManager: Create a static object dynamically
diff mbox series

Message ID 20210526084628.3893266-2-hiroh@chromium.org
State Accepted
Commit 459b3bc6a979a778f1689a86be3589101853977e
Headers show
Series
  • Fix SIGSEGV in ChromeOS camera3 test
Related show

Commit Message

Hirokazu Honda May 26, 2021, 8:46 a.m. UTC 
Originally CameraHalManager is created in the libcamera start up
and destroyed in the libcamera termination. However,
CameraHalManager destructor can access  other static objects that
has been destroyed.
Avoid this issue by destroying CameraHalManager when tear_down() is
called in ChromeOS or leaking it in other platforms.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera3_hal.cpp        | 13 ++++++-------
 src/android/camera_hal_manager.cpp |  7 +++++++
 src/android/camera_hal_manager.h   |  5 ++++-
 src/android/cros/camera3_hal.cpp   |  3 +++
 src/android/cros/meson.build       |  3 ++-
 5 files changed, 22 insertions(+), 9 deletions(-)

Comments

Laurent Pinchart May 26, 2021, 1:35 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Wed, May 26, 2021 at 05:46:28PM +0900, Hirokazu Honda wrote:
> Originally CameraHalManager is created in the libcamera start up
> and destroyed in the libcamera termination. However,
> CameraHalManager destructor can access  other static objects that
> has been destroyed.
> Avoid this issue by destroying CameraHalManager when tear_down() is
> called in ChromeOS or leaking it in other platforms.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera3_hal.cpp        | 13 ++++++-------
>  src/android/camera_hal_manager.cpp |  7 +++++++
>  src/android/camera_hal_manager.h   |  5 ++++-
>  src/android/cros/camera3_hal.cpp   |  3 +++
>  src/android/cros/meson.build       |  3 ++-
>  5 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> index 08773d33..e6d435e0 100644
> --- a/src/android/camera3_hal.cpp
> +++ b/src/android/camera3_hal.cpp
> @@ -16,25 +16,23 @@ using namespace libcamera;
>  
>  LOG_DEFINE_CATEGORY(HAL)
>  
> -static CameraHalManager cameraManager;
> -
>  /*------------------------------------------------------------------------------
>   * Android Camera HAL callbacks
>   */
>  
>  static int hal_get_number_of_cameras()
>  {
> -	return cameraManager.numCameras();
> +	return CameraHalManager::instance()->numCameras();
>  }
>  
>  static int hal_get_camera_info(int id, struct camera_info *info)
>  {
> -	return cameraManager.getCameraInfo(id, info);
> +	return CameraHalManager::instance()->getCameraInfo(id, info);
>  }
>  
>  static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
>  {
> -	cameraManager.setCallbacks(callbacks);
> +	CameraHalManager::instance()->setCallbacks(callbacks);
>  
>  	return 0;
>  }
> @@ -62,7 +60,7 @@ static int hal_init()
>  {
>  	LOG(HAL, Info) << "Initialising Android camera HAL";
>  
> -	cameraManager.init();
> +	CameraHalManager::instance()->init();
>  
>  	return 0;
>  }
> @@ -77,7 +75,8 @@ static int hal_dev_open(const hw_module_t *module, const char *name,
>  	LOG(HAL, Debug) << "Open camera " << name;
>  
>  	int id = atoi(name);
> -	auto [camera, ret] = cameraManager.open(id, module);
> +
> +	auto [camera, ret] = CameraHalManager::instance()->open(id, module);
>  	if (!camera) {
>  		LOG(HAL, Error)
>  			<< "Failed to open camera module '" << id << "'";
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index f5b86974..54087d3a 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -37,6 +37,13 @@ CameraHalManager::CameraHalManager()
>  /* CameraManager calls stop() in the destructor. */
>  CameraHalManager::~CameraHalManager() = default;
>  
> +/* static */
> +CameraHalManager *CameraHalManager::instance()
> +{
> +	static CameraHalManager *cameraHalManager = new CameraHalManager;
> +	return cameraHalManager;

I wonder if it would make sense to create the instance in the HAL's init
function, but for now I think this is good.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

It would be nice to have this fixed in Android itself, the fact that the
HAL API has an .init() function but no cleanup/exit/tear_down isn't
great.

> +}
> +
>  int CameraHalManager::init()
>  {
>  	cameraManager_ = std::make_unique<CameraManager>();
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index af1581da..db9354a7 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -26,9 +26,10 @@ class CameraDevice;
>  class CameraHalManager
>  {
>  public:
> -	CameraHalManager();
>  	~CameraHalManager();
>  
> +	static CameraHalManager *instance();
> +
>  	int init();
>  
>  	std::tuple<CameraDevice *, int>
> @@ -44,6 +45,8 @@ private:
>  
>  	static constexpr unsigned int firstExternalCameraId_ = 1000;
>  
> +	CameraHalManager();
> +
>  	static int32_t cameraLocation(const libcamera::Camera *cam);
>  
>  	void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
> diff --git a/src/android/cros/camera3_hal.cpp b/src/android/cros/camera3_hal.cpp
> index 31ad36ac..d6fc1d0f 100644
> --- a/src/android/cros/camera3_hal.cpp
> +++ b/src/android/cros/camera3_hal.cpp
> @@ -7,12 +7,15 @@
>  
>  #include <cros-camera/cros_camera_hal.h>
>  
> +#include "../camera_hal_manager.h"
> +
>  static void set_up(cros::CameraMojoChannelManagerToken *token)
>  {
>  }
>  
>  static void tear_down()
>  {
> +	delete CameraHalManager::instance();
>  }
>  
>  cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {
> diff --git a/src/android/cros/meson.build b/src/android/cros/meson.build
> index 4aab0f20..13ec8f0a 100644
> --- a/src/android/cros/meson.build
> +++ b/src/android/cros/meson.build
> @@ -12,6 +12,7 @@ cros_hal_info = static_library('cros_hal_info',
>                                 cros_hal_info_sources,
>                                 dependencies : dependency('libcros_camera'),
>                                 c_args : '-Wno-shadow',
> -                               include_directories : android_includes)
> +                               include_directories : [android_includes,
> +                                                      libcamera_includes])
>  
>  libcamera_objects += cros_hal_info.extract_objects('camera3_hal.cpp')
Jacopo Mondi May 26, 2021, 7:57 p.m. UTC | #2 
Hi Hiro,

On Wed, May 26, 2021 at 05:46:28PM +0900, Hirokazu Honda wrote:
> Originally CameraHalManager is created in the libcamera start up
> and destroyed in the libcamera termination. However,
> CameraHalManager destructor can access  other static objects that
> has been destroyed.
> Avoid this issue by destroying CameraHalManager when tear_down() is
> called in ChromeOS or leaking it in other platforms.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  src/android/camera3_hal.cpp        | 13 ++++++-------
>  src/android/camera_hal_manager.cpp |  7 +++++++
>  src/android/camera_hal_manager.h   |  5 ++++-
>  src/android/cros/camera3_hal.cpp   |  3 +++
>  src/android/cros/meson.build       |  3 ++-
>  5 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> index 08773d33..e6d435e0 100644
> --- a/src/android/camera3_hal.cpp
> +++ b/src/android/camera3_hal.cpp
> @@ -16,25 +16,23 @@ using namespace libcamera;
>
>  LOG_DEFINE_CATEGORY(HAL)
>
> -static CameraHalManager cameraManager;
> -
>  /*------------------------------------------------------------------------------
>   * Android Camera HAL callbacks
>   */
>
>  static int hal_get_number_of_cameras()
>  {
> -	return cameraManager.numCameras();
> +	return CameraHalManager::instance()->numCameras();
>  }
>
>  static int hal_get_camera_info(int id, struct camera_info *info)
>  {
> -	return cameraManager.getCameraInfo(id, info);
> +	return CameraHalManager::instance()->getCameraInfo(id, info);
>  }
>
>  static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
>  {
> -	cameraManager.setCallbacks(callbacks);
> +	CameraHalManager::instance()->setCallbacks(callbacks);
>
>  	return 0;
>  }
> @@ -62,7 +60,7 @@ static int hal_init()
>  {
>  	LOG(HAL, Info) << "Initialising Android camera HAL";
>
> -	cameraManager.init();
> +	CameraHalManager::instance()->init();
>
>  	return 0;
>  }
> @@ -77,7 +75,8 @@ static int hal_dev_open(const hw_module_t *module, const char *name,
>  	LOG(HAL, Debug) << "Open camera " << name;
>
>  	int id = atoi(name);
> -	auto [camera, ret] = cameraManager.open(id, module);
> +
> +	auto [camera, ret] = CameraHalManager::instance()->open(id, module);
>  	if (!camera) {
>  		LOG(HAL, Error)
>  			<< "Failed to open camera module '" << id << "'";
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index f5b86974..54087d3a 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -37,6 +37,13 @@ CameraHalManager::CameraHalManager()
>  /* CameraManager calls stop() in the destructor. */
>  CameraHalManager::~CameraHalManager() = default;
>
> +/* static */
> +CameraHalManager *CameraHalManager::instance()
> +{
> +	static CameraHalManager *cameraHalManager = new CameraHalManager;
> +	return cameraHalManager;
> +}
> +
>  int CameraHalManager::init()
>  {
>  	cameraManager_ = std::make_unique<CameraManager>();
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index af1581da..db9354a7 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -26,9 +26,10 @@ class CameraDevice;
>  class CameraHalManager
>  {
>  public:
> -	CameraHalManager();
>  	~CameraHalManager();
>
> +	static CameraHalManager *instance();
> +
>  	int init();
>
>  	std::tuple<CameraDevice *, int>
> @@ -44,6 +45,8 @@ private:
>
>  	static constexpr unsigned int firstExternalCameraId_ = 1000;
>
> +	CameraHalManager();
> +
>  	static int32_t cameraLocation(const libcamera::Camera *cam);
>
>  	void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
> diff --git a/src/android/cros/camera3_hal.cpp b/src/android/cros/camera3_hal.cpp
> index 31ad36ac..d6fc1d0f 100644
> --- a/src/android/cros/camera3_hal.cpp
> +++ b/src/android/cros/camera3_hal.cpp
> @@ -7,12 +7,15 @@
>
>  #include <cros-camera/cros_camera_hal.h>
>
> +#include "../camera_hal_manager.h"
> +
>  static void set_up(cros::CameraMojoChannelManagerToken *token)
>  {
>  }
>
>  static void tear_down()
>  {
> +	delete CameraHalManager::instance();
>  }
>
>  cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {
> diff --git a/src/android/cros/meson.build b/src/android/cros/meson.build
> index 4aab0f20..13ec8f0a 100644
> --- a/src/android/cros/meson.build
> +++ b/src/android/cros/meson.build
> @@ -12,6 +12,7 @@ cros_hal_info = static_library('cros_hal_info',
>                                 cros_hal_info_sources,
>                                 dependencies : dependency('libcros_camera'),
>                                 c_args : '-Wno-shadow',
> -                               include_directories : android_includes)
> +                               include_directories : [android_includes,
> +                                                      libcamera_includes])
>
>  libcamera_objects += cros_hal_info.extract_objects('camera3_hal.cpp')
> --
> 2.31.1.818.g46aad6cb9e-goog
>

Patch
diff mbox series 

diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
index 08773d33..e6d435e0 100644
--- a/src/android/camera3_hal.cpp
+++ b/src/android/camera3_hal.cpp
@@ -16,25 +16,23 @@  using namespace libcamera;
 
 LOG_DEFINE_CATEGORY(HAL)
 
-static CameraHalManager cameraManager;
-
 /*------------------------------------------------------------------------------
  * Android Camera HAL callbacks
  */
 
 static int hal_get_number_of_cameras()
 {
-	return cameraManager.numCameras();
+	return CameraHalManager::instance()->numCameras();
 }
 
 static int hal_get_camera_info(int id, struct camera_info *info)
 {
-	return cameraManager.getCameraInfo(id, info);
+	return CameraHalManager::instance()->getCameraInfo(id, info);
 }
 
 static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
 {
-	cameraManager.setCallbacks(callbacks);
+	CameraHalManager::instance()->setCallbacks(callbacks);
 
 	return 0;
 }
@@ -62,7 +60,7 @@  static int hal_init()
 {
 	LOG(HAL, Info) << "Initialising Android camera HAL";
 
-	cameraManager.init();
+	CameraHalManager::instance()->init();
 
 	return 0;
 }
@@ -77,7 +75,8 @@  static int hal_dev_open(const hw_module_t *module, const char *name,
 	LOG(HAL, Debug) << "Open camera " << name;
 
 	int id = atoi(name);
-	auto [camera, ret] = cameraManager.open(id, module);
+
+	auto [camera, ret] = CameraHalManager::instance()->open(id, module);
 	if (!camera) {
 		LOG(HAL, Error)
 			<< "Failed to open camera module '" << id << "'";
diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
index f5b86974..54087d3a 100644
--- a/src/android/camera_hal_manager.cpp
+++ b/src/android/camera_hal_manager.cpp
@@ -37,6 +37,13 @@  CameraHalManager::CameraHalManager()
 /* CameraManager calls stop() in the destructor. */
 CameraHalManager::~CameraHalManager() = default;
 
+/* static */
+CameraHalManager *CameraHalManager::instance()
+{
+	static CameraHalManager *cameraHalManager = new CameraHalManager;
+	return cameraHalManager;
+}
+
 int CameraHalManager::init()
 {
 	cameraManager_ = std::make_unique<CameraManager>();
diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
index af1581da..db9354a7 100644
--- a/src/android/camera_hal_manager.h
+++ b/src/android/camera_hal_manager.h
@@ -26,9 +26,10 @@  class CameraDevice;
 class CameraHalManager
 {
 public:
-	CameraHalManager();
 	~CameraHalManager();
 
+	static CameraHalManager *instance();
+
 	int init();
 
 	std::tuple<CameraDevice *, int>
@@ -44,6 +45,8 @@  private:
 
 	static constexpr unsigned int firstExternalCameraId_ = 1000;
 
+	CameraHalManager();
+
 	static int32_t cameraLocation(const libcamera::Camera *cam);
 
 	void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
diff --git a/src/android/cros/camera3_hal.cpp b/src/android/cros/camera3_hal.cpp
index 31ad36ac..d6fc1d0f 100644
--- a/src/android/cros/camera3_hal.cpp
+++ b/src/android/cros/camera3_hal.cpp
@@ -7,12 +7,15 @@ 
 
 #include <cros-camera/cros_camera_hal.h>
 
+#include "../camera_hal_manager.h"
+
 static void set_up(cros::CameraMojoChannelManagerToken *token)
 {
 }
 
 static void tear_down()
 {
+	delete CameraHalManager::instance();
 }
 
 cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {
diff --git a/src/android/cros/meson.build b/src/android/cros/meson.build
index 4aab0f20..13ec8f0a 100644
--- a/src/android/cros/meson.build
+++ b/src/android/cros/meson.build
@@ -12,6 +12,7 @@  cros_hal_info = static_library('cros_hal_info',
                                cros_hal_info_sources,
                                dependencies : dependency('libcros_camera'),
                                c_args : '-Wno-shadow',
-                               include_directories : android_includes)
+                               include_directories : [android_includes,
+                                                      libcamera_includes])
 
 libcamera_objects += cros_hal_info.extract_objects('camera3_hal.cpp')