<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 24, 2021 at 11:06 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Mon, May 24, 2021 at 08:56:40PM +0900, Hirokazu Honda wrote:<br>
> Originally CameraHalManager is created in the libcamera start up<br>
> and destroyed in the libcamera termination. However,<br>
> CameraHalManager destructor can access other static objects that<br>
> has been destroyed.<br></blockquote><div><br></div><div>Self review: s/has/have/</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Avoid this issue by destroying CameraHalManager when tear_down() is<br>
> called in ChromeOS or leaking it in other platforms.<br>
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> src/android/camera3_hal.cpp | 14 +++++++-------<br>
> src/android/camera_hal_manager.cpp | 7 +++++++<br>
> src/android/camera_hal_manager.h | 5 ++++-<br>
> 3 files changed, 18 insertions(+), 8 deletions(-)<br>
><br>
> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp<br>
> index f2d4799f..59d51ed5 100644<br>
> --- a/src/android/camera3_hal.cpp<br>
> +++ b/src/android/camera3_hal.cpp<br>
> @@ -24,25 +24,23 @@ using namespace libcamera;<br>
><br>
> LOG_DEFINE_CATEGORY(HAL)<br>
><br>
> -static CameraHalManager cameraManager;<br>
> -<br>
> /*------------------------------------------------------------------------------<br>
> * Android Camera HAL callbacks<br>
> */<br>
><br>
> static int hal_get_number_of_cameras()<br>
> {<br>
> - return cameraManager.numCameras();<br>
> + return CameraHalManager::instance()->numCameras();<br>
> }<br>
><br>
> static int hal_get_camera_info(int id, struct camera_info *info)<br>
> {<br>
> - return cameraManager.getCameraInfo(id, info);<br>
> + return CameraHalManager::instance()->getCameraInfo(id, info);<br>
> }<br>
><br>
> static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)<br>
> {<br>
> - cameraManager.setCallbacks(callbacks);<br>
> + CameraHalManager::instance()->setCallbacks(callbacks);<br>
><br>
> return 0;<br>
> }<br>
> @@ -70,7 +68,7 @@ static int hal_init()<br>
> {<br>
> LOG(HAL, Info) << "Initialising Android camera HAL";<br>
><br>
> - cameraManager.init();<br>
> + CameraHalManager::instance()->init();<br>
><br>
> return 0;<br>
> }<br>
> @@ -85,7 +83,8 @@ static int hal_dev_open(const hw_module_t *module, const char *name,<br>
> LOG(HAL, Debug) << "Open camera " << name;<br>
><br>
> int id = atoi(name);<br>
> - auto [camera, ret] = cameraManager.open(id, module);<br>
> +<br>
> + auto [camera, ret] = CameraHalManager::instance()->open(id, module);<br>
> if (!camera) {<br>
> LOG(HAL, Error)<br>
> << "Failed to open camera module '" << id << "'";<br>
> @@ -135,6 +134,7 @@ static void set_up(cros::CameraMojoChannelManagerToken *token)<br>
><br>
> static void tear_down()<br>
> {<br>
> + delete CameraHalManager::instance();<br>
> }<br>
><br>
> cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {<br>
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp<br>
> index bf3fcda7..387b600d 100644<br>
> --- a/src/android/camera_hal_manager.cpp<br>
> +++ b/src/android/camera_hal_manager.cpp<br>
> @@ -37,6 +37,13 @@ CameraHalManager::CameraHalManager()<br>
> /* CameraManager calls stop() in the destructor. */<br>
> CameraHalManager::~CameraHalManager() = default;<br>
><br>
> +/* static */<br>
> +CameraHalManager *CameraHalManager::instance()<br>
> +{<br>
> + static CameraHalManager *cameraHalManager = new CameraHalManager;<br>
> + return cameraHalManager;<br>
> +}<br>
> +<br>
<br>
I'm ok with the introduction of the singleton, but why do we need to<br>
move cros-specific calls to camera3_hal.cpp ?<br>
<br>
Can't the singleton instance be deleted by the existing tear-down<br>
function in src/android/cros/camera3_hal.cpp ?<br>
<br></blockquote><div><br></div><div>Sorry, I don't write the reason about that.</div><div>I tried including "../camera_hal_manager.h" in src/android/cros/camera3_hal.cpp.</div><div>However, I couldn't because the meson file doesn't contain that in include paths.</div><div>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.</div><div>I don't know if it is a good idea. I appreciate any suggestion.</div><div><br></div><div>Thanks,</div><div>-Hiro</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
j<br>
<br>
> int CameraHalManager::init()<br>
> {<br>
> cameraManager_ = std::make_unique<CameraManager>();<br>
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h<br>
> index d9bf2798..9b43afc4 100644<br>
> --- a/src/android/camera_hal_manager.h<br>
> +++ b/src/android/camera_hal_manager.h<br>
> @@ -24,9 +24,10 @@ class CameraDevice;<br>
> class CameraHalManager<br>
> {<br>
> public:<br>
> - CameraHalManager();<br>
> ~CameraHalManager();<br>
><br>
> + static CameraHalManager *instance();<br>
> +<br>
> int init();<br>
><br>
> std::tuple<CameraDevice *, int><br>
> @@ -42,6 +43,8 @@ private:<br>
><br>
> static constexpr unsigned int firstExternalCameraId_ = 1000;<br>
><br>
> + CameraHalManager();<br>
> +<br>
> static int32_t cameraLocation(const libcamera::Camera *cam);<br>
><br>
> void cameraAdded(std::shared_ptr<libcamera::Camera> cam);<br>
> --<br>
> 2.31.1.818.g46aad6cb9e-goog<br>
><br>
</blockquote></div></div>