[{"id":17210,"web_url":"https://patchwork.libcamera.org/comment/17210/","msgid":"<20210524140709.fcow5qi76balt5sc@uno.localdomain>","date":"2021-05-24T14:07:09","subject":"Re: [libcamera-devel] [PATCH RFC 2/2] android: CameraHalManager:\n\tCreate a static object dynamically","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, May 24, 2021 at 08:56:40PM +0900, Hirokazu Honda wrote:\n> Originally CameraHalManager is created in the libcamera start up\n> and destroyed in the libcamera termination. However,\n> CameraHalManager destructor can access other static objects that\n> has been destroyed.\n> Avoid this issue by destroying CameraHalManager when tear_down() is\n> called in ChromeOS or leaking it in other platforms.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera3_hal.cpp | 14 +++++++-------\n> src/android/camera_hal_manager.cpp | 7 +++++++\n> src/android/camera_hal_manager.h | 5 ++++-\n> 3 files changed, 18 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp\n> index f2d4799f..59d51ed5 100644\n> --- a/src/android/camera3_hal.cpp\n> +++ b/src/android/camera3_hal.cpp\n> @@ -24,25 +24,23 @@ using namespace libcamera;\n>\n> LOG_DEFINE_CATEGORY(HAL)\n>\n> -static CameraHalManager cameraManager;\n> -\n> /*------------------------------------------------------------------------------\n> * Android Camera HAL callbacks\n> */\n>\n> static int hal_get_number_of_cameras()\n> {\n> -\treturn cameraManager.numCameras();\n> +\treturn CameraHalManager::instance()->numCameras();\n> }\n>\n> static int hal_get_camera_info(int id, struct camera_info *info)\n> {\n> -\treturn cameraManager.getCameraInfo(id, info);\n> +\treturn CameraHalManager::instance()->getCameraInfo(id, info);\n> }\n>\n> static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)\n> {\n> -\tcameraManager.setCallbacks(callbacks);\n> +\tCameraHalManager::instance()->setCallbacks(callbacks);\n>\n> \treturn 0;\n> }\n> @@ -70,7 +68,7 @@ static int hal_init()\n> {\n> \tLOG(HAL, Info) << \"Initialising Android camera HAL\";\n>\n> -\tcameraManager.init();\n> +\tCameraHalManager::instance()->init();\n>\n> \treturn 0;\n> }\n> @@ -85,7 +83,8 @@ static int hal_dev_open(const hw_module_t *module, const char *name,\n> \tLOG(HAL, Debug) << \"Open camera \" << name;\n>\n> \tint id = atoi(name);\n> -\tauto [camera, ret] = cameraManager.open(id, module);\n> +\n> +\tauto [camera, ret] = CameraHalManager::instance()->open(id, module);\n> \tif (!camera) {\n> \t\tLOG(HAL, Error)\n> \t\t\t<< \"Failed to open camera module '\" << id << \"'\";\n> @@ -135,6 +134,7 @@ static void set_up(cros::CameraMojoChannelManagerToken *token)\n>\n> static void tear_down()\n> {\n> +\tdelete CameraHalManager::instance();\n> }\n>\n> cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index bf3fcda7..387b600d 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -37,6 +37,13 @@ CameraHalManager::CameraHalManager()\n> /* CameraManager calls stop() in the destructor. */\n> CameraHalManager::~CameraHalManager() = default;\n>\n> +/* static */\n> +CameraHalManager *CameraHalManager::instance()\n> +{\n> +\tstatic CameraHalManager *cameraHalManager = new CameraHalManager;\n> +\treturn cameraHalManager;\n> +}\n> +\n\nI'm ok with the introduction of the singleton, but why do we need to\nmove cros-specific calls to camera3_hal.cpp ?\n\nCan't the singleton instance be deleted by the existing tear-down\nfunction in src/android/cros/camera3_hal.cpp ?\n\nThanks\n j\n\n> int CameraHalManager::init()\n> {\n> \tcameraManager_ = std::make_unique();\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index d9bf2798..9b43afc4 100644\n> --- a/src/android/camera_hal_manager.h\n> +++ b/src/android/camera_hal_manager.h\n> @@ -24,9 +24,10 @@ class CameraDevice;\n> class CameraHalManager\n> {\n> public:\n> -\tCameraHalManager();\n> \t~CameraHalManager();\n>\n> +\tstatic CameraHalManager *instance();\n> +\n> \tint init();\n>\n> \tstd::tuple\n> @@ -42,6 +43,8 @@ private:\n>\n> \tstatic constexpr unsigned int firstExternalCameraId_ = 1000;\n>\n> +\tCameraHalManager();\n> +\n> \tstatic int32_t cameraLocation(const libcamera::Camera *cam);\n>\n> \tvoid cameraAdded(std::shared_ptr cam);\n> --\n> 2.31.1.818.g46aad6cb9e-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4D4F7C3200\n\tfor ;\n\tMon, 24 May 2021 14:06:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B06BB602B0;\n\tMon, 24 May 2021 16:06:25 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D1E4601AA\n\tfor ;\n\tMon, 24 May 2021 16:06:24 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id B74D1240007;\n\tMon, 24 May 2021 14:06:23 +0000 (UTC)"],"Date":"Mon, 24 May 2021 16:07:09 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210524140709.fcow5qi76balt5sc@uno.localdomain>","References":"<20210524115640.2334778-1-hiroh@chromium.org>\n\t<20210524115640.2334778-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210524115640.2334778-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH RFC 2/2] android: CameraHalManager:\n\tCreate a static object dynamically","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17221,"web_url":"https://patchwork.libcamera.org/comment/17221/","msgid":"","date":"2021-05-25T02:19:01","subject":"Re: [libcamera-devel] [PATCH RFC 2/2] android: CameraHalManager:\n\tCreate a static object dynamically","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, May 24, 2021 at 11:06 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Mon, May 24, 2021 at 08:56:40PM +0900, Hirokazu Honda wrote:\n> > Originally CameraHalManager is created in the libcamera start up\n> > and destroyed in the libcamera termination. However,\n> > CameraHalManager destructor can access other static objects that\n> > has been destroyed.\n>\n\nSelf review: s/has/have/\n\n\n> > Avoid this issue by destroying CameraHalManager when tear_down() is\n> > called in ChromeOS or leaking it in other platforms.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera3_hal.cpp | 14 +++++++-------\n> > src/android/camera_hal_manager.cpp | 7 +++++++\n> > src/android/camera_hal_manager.h | 5 ++++-\n> > 3 files changed, 18 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp\n> > index f2d4799f..59d51ed5 100644\n> > --- a/src/android/camera3_hal.cpp\n> > +++ b/src/android/camera3_hal.cpp\n> > @@ -24,25 +24,23 @@ using namespace libcamera;\n> >\n> > LOG_DEFINE_CATEGORY(HAL)\n> >\n> > -static CameraHalManager cameraManager;\n> > -\n> >\n> /*------------------------------------------------------------------------------\n> > * Android Camera HAL callbacks\n> > */\n> >\n> > static int hal_get_number_of_cameras()\n> > {\n> > - return cameraManager.numCameras();\n> > + return CameraHalManager::instance()->numCameras();\n> > }\n> >\n> > static int hal_get_camera_info(int id, struct camera_info *info)\n> > {\n> > - return cameraManager.getCameraInfo(id, info);\n> > + return CameraHalManager::instance()->getCameraInfo(id, info);\n> > }\n> >\n> > static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)\n> > {\n> > - cameraManager.setCallbacks(callbacks);\n> > + CameraHalManager::instance()->setCallbacks(callbacks);\n> >\n> > return 0;\n> > }\n> > @@ -70,7 +68,7 @@ static int hal_init()\n> > {\n> > LOG(HAL, Info) << \"Initialising Android camera HAL\";\n> >\n> > - cameraManager.init();\n> > + CameraHalManager::instance()->init();\n> >\n> > return 0;\n> > }\n> > @@ -85,7 +83,8 @@ static int hal_dev_open(const hw_module_t *module,\n> const char *name,\n> > LOG(HAL, Debug) << \"Open camera \" << name;\n> >\n> > int id = atoi(name);\n> > - auto [camera, ret] = cameraManager.open(id, module);\n> > +\n> > + auto [camera, ret] = CameraHalManager::instance()->open(id,\n> module);\n> > if (!camera) {\n> > LOG(HAL, Error)\n> > << \"Failed to open camera module '\" << id << \"'\";\n> > @@ -135,6 +134,7 @@ static void\n> set_up(cros::CameraMojoChannelManagerToken *token)\n> >\n> > static void tear_down()\n> > {\n> > + delete CameraHalManager::instance();\n> > }\n> >\n> > cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {\n> > diff --git a/src/android/camera_hal_manager.cpp\n> b/src/android/camera_hal_manager.cpp\n> > index bf3fcda7..387b600d 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -37,6 +37,13 @@ CameraHalManager::CameraHalManager()\n> > /* CameraManager calls stop() in the destructor. */\n> > CameraHalManager::~CameraHalManager() = default;\n> >\n> > +/* static */\n> > +CameraHalManager *CameraHalManager::instance()\n> > +{\n> > + static CameraHalManager *cameraHalManager = new CameraHalManager;\n> > + return cameraHalManager;\n> > +}\n> > +\n>\n> I'm ok with the introduction of the singleton, but why do we need to\n> move cros-specific calls to camera3_hal.cpp ?\n>\n> Can't the singleton instance be deleted by the existing tear-down\n> function in src/android/cros/camera3_hal.cpp ?\n>\n>\nSorry, I don't write the reason about that.\nI tried including \"../camera_hal_manager.h\" in\nsrc/android/cros/camera3_hal.cpp.\nHowever, I couldn't because the meson file doesn't contain that in include\npaths.\nI first thought adding cros/camera3_hal.cpp to android_hal_sources like\ncros_camera_buffer.cpp, but rethought it might be better to put the code\nof src/android/cros/camera3_hal.cpp to src/android/camera3_hal.cpp.\nI don't know if it is a good idea. I appreciate any suggestion.\n\nThanks,\n-Hiro\n\n\n> Thanks\n> j\n>\n> > int CameraHalManager::init()\n> > {\n> > cameraManager_ = std::make_unique();\n> > diff --git a/src/android/camera_hal_manager.h\n> b/src/android/camera_hal_manager.h\n> > index d9bf2798..9b43afc4 100644\n> > --- a/src/android/camera_hal_manager.h\n> > +++ b/src/android/camera_hal_manager.h\n> > @@ -24,9 +24,10 @@ class CameraDevice;\n> > class CameraHalManager\n> > {\n> > public:\n> > - CameraHalManager();\n> > ~CameraHalManager();\n> >\n> > + static CameraHalManager *instance();\n> > +\n> > int init();\n> >\n> > std::tuple\n> > @@ -42,6 +43,8 @@ private:\n> >\n> > static constexpr unsigned int firstExternalCameraId_ = 1000;\n> >\n> > + CameraHalManager();\n> > +\n> > static int32_t cameraLocation(const libcamera::Camera *cam);\n> >\n> > void cameraAdded(std::shared_ptr cam);\n> > --\n> > 2.31.1.818.g46aad6cb9e-goog\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 489CBC3201\n\tfor ;\n\tTue, 25 May 2021 02:19:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A774A68919;\n\tTue, 25 May 2021 04:19:13 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D307601A9\n\tfor ;\n\tTue, 25 May 2021 04:19:12 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id a25so34236463edr.12\n\tfor ;\n\tMon, 24 May 2021 19:19:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"TMeWNAjr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=VKS+qRly7MALRGKQ3yoENrm5Vz5p/ynarDzYesuVdD0=;\n\tb=TMeWNAjrK0ZKGzOJFhgxjHFDVvDLpzYCObzz/tQpxrhlemCtbeMghqWzfYfLN5O5xu\n\tmrcjhOkNVmYSszKLKOXxj0rkUYAciuFf0uvyOe9AS1PSuFPkQH2fKrTv7PdpInmc7YxF\n\tQPMb3oQcCDEZvs4ZW6VO/pmEkdyE0R0Hc6OUs=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=VKS+qRly7MALRGKQ3yoENrm5Vz5p/ynarDzYesuVdD0=;\n\tb=UE4a+NZq8GzRsP8DUqfCu+lDa6xxSFIS9loNrzsuWgZauQ8J/lZLxQN/pudSh91zWP\n\tbyuxdiJu4hictYbovbJuXQYxkDFhoa1HPJqBL6t5MoqLffqh44iSDnYqbNIdefFGd1Ay\n\ty1k96fZUSRJaFMsRRAhu5vZySKqy1zqkbWVr/Rx31pQGCbSpvj/gccyCy0N1kG6IjWYn\n\t++QaUhyFaSDy13lcqSZOns+1xyMrqN4/mQH9TmVdyGlATOYUJMrf3D1odFAbGJH0cD+u\n\tJSw3aCOjuuwTUzeKPlQTyyp2MMOWvn0skL/O5zHHuFWh/lRTcLkNuowZM/czACPJvp2e\n\teJEw==","X-Gm-Message-State":"AOAM531dggWnLl+MdoPW/6OP2lKJY7icGgWcoRwQe1x3qO491ly79sbK\n\tb9aHjvj66fdlrTGeDRyLn8kFNUXvU7BFoT3ay48FbQ==","X-Google-Smtp-Source":"ABdhPJx20W+8MWT29xtIAHlQh0x/bctZdZ+o38NOOGMDgingmvIemB4RlXLT6kV/SjQu2joqg0nlm3Zs2sv7JxFB24s=","X-Received":"by 2002:a05:6402:2547:: with SMTP id\n\tl7mr29449902edb.73.1621909151888; \n\tMon, 24 May 2021 19:19:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20210524115640.2334778-1-hiroh@chromium.org>\n\t<20210524115640.2334778-3-hiroh@chromium.org>\n\t<20210524140709.fcow5qi76balt5sc@uno.localdomain>","In-Reply-To":"<20210524140709.fcow5qi76balt5sc@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 25 May 2021 11:19:01 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000db480f05c31e26f2\"","Subject":"Re: [libcamera-devel] [PATCH RFC 2/2] android: CameraHalManager:\n\tCreate a static object dynamically","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]