[{"id":15809,"web_url":"https://patchwork.libcamera.org/comment/15809/","msgid":"","date":"2021-03-23T01:51:55","subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Tue, Mar 23, 2021 at 10:42:19AM +0900, Hirokazu Honda wrote:\n> CameraDevice is owned by CameraHalManager. The ownership of the\n> object is not shared with other classes. So CameraHalManager\n> should manage CameraDevice with std::unique_ptr.\n> \n> Signed-off-by: Hirokazu Honda \n> \n> ---\n> src/android/camera_device.cpp | 5 ++---\n> src/android/camera_device.h | 2 +-\n> src/android/camera_hal_manager.cpp | 10 ++++------\n> src/android/camera_hal_manager.h | 2 +-\n> 4 files changed, 8 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 72a89258..d0955de7 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -350,11 +350,10 @@ CameraDevice::~CameraDevice()\n> \t\tdelete it.second;\n> }\n> \n> -std::shared_ptr CameraDevice::create(unsigned int id,\n> +std::unique_ptr CameraDevice::create(unsigned int id,\n> \t\t\t\t\t\t const std::shared_ptr &cam)\n> {\n> -\tCameraDevice *camera = new CameraDevice(id, cam);\n> -\treturn std::shared_ptr(camera);\n> +\treturn std::unique_ptr(new CameraDevice(id, cam));\n\nThis could use\n\n\treturn std::make_unique(id, cam);\n\nReviewed-by: Laurent Pinchart \n\n> }\n> \n> /*\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 823d561c..8be7f305 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -32,7 +32,7 @@\n> class CameraDevice : protected libcamera::Loggable\n> {\n> public:\n> -\tstatic std::shared_ptr create(unsigned int id,\n> +\tstatic std::unique_ptr create(unsigned int id,\n> \t\t\t\t\t\t const std::shared_ptr &cam);\n> \t~CameraDevice();\n> \n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 189eda2b..b3c85406 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -36,8 +36,6 @@ CameraHalManager::CameraHalManager()\n> \n> CameraHalManager::~CameraHalManager()\n> {\n> -\tcameras_.clear();\n> -\n> \tif (cameraManager_) {\n> \t\tcameraManager_->stop();\n> \t\tdelete cameraManager_;\n> @@ -124,7 +122,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t}\n> \n> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> -\tstd::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> +\tstd::unique_ptr camera = CameraDevice::create(id, std::move(cam));\n> \tint ret = camera->initialize();\n> \tif (ret) {\n> \t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> @@ -154,7 +152,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> \tMutexLocker locker(mutex_);\n> \n> \tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> -\t\t\t\t [&cam](std::shared_ptr &camera) {\n> +\t\t\t\t [&cam](const std::unique_ptr &camera) {\n> \t\t\t\t\t return cam == camera->camera();\n> \t\t\t\t });\n> \tif (iter == cameras_.end())\n> @@ -191,7 +189,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n> {\n> \tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> -\t\t\t\t [id](std::shared_ptr &camera) {\n> +\t\t\t\t [id](const std::unique_ptr &camera) {\n> \t\t\t\t\t return camera->id() == id;\n> \t\t\t\t });\n> \tif (iter == cameras_.end())\n> @@ -243,7 +241,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)\n> \t * Internal cameras are already assumed to be present at module load\n> \t * time by the Android framework.\n> \t */\n> -\tfor (std::shared_ptr &camera : cameras_) {\n> +\tfor (const std::unique_ptr &camera : cameras_) {\n> \t\tunsigned int id = camera->id();\n> \t\tif (id >= firstExternalCameraId_)\n> \t\t\tcallbacks_->camera_device_status_change(callbacks_, id,\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index a91decc7..65bb3554 100644\n> --- a/src/android/camera_hal_manager.h\n> +++ b/src/android/camera_hal_manager.h\n> @@ -50,7 +50,7 @@ private:\n> \tlibcamera::CameraManager *cameraManager_;\n> \n> \tconst camera_module_callbacks_t *callbacks_;\n> -\tstd::vector> cameras_;\n> +\tstd::vector> cameras_;\n> \tstd::map cameraIdsMap_;\n> \tMutex mutex_;\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 4E14DC32E1\n\tfor ;\n\tTue, 23 Mar 2021 01:52:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC33668D67;\n\tTue, 23 Mar 2021 02:52:38 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2464B6051B\n\tfor ;\n\tTue, 23 Mar 2021 02:52:37 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80CA8885;\n\tTue, 23 Mar 2021 02:52:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ntMQsbnk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616464356;\n\tbh=L8/JVMoiBYHmtX0oY/ipGCPcORGM9Vo+LJkvFn8kXxc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ntMQsbnksdDwPFp1tm3aTaZFlnZDZJwftz6SW+v7NWgIB+EdLvofHlQuxy5tz1uFZ\n\t+9/mWx9aRJUVQ78KnE8eLFu5z9AXWGL5GWUFUX8QZ0Qos1EWc3Kahl5oeZAM+TwNob\n\tEhlx54I1/UTrXvheP6BPAP6w6VmdH2PRqUZDMVJI=","Date":"Tue, 23 Mar 2021 03:51:55 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210323014226.3211412-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15817,"web_url":"https://patchwork.libcamera.org/comment/15817/","msgid":"","date":"2021-03-23T02:24:11","subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thanks for reviewing.\n\nOn Tue, Mar 23, 2021 at 10:52 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 23, 2021 at 10:42:19AM +0900, Hirokazu Honda wrote:\n> > CameraDevice is owned by CameraHalManager. The ownership of the\n> > object is not shared with other classes. So CameraHalManager\n> > should manage CameraDevice with std::unique_ptr.\n> >\n> > Signed-off-by: Hirokazu Honda \n> >\n> > ---\n> > src/android/camera_device.cpp | 5 ++---\n> > src/android/camera_device.h | 2 +-\n> > src/android/camera_hal_manager.cpp | 10 ++++------\n> > src/android/camera_hal_manager.h | 2 +-\n> > 4 files changed, 8 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 72a89258..d0955de7 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -350,11 +350,10 @@ CameraDevice::~CameraDevice()\n> > delete it.second;\n> > }\n> >\n> > -std::shared_ptr CameraDevice::create(unsigned int id,\n> > +std::unique_ptr CameraDevice::create(unsigned int id,\n> > const std::shared_ptr &cam)\n> > {\n> > - CameraDevice *camera = new CameraDevice(id, cam);\n> > - return std::shared_ptr(camera);\n> > + return std::unique_ptr(new CameraDevice(id, cam));\n>\n> This could use\n>\n> return std::make_unique(id, cam);\n>\n\nstd::make_uniue cannot be used because CameraDevice ctor is private.\nBy the way, chromium is base::WrapUnique for this.\nhttps://source.chromium.org/chromium/chromium/src/+/master:base/memory/ptr_util.h;l=17;drc=0e26b21394de64e30cdeebe308df851129dd5eff\n\nRegards,\n-Hiro\n\n> Reviewed-by: Laurent Pinchart \n>\n> > }\n> >\n> > /*\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 823d561c..8be7f305 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -32,7 +32,7 @@\n> > class CameraDevice : protected libcamera::Loggable\n> > {\n> > public:\n> > - static std::shared_ptr create(unsigned int id,\n> > + static std::unique_ptr create(unsigned int id,\n> > const std::shared_ptr &cam);\n> > ~CameraDevice();\n> >\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > index 189eda2b..b3c85406 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -36,8 +36,6 @@ CameraHalManager::CameraHalManager()\n> >\n> > CameraHalManager::~CameraHalManager()\n> > {\n> > - cameras_.clear();\n> > -\n> > if (cameraManager_) {\n> > cameraManager_->stop();\n> > delete cameraManager_;\n> > @@ -124,7 +122,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > }\n> >\n> > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > - std::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > + std::unique_ptr camera = CameraDevice::create(id, std::move(cam));\n> > int ret = camera->initialize();\n> > if (ret) {\n> > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > @@ -154,7 +152,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> > MutexLocker locker(mutex_);\n> >\n> > auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > - [&cam](std::shared_ptr &camera) {\n> > + [&cam](const std::unique_ptr &camera) {\n> > return cam == camera->camera();\n> > });\n> > if (iter == cameras_.end())\n> > @@ -191,7 +189,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n> > {\n> > auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > - [id](std::shared_ptr &camera) {\n> > + [id](const std::unique_ptr &camera) {\n> > return camera->id() == id;\n> > });\n> > if (iter == cameras_.end())\n> > @@ -243,7 +241,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)\n> > * Internal cameras are already assumed to be present at module load\n> > * time by the Android framework.\n> > */\n> > - for (std::shared_ptr &camera : cameras_) {\n> > + for (const std::unique_ptr &camera : cameras_) {\n> > unsigned int id = camera->id();\n> > if (id >= firstExternalCameraId_)\n> > callbacks_->camera_device_status_change(callbacks_, id,\n> > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > index a91decc7..65bb3554 100644\n> > --- a/src/android/camera_hal_manager.h\n> > +++ b/src/android/camera_hal_manager.h\n> > @@ -50,7 +50,7 @@ private:\n> > libcamera::CameraManager *cameraManager_;\n> >\n> > const camera_module_callbacks_t *callbacks_;\n> > - std::vector> cameras_;\n> > + std::vector> cameras_;\n> > std::map cameraIdsMap_;\n> > Mutex mutex_;\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 06366BD80C\n\tfor ;\n\tTue, 23 Mar 2021 02:24:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7026268D6D;\n\tTue, 23 Mar 2021 03:24:23 +0100 (CET)","from mail-ej1-x632.google.com (mail-ej1-x632.google.com\n\t[IPv6:2a00:1450:4864:20::632])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 092896084F\n\tfor ;\n\tTue, 23 Mar 2021 03:24:22 +0100 (CET)","by mail-ej1-x632.google.com with SMTP id k10so24563623ejg.0\n\tfor ;\n\tMon, 22 Mar 2021 19:24:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"EB68uYRi\"; 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=7CMt+UOSI2XWtrTaCleZnaL5uPHKzgrDeoAaeE9ob3Y=;\n\tb=EB68uYRiOFUUJmFUVohqSwI7kPB6XegoQEdRStmuCjS7QS8uuyGI9hP0TJdxwg1s/K\n\tQukDUKfmUVrWq1Gxj2HgpXRNtJj9xRc08+X/C5CrDrolo6x6vsUIHkSUZX8i07OkkocS\n\togIXWgdXC5XMVX7NKmz2idXbnz43wVPdzIbCs=","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=7CMt+UOSI2XWtrTaCleZnaL5uPHKzgrDeoAaeE9ob3Y=;\n\tb=OpcXtAa1peoyTXxg0mHRjzcMA78ezSyWRzvDemlATSXVocnDajOO6gHH8oVXZwn0ik\n\tTy/rh7otFppks561IPD9YTTxoEmGgs1AJhp2iAtlAx+SjTddTLipsw8rSCwbe6kKVUqe\n\tXY0h7ZcHUuNnJCpI5u2EV2/DCI1CkB1eJd5CnvxQVzjIuk1K0LvMa3bj7sIzzQ0qydGy\n\tnFIfNvP7O6pxewOujNKEDN8Xy5VPH4WZLxrnLS8z6j2QmchWgrOAFmInlZ1VuciYjO1o\n\teFfpz6qSvqOFKy5yjbWc/FdmaZZn79H2aC8p55uqEOJMja2sKoEqPg5oiLYUKbotKhZ8\n\tx3xg==","X-Gm-Message-State":"AOAM532QrFfJTw4XGzfH+Z+4q/tqqv2U9fYiSBf9COQDOQ9Ww6EpUpmn\n\tGliad8wWS+ZdEcJG0hqVqqfFIoUxd8jy2ak4PeGB24lrEHtpVw==","X-Google-Smtp-Source":"ABdhPJzjPyIpvplteZgu9OOPtLWS5sUWsOQzUSH1uaVffhrZ+QkxUTIQscQP57PqmVxUoL4RFwZnruSgrFDDosO0xZk=","X-Received":"by 2002:a17:907:36e:: with SMTP id\n\trs14mr2599236ejb.42.1616466261633; \n\tMon, 22 Mar 2021 19:24:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 23 Mar 2021 11:24:11 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15834,"web_url":"https://patchwork.libcamera.org/comment/15834/","msgid":"","date":"2021-03-23T13:59:34","subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Mar 23, 2021 at 11:24:11AM +0900, Hirokazu Honda wrote:\n> On Tue, Mar 23, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > On Tue, Mar 23, 2021 at 10:42:19AM +0900, Hirokazu Honda wrote:\n> > > CameraDevice is owned by CameraHalManager. The ownership of the\n> > > object is not shared with other classes. So CameraHalManager\n> > > should manage CameraDevice with std::unique_ptr.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > >\n> > > ---\n> > > src/android/camera_device.cpp | 5 ++---\n> > > src/android/camera_device.h | 2 +-\n> > > src/android/camera_hal_manager.cpp | 10 ++++------\n> > > src/android/camera_hal_manager.h | 2 +-\n> > > 4 files changed, 8 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 72a89258..d0955de7 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -350,11 +350,10 @@ CameraDevice::~CameraDevice()\n> > > delete it.second;\n> > > }\n> > >\n> > > -std::shared_ptr CameraDevice::create(unsigned int id,\n> > > +std::unique_ptr CameraDevice::create(unsigned int id,\n> > > const std::shared_ptr &cam)\n> > > {\n> > > - CameraDevice *camera = new CameraDevice(id, cam);\n> > > - return std::shared_ptr(camera);\n> > > + return std::unique_ptr(new CameraDevice(id, cam));\n> >\n> > This could use\n> >\n> > return std::make_unique(id, cam);\n> \n> std::make_uniue cannot be used because CameraDevice ctor is private.\n\nAh, good point. Let's leave it as-is then.\n\n> By the way, chromium is base::WrapUnique for this.\n> https://source.chromium.org/chromium/chromium/src/+/master:base/memory/ptr_util.h;l=17;drc=0e26b21394de64e30cdeebe308df851129dd5eff\n\nDo I understand correctly that it would result in the following change ?\n\n-\treturn std::unique_ptr(new CameraDevice(id, cam));\n+\treturn base::WrapUnique(new CameraDevice(id, cam));\n\nIt's a bit shorter, but possibly not worth creating a specific helper in\nlibcamera given how uncommon private constructors are in our code base.\n\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > }\n> > >\n> > > /*\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 823d561c..8be7f305 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -32,7 +32,7 @@\n> > > class CameraDevice : protected libcamera::Loggable\n> > > {\n> > > public:\n> > > - static std::shared_ptr create(unsigned int id,\n> > > + static std::unique_ptr create(unsigned int id,\n> > > const std::shared_ptr &cam);\n> > > ~CameraDevice();\n> > >\n> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > index 189eda2b..b3c85406 100644\n> > > --- a/src/android/camera_hal_manager.cpp\n> > > +++ b/src/android/camera_hal_manager.cpp\n> > > @@ -36,8 +36,6 @@ CameraHalManager::CameraHalManager()\n> > >\n> > > CameraHalManager::~CameraHalManager()\n> > > {\n> > > - cameras_.clear();\n> > > -\n> > > if (cameraManager_) {\n> > > cameraManager_->stop();\n> > > delete cameraManager_;\n> > > @@ -124,7 +122,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > }\n> > >\n> > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > - std::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > + std::unique_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > int ret = camera->initialize();\n> > > if (ret) {\n> > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > @@ -154,7 +152,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> > > MutexLocker locker(mutex_);\n> > >\n> > > auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > > - [&cam](std::shared_ptr &camera) {\n> > > + [&cam](const std::unique_ptr &camera) {\n> > > return cam == camera->camera();\n> > > });\n> > > if (iter == cameras_.end())\n> > > @@ -191,7 +189,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> > > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n> > > {\n> > > auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > > - [id](std::shared_ptr &camera) {\n> > > + [id](const std::unique_ptr &camera) {\n> > > return camera->id() == id;\n> > > });\n> > > if (iter == cameras_.end())\n> > > @@ -243,7 +241,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)\n> > > * Internal cameras are already assumed to be present at module load\n> > > * time by the Android framework.\n> > > */\n> > > - for (std::shared_ptr &camera : cameras_) {\n> > > + for (const std::unique_ptr &camera : cameras_) {\n> > > unsigned int id = camera->id();\n> > > if (id >= firstExternalCameraId_)\n> > > callbacks_->camera_device_status_change(callbacks_, id,\n> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > index a91decc7..65bb3554 100644\n> > > --- a/src/android/camera_hal_manager.h\n> > > +++ b/src/android/camera_hal_manager.h\n> > > @@ -50,7 +50,7 @@ private:\n> > > libcamera::CameraManager *cameraManager_;\n> > >\n> > > const camera_module_callbacks_t *callbacks_;\n> > > - std::vector> cameras_;\n> > > + std::vector> cameras_;\n> > > std::map cameraIdsMap_;\n> > > Mutex mutex_;\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 6F983C32E4\n\tfor ;\n\tTue, 23 Mar 2021 14:00:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9DEC68D63;\n\tTue, 23 Mar 2021 15:00:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3BD37602D7\n\tfor ;\n\tTue, 23 Mar 2021 15:00:17 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A980885;\n\tTue, 23 Mar 2021 15:00:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p5DTNClN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616508016;\n\tbh=8KavbtXLc9kkSAQSe88VNrju6xOflRUCoAqWztWSilQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p5DTNClNZgRatkAVkMUNV0ycOxu526ql7pADeIM/97CtvWtZTe4eRJJ5hSaszXska\n\tfocTiM4ivgm8HTGbYX4e4s6pQuqM0KfZbwsGoABQPGoHnxAqa9EabPq5y/6aKPJKwh\n\th0wGkq+2REJ08QBWfLBYMG6p7BEnofo3TSFdNHfY=","Date":"Tue, 23 Mar 2021 15:59:34 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-2-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15849,"web_url":"https://patchwork.libcamera.org/comment/15849/","msgid":"","date":"2021-03-24T06:36:40","subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Mar 23, 2021 at 11:00 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Mar 23, 2021 at 11:24:11AM +0900, Hirokazu Honda wrote:\n> > On Tue, Mar 23, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > > On Tue, Mar 23, 2021 at 10:42:19AM +0900, Hirokazu Honda wrote:\n> > > > CameraDevice is owned by CameraHalManager. The ownership of the\n> > > > object is not shared with other classes. So CameraHalManager\n> > > > should manage CameraDevice with std::unique_ptr.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > >\n> > > > ---\n> > > > src/android/camera_device.cpp | 5 ++---\n> > > > src/android/camera_device.h | 2 +-\n> > > > src/android/camera_hal_manager.cpp | 10 ++++------\n> > > > src/android/camera_hal_manager.h | 2 +-\n> > > > 4 files changed, 8 insertions(+), 11 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 72a89258..d0955de7 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -350,11 +350,10 @@ CameraDevice::~CameraDevice()\n> > > > delete it.second;\n> > > > }\n> > > >\n> > > > -std::shared_ptr CameraDevice::create(unsigned int id,\n> > > > +std::unique_ptr CameraDevice::create(unsigned int id,\n> > > > const std::shared_ptr &cam)\n> > > > {\n> > > > - CameraDevice *camera = new CameraDevice(id, cam);\n> > > > - return std::shared_ptr(camera);\n> > > > + return std::unique_ptr(new CameraDevice(id, cam));\n> > >\n> > > This could use\n> > >\n> > > return std::make_unique(id, cam);\n> >\n> > std::make_uniue cannot be used because CameraDevice ctor is private.\n>\n> Ah, good point. Let's leave it as-is then.\n>\n> > By the way, chromium is base::WrapUnique for this.\n> > https://source.chromium.org/chromium/chromium/src/+/master:base/memory/ptr_util.h;l=17;drc=0e26b21394de64e30cdeebe308df851129dd5eff\n>\n> Do I understand correctly that it would result in the following change ?\n>\n> - return std::unique_ptr(new CameraDevice(id, cam));\n> + return base::WrapUnique(new CameraDevice(id, cam));\n>\n> It's a bit shorter, but possibly not worth creating a specific helper in\n> libcamera given how uncommon private constructors are in our code base.\n>\n\nRight. I agree with you.\n-Hiro\n\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > > }\n> > > >\n> > > > /*\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index 823d561c..8be7f305 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -32,7 +32,7 @@\n> > > > class CameraDevice : protected libcamera::Loggable\n> > > > {\n> > > > public:\n> > > > - static std::shared_ptr create(unsigned int id,\n> > > > + static std::unique_ptr create(unsigned int id,\n> > > > const std::shared_ptr &cam);\n> > > > ~CameraDevice();\n> > > >\n> > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > index 189eda2b..b3c85406 100644\n> > > > --- a/src/android/camera_hal_manager.cpp\n> > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > @@ -36,8 +36,6 @@ CameraHalManager::CameraHalManager()\n> > > >\n> > > > CameraHalManager::~CameraHalManager()\n> > > > {\n> > > > - cameras_.clear();\n> > > > -\n> > > > if (cameraManager_) {\n> > > > cameraManager_->stop();\n> > > > delete cameraManager_;\n> > > > @@ -124,7 +122,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > }\n> > > >\n> > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > - std::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > > + std::unique_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > > int ret = camera->initialize();\n> > > > if (ret) {\n> > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > @@ -154,7 +152,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> > > > MutexLocker locker(mutex_);\n> > > >\n> > > > auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > > > - [&cam](std::shared_ptr &camera) {\n> > > > + [&cam](const std::unique_ptr &camera) {\n> > > > return cam == camera->camera();\n> > > > });\n> > > > if (iter == cameras_.end())\n> > > > @@ -191,7 +189,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> > > > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n> > > > {\n> > > > auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > > > - [id](std::shared_ptr &camera) {\n> > > > + [id](const std::unique_ptr &camera) {\n> > > > return camera->id() == id;\n> > > > });\n> > > > if (iter == cameras_.end())\n> > > > @@ -243,7 +241,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)\n> > > > * Internal cameras are already assumed to be present at module load\n> > > > * time by the Android framework.\n> > > > */\n> > > > - for (std::shared_ptr &camera : cameras_) {\n> > > > + for (const std::unique_ptr &camera : cameras_) {\n> > > > unsigned int id = camera->id();\n> > > > if (id >= firstExternalCameraId_)\n> > > > callbacks_->camera_device_status_change(callbacks_, id,\n> > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > index a91decc7..65bb3554 100644\n> > > > --- a/src/android/camera_hal_manager.h\n> > > > +++ b/src/android/camera_hal_manager.h\n> > > > @@ -50,7 +50,7 @@ private:\n> > > > libcamera::CameraManager *cameraManager_;\n> > > >\n> > > > const camera_module_callbacks_t *callbacks_;\n> > > > - std::vector> cameras_;\n> > > > + std::vector> cameras_;\n> > > > std::map cameraIdsMap_;\n> > > > Mutex mutex_;\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 B69C6C32E4\n\tfor ;\n\tWed, 24 Mar 2021 06:36:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16C5468D68;\n\tWed, 24 Mar 2021 07:36:52 +0100 (CET)","from mail-ej1-x629.google.com (mail-ej1-x629.google.com\n\t[IPv6:2a00:1450:4864:20::629])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7DD5268D47\n\tfor ;\n\tWed, 24 Mar 2021 07:36:50 +0100 (CET)","by mail-ej1-x629.google.com with SMTP id u9so31001663ejj.7\n\tfor ;\n\tTue, 23 Mar 2021 23:36:50 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"JiigXy/N\"; 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=1b6dC7oGSE5RETptK3yRse13WG45JtCF589O8RCqN74=;\n\tb=JiigXy/NrqPMpdHnrQmbCx7pZwIdBX+vgUoJJD7bxsWbY/QQ99DgU8d7MmMPy7dij+\n\txQDesX2W27sZDGkcEV0hWnfgc0StEbsmxNQM9KjeNeS27Jnt50JCwCbTbl7eZOuWyb2V\n\tYBE0RewDYHbWLePUr5azhUHsVujGwu9SJGqEM=","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=1b6dC7oGSE5RETptK3yRse13WG45JtCF589O8RCqN74=;\n\tb=fm29wHhAsU1zHQqO1Q17JBwkXOG++XXbEoU7UGxA9kfi4xpwbNjOijusH+Xe6GXI/5\n\tqtGekXPSPvvk4gHktoHZ4k0GtEJAi9cMecihHsF/gjDU2vW2hbdFMvys6sg0gi85HQWE\n\tM6J+ZQZ5FRYMITJuDC5CnBL2pxMMxKakdV6F9fr9LtxMyfaykBTPSz/A52YwjrbG7/CQ\n\tTyzPQLXBkh44GXAt0iRHqLTj7ATiodQEmVHTE9pPrrmv23/jHUx1gLV5sp7eLEFD+o/t\n\tQmJtBpUo7ssBmR2yVVCI9L9GkSKMMMHXWWv+oVwhvSbs+9LKV+vDaGzFoKJHcbUMu+qb\n\tZzhg==","X-Gm-Message-State":"AOAM530+EH3AAdAumlNC2INv55fT42+JZE3VbNqZ4QTsZljjAApaK/8Y\n\ts4qO10H7HdElLph0lGcj6C0MOxibnxWeG+vNf4SF4EkHkDA=","X-Google-Smtp-Source":"ABdhPJwsBpXJHW52XQW4apiEKS65T7V29e7jApJait8qgMGybLzllenU5CgfuP2giwnS7g7O1Kbss+txgzPcxpj7Zws=","X-Received":"by 2002:a17:906:b015:: with SMTP id\n\tv21mr1903986ejy.243.1616567810034; \n\tTue, 23 Mar 2021 23:36:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-2-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 24 Mar 2021 15:36:40 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold\n\tCameraDevice with std::unique_ptr","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]