[libcamera-devel] [PATCH v2 1/1] android: CameraHalManager: Create a static object dynamically

[Jacopo Mondi]

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 at chromium.org>

Reviewed-by: Jacopo Mondi <jacopo at 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
>