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

[Hirokazu Honda]

Hi Laurent,

On Mon, May 24, 2021 at 11:06 PM Jacopo Mondi <jacopo at 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 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 ?
>
>
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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210525/1302ca0a/attachment.htm>