[libcamera-devel] [PATCH RFC 2/2] android: CameraHalManager: Create a static object dynamically

[Jacopo Mondi]

Hi Hiro,

On Mon, May 24, 2021 at 08:56:40PM +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>
> ---
>  src/android/camera3_hal.cpp        | 14 +++++++-------
>  src/android/camera_hal_manager.cpp |  7 +++++++
>  src/android/camera_hal_manager.h   |  5 ++++-
>  3 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> index f2d4799f..59d51ed5 100644
> --- a/src/android/camera3_hal.cpp
> +++ b/src/android/camera3_hal.cpp
> @@ -24,25 +24,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;
>  }
> @@ -70,7 +68,7 @@ static int hal_init()
>  {
>  	LOG(HAL, Info) << "Initialising Android camera HAL";
>
> -	cameraManager.init();
> +	CameraHalManager::instance()->init();
>
>  	return 0;
>  }
> @@ -85,7 +83,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 << "'";
> @@ -135,6 +134,7 @@ 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/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index bf3fcda7..387b600d 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'm ok with the introduction of the singleton, but why do we need to
move cros-specific calls to camera3_hal.cpp ?

Can't the singleton instance be deleted by the existing tear-down
function in src/android/cros/camera3_hal.cpp ?

Thanks
   j

>  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 d9bf2798..9b43afc4 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -24,9 +24,10 @@ class CameraDevice;
>  class CameraHalManager
>  {
>  public:
> -	CameraHalManager();
>  	~CameraHalManager();
>
> +	static CameraHalManager *instance();
> +
>  	int init();
>
>  	std::tuple<CameraDevice *, int>
> @@ -42,6 +43,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);
> --
> 2.31.1.818.g46aad6cb9e-goog
>