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

Message ID 20210524115640.2334778-3-hiroh@chromium.org
State Superseded
Headers show
Series
  • Fix SIGSEGV in ChromeOS camera3 test
Related show

Commit Message

Hirokazu Honda May 24, 2021, 11:56 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        | 14 +++++++-------
 src/android/camera_hal_manager.cpp |  7 +++++++
 src/android/camera_hal_manager.h   |  5 ++++-
 3 files changed, 18 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi May 24, 2021, 2:07 p.m. UTC | #1 
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@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
>
Hirokazu Honda May 25, 2021, 2:19 a.m. UTC | #2 
Hi Laurent,

On Mon, May 24, 2021 at 11:06 PM Jacopo Mondi <jacopo@jmondi.org> wrote:

> 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.
>

Self review: s/has/have/


> > 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        | 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 ?
>
>
Sorry, I don't write the reason about that.
I tried including "../camera_hal_manager.h" in
src/android/cros/camera3_hal.cpp.
However, I couldn't because the meson file doesn't contain that in include
paths.
I first thought adding cros/camera3_hal.cpp to android_hal_sources like
cros_camera_buffer.cpp, but rethought it might be better to put the code
of  src/android/cros/camera3_hal.cpp to  src/android/camera3_hal.cpp.
I don't know if it is a good idea. I appreciate any suggestion.

Thanks,
-Hiro


> 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
> >
>

Patch
diff mbox series 

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;
+}
+
 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);