[{"id":15811,"web_url":"https://patchwork.libcamera.org/comment/15811/","msgid":"<YFlMaQsK3d4BABqR@pendragon.ideasonboard.com>","date":"2021-03-23T02:03:21","subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","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:21AM +0900, Hirokazu Honda wrote:\n> This fixes a bug of calling Camera::id() of a moved Camera.\n\nIndeed, good catch. I however wonder if it wouldn't be simpler to just\ndrop the move, as cam is a shared pointer ?\n\n-\tstd::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));\n+\tstd::shared_ptr<CameraDevice> camera = CameraDevice::create(id, cam);\n\n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> \n> ---\n>  src/android/camera_hal_manager.cpp | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index fa398fea..df3f1cd2 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -95,7 +95,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n>  \t * IDs starts from '0' for internal cameras and '1000' for external\n>  \t * cameras.\n>  \t */\n> -\tauto iter = cameraIdsMap_.find(cam->id());\n> +\tstd::string cameraId = cam->id();\n> +\tauto iter = cameraIdsMap_.find(cameraId);\n>  \tif (iter != cameraIdsMap_.end()) {\n>  \t\tid = iter->second;\n>  \t} else {\n> @@ -117,12 +118,12 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n>  \tstd::unique_ptr<CameraDevice> 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> +\t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cameraId;\n>  \t\treturn;\n>  \t}\n> \n>  \tif (isCameraNew) {\n> -\t\tcameraIdsMap_.emplace(cam->id(), id);\n> +\t\tcameraIdsMap_.emplace(cameraId, id);\n> \n>  \t\tif (isCameraExternal)\n>  \t\t\tnextExternalCameraId_++;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 ED60ABD80C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 23 Mar 2021 02:04:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5355368D66;\n\tTue, 23 Mar 2021 03:04:04 +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 6F77A6051B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 23 Mar 2021 03:04:03 +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 A3602885;\n\tTue, 23 Mar 2021 03:04:02 +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=\"cBMXrhVI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616465043;\n\tbh=yvXWjF/EpoeeKd4m6M1DxQ6L6XqyanCFtiMNlO1Ccvs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cBMXrhVISFSoU7kE37M/PPw/lnw6QphO2Jd3XYHf/62vCvkq9mg7LWxkS3yff83Qe\n\tYXYJEDOhf3VtLVlaJbtLvOWjIaCo+x2ViRjyGr0V0A4RxLS/KsjDjVQzPpWZ65G3CG\n\tw8Be3Z5FNCDI/A3Y+9YWCd2gnQLnIgyQ5fekWiQE=","Date":"Tue, 23 Mar 2021 04:03:21 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YFlMaQsK3d4BABqR@pendragon.ideasonboard.com>","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210323014226.3211412-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15819,"web_url":"https://patchwork.libcamera.org/comment/15819/","msgid":"<CAO5uPHO33NOrMgthxaK4LtTiWgBfR7q5pvpdhNwJ0xqKcH+bCw@mail.gmail.com>","date":"2021-03-23T02:33:40","subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Mar 23, 2021 at 11:04 AM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 23, 2021 at 10:42:21AM +0900, Hirokazu Honda wrote:\n> > This fixes a bug of calling Camera::id() of a moved Camera.\n>\n> Indeed, good catch. I however wonder if it wouldn't be simpler to just\n> drop the move, as cam is a shared pointer ?\n>\n\nThis might be related to the next patch.\nI would pass either by copy or std::move().\nI pass by copy only if a caller still needs the object, otherwise pass\nby std::move().\nIn this case, we don't need the object's resource, but only the string.\nSo storing it before it enables us to pass cam by std::move().\nI would rather do this because of this reason.\nHow do you think?\n\nThis shared_ptr strategy is come from chromium c++ style guide, where\nscoped_refptr is almost equivalent to shared_ptr.\nhttps://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#object-ownership-and-calling-conventions\n\nBest Regards,\n-Hiro\n\n> -       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));\n> +       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, cam);\n>\n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> >\n> > ---\n> >  src/android/camera_hal_manager.cpp | 7 ++++---\n> >  1 file changed, 4 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > index fa398fea..df3f1cd2 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -95,7 +95,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n> >        * IDs starts from '0' for internal cameras and '1000' for external\n> >        * cameras.\n> >        */\n> > -     auto iter = cameraIdsMap_.find(cam->id());\n> > +     std::string cameraId = cam->id();\n> > +     auto iter = cameraIdsMap_.find(cameraId);\n> >       if (iter != cameraIdsMap_.end()) {\n> >               id = iter->second;\n> >       } else {\n> > @@ -117,12 +118,12 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n> >       std::unique_ptr<CameraDevice> 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> > +             LOG(HAL, Error) << \"Failed to initialize camera: \" << cameraId;\n> >               return;\n> >       }\n> >\n> >       if (isCameraNew) {\n> > -             cameraIdsMap_.emplace(cam->id(), id);\n> > +             cameraIdsMap_.emplace(cameraId, id);\n> >\n> >               if (isCameraExternal)\n> >                       nextExternalCameraId_++;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 B6BDDC32E1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 23 Mar 2021 02:33:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 284196051B;\n\tTue, 23 Mar 2021 03:33:51 +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 DCBF66051B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 23 Mar 2021 03:33:49 +0100 (CET)","by mail-ej1-x632.google.com with SMTP id w3so24573493ejc.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 22 Mar 2021 19:33:49 -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=\"RVesiUwc\"; 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=hiRpOJcsAN+0Mc1OuDqlMWsLpalbyzfI7DbDtLE66hw=;\n\tb=RVesiUwclLzNbDygghWO6GwxhNR1B68d0PSqWzuCD8a4lzwASl0jHrYcuca3CXPgA+\n\tY+0x/umpznMXCh7K1GMcjbFGfYaL4TrIJhfBS/q6PjbtpsfSoK9QOdRk/mtiibDGEbS4\n\tbkuo4fgrtKyWccQjN9MCub6QlDcy07RzT8XYY=","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=hiRpOJcsAN+0Mc1OuDqlMWsLpalbyzfI7DbDtLE66hw=;\n\tb=gfgzSnegfjSlPOa7VRA0/qx7wHpJ34vi/Yy/GpKu8bwzFqIeokxZmXKS+EjLuPX1M0\n\tzwP5haJTI6V9kzLxsXOJ0QIUL+j4KUxLcHGgpBpY0aaUxAtESEicmHtPxAtPUEnYkURD\n\tY62HyNAgZsm3aKNKVwRAqojYIyHNwSrYzoj8qSaKFgklaK01clMuvW1ZiKR5WkabK/pA\n\tcevVnVLVx1GD5nSpadqTkyH19KmjL2OOTAi9tGxx8q7yDxrSSV2MWJCMv+Fu2V3E+AOM\n\tM8CDdwXueGsShXRZs0kuEOUVBUvIb6AKDLkevtZWTHQe/fogR3ko026k/HqoInBVJnog\n\tls7g==","X-Gm-Message-State":"AOAM5327y0e+ctfBlBDH/mtR0vJlHxdzjlBDqRfbMnVtr1I7N4yvcAHh\n\tEBmj5TlOtWTL4qqrZGIti/pMJYUtTOVb27Yoh7yALD+04Re4SA==","X-Google-Smtp-Source":"ABdhPJyhCig7vFA559LaQ9idypbLGZyRA6L1SXBQ9Ft2q3ljoY5wNDZ1GCBf6knWee6SbL1cECoc3aTNyeZ1sjn+0vM=","X-Received":"by 2002:a17:906:819:: with SMTP id\n\te25mr2688775ejd.292.1616466829560; \n\tMon, 22 Mar 2021 19:33:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-4-hiroh@chromium.org>\n\t<YFlMaQsK3d4BABqR@pendragon.ideasonboard.com>","In-Reply-To":"<YFlMaQsK3d4BABqR@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Tue, 23 Mar 2021 11:33:40 +0900","Message-ID":"<CAO5uPHO33NOrMgthxaK4LtTiWgBfR7q5pvpdhNwJ0xqKcH+bCw@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15836,"web_url":"https://patchwork.libcamera.org/comment/15836/","msgid":"<YFn6zgX2aorY0Dzu@pendragon.ideasonboard.com>","date":"2021-03-23T14:27:26","subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","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:33:40AM +0900, Hirokazu Honda wrote:\n> On Tue, Mar 23, 2021 at 11:04 AM Laurent Pinchart wrote:\n> > On Tue, Mar 23, 2021 at 10:42:21AM +0900, Hirokazu Honda wrote:\n> > > This fixes a bug of calling Camera::id() of a moved Camera.\n> >\n> > Indeed, good catch. I however wonder if it wouldn't be simpler to just\n> > drop the move, as cam is a shared pointer ?\n> \n> This might be related to the next patch.\n> I would pass either by copy or std::move().\n> I pass by copy only if a caller still needs the object, otherwise pass\n> by std::move().\n> In this case, we don't need the object's resource, but only the string.\n> So storing it before it enables us to pass cam by std::move().\n> I would rather do this because of this reason.\n> How do you think?\n\nIf it was a std::unique_ptr I would certainly agree. With a\nstd::shared_ptr it's a bit different, as both the caller and the callee\ncan store their own difference. As we still have a use case for using\nthe cam object after the call to CameraDevice::create(), I think that\nnot transfering ownership will make the code simpler here.\n\n> This shared_ptr strategy is come from chromium c++ style guide, where\n> scoped_refptr is almost equivalent to shared_ptr.\n> https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#object-ownership-and-calling-conventions\n> \n> > -       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));\n> > +       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, cam);\n> >\n> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > >\n> > > ---\n> > >  src/android/camera_hal_manager.cpp | 7 ++++---\n> > >  1 file changed, 4 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > index fa398fea..df3f1cd2 100644\n> > > --- a/src/android/camera_hal_manager.cpp\n> > > +++ b/src/android/camera_hal_manager.cpp\n> > > @@ -95,7 +95,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n> > >        * IDs starts from '0' for internal cameras and '1000' for external\n> > >        * cameras.\n> > >        */\n> > > -     auto iter = cameraIdsMap_.find(cam->id());\n> > > +     std::string cameraId = cam->id();\n> > > +     auto iter = cameraIdsMap_.find(cameraId);\n> > >       if (iter != cameraIdsMap_.end()) {\n> > >               id = iter->second;\n> > >       } else {\n> > > @@ -117,12 +118,12 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n> > >       std::unique_ptr<CameraDevice> 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> > > +             LOG(HAL, Error) << \"Failed to initialize camera: \" << cameraId;\n> > >               return;\n> > >       }\n> > >\n> > >       if (isCameraNew) {\n> > > -             cameraIdsMap_.emplace(cam->id(), id);\n> > > +             cameraIdsMap_.emplace(cameraId, id);\n> > >\n> > >               if (isCameraExternal)\n> > >                       nextExternalCameraId_++;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 073E7C32E5\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 23 Mar 2021 14:28:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F8CC68D69;\n\tTue, 23 Mar 2021 15:28:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B6CD68D5E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 23 Mar 2021 15:28:09 +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 B2C9C885;\n\tTue, 23 Mar 2021 15:28:08 +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=\"lJEqQHOW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616509688;\n\tbh=b/mb77cmUr8VJPrhX++pO1CAC1/Em4S6UUesIY68JnQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lJEqQHOWjO7CvoYBVMB0VRPLLeUh6VIf1AiOcNBdzxi3FlLutG3C294GTJaoJbA+C\n\tFnQ6lJBoizBqRMn0xqYydFYzd5ZWSvPUoesbiVkw38kTDurqPj65ysupYqBxpyQ3g5\n\tMo9eeYGv8yecD3MNSsVxndfHLa3T6iAguOz/pwbE=","Date":"Tue, 23 Mar 2021 16:27:26 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YFn6zgX2aorY0Dzu@pendragon.ideasonboard.com>","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-4-hiroh@chromium.org>\n\t<YFlMaQsK3d4BABqR@pendragon.ideasonboard.com>\n\t<CAO5uPHO33NOrMgthxaK4LtTiWgBfR7q5pvpdhNwJ0xqKcH+bCw@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHO33NOrMgthxaK4LtTiWgBfR7q5pvpdhNwJ0xqKcH+bCw@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15850,"web_url":"https://patchwork.libcamera.org/comment/15850/","msgid":"<CAO5uPHMqEA+JkHMi6o9uex-c7i3B9oRrx1QypO47F4j97hVLTA@mail.gmail.com>","date":"2021-03-24T06:46:50","subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","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:28 PM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Mar 23, 2021 at 11:33:40AM +0900, Hirokazu Honda wrote:\n> > On Tue, Mar 23, 2021 at 11:04 AM Laurent Pinchart wrote:\n> > > On Tue, Mar 23, 2021 at 10:42:21AM +0900, Hirokazu Honda wrote:\n> > > > This fixes a bug of calling Camera::id() of a moved Camera.\n> > >\n> > > Indeed, good catch. I however wonder if it wouldn't be simpler to just\n> > > drop the move, as cam is a shared pointer ?\n> >\n> > This might be related to the next patch.\n> > I would pass either by copy or std::move().\n> > I pass by copy only if a caller still needs the object, otherwise pass\n> > by std::move().\n> > In this case, we don't need the object's resource, but only the string.\n> > So storing it before it enables us to pass cam by std::move().\n> > I would rather do this because of this reason.\n> > How do you think?\n>\n> If it was a std::unique_ptr I would certainly agree. With a\n> std::shared_ptr it's a bit different, as both the caller and the callee\n> can store their own difference. As we still have a use case for using\n> the cam object after the call to CameraDevice::create(), I think that\n> not transfering ownership will make the code simpler here.\n>\n\nI see. I will upload the patch to not std::move().\n-Hiro\n\n> > This shared_ptr strategy is come from chromium c++ style guide, where\n> > scoped_refptr is almost equivalent to shared_ptr.\n> > https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#object-ownership-and-calling-conventions\n> >\n> > > -       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));\n> > > +       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, cam);\n> > >\n> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > >\n> > > > ---\n> > > >  src/android/camera_hal_manager.cpp | 7 ++++---\n> > > >  1 file changed, 4 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > index fa398fea..df3f1cd2 100644\n> > > > --- a/src/android/camera_hal_manager.cpp\n> > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > @@ -95,7 +95,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n> > > >        * IDs starts from '0' for internal cameras and '1000' for external\n> > > >        * cameras.\n> > > >        */\n> > > > -     auto iter = cameraIdsMap_.find(cam->id());\n> > > > +     std::string cameraId = cam->id();\n> > > > +     auto iter = cameraIdsMap_.find(cameraId);\n> > > >       if (iter != cameraIdsMap_.end()) {\n> > > >               id = iter->second;\n> > > >       } else {\n> > > > @@ -117,12 +118,12 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)\n> > > >       std::unique_ptr<CameraDevice> 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> > > > +             LOG(HAL, Error) << \"Failed to initialize camera: \" << cameraId;\n> > > >               return;\n> > > >       }\n> > > >\n> > > >       if (isCameraNew) {\n> > > > -             cameraIdsMap_.emplace(cam->id(), id);\n> > > > +             cameraIdsMap_.emplace(cameraId, id);\n> > > >\n> > > >               if (isCameraExternal)\n> > > >                       nextExternalCameraId_++;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6061CC32E5\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 06:47:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B53468D63;\n\tWed, 24 Mar 2021 07:47:02 +0100 (CET)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 34C2268D47\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 07:47:01 +0100 (CET)","by mail-ed1-x529.google.com with SMTP id l18so18144009edc.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 23 Mar 2021 23:47:01 -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=\"ZiAcZd9V\"; 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=LRgkqYh5JE5jl+IYolhJd4w4NLLMQRC9cYN3vBNjcLk=;\n\tb=ZiAcZd9VEpsiOqcaEuO2Lg6XgVp2HPX2IvZPsjSUPwkYo7ES/we2r/d5VxCfykw2vq\n\tYn+6gutLWOvd4KCr08Niewoulyldw9REFkHrxvyxVnV0PJNB0miR1Z55iSLSWPfRF9NS\n\tZjwQg02iYMdRwXtLBIz1GXJELTbIBmu43N8qw=","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=LRgkqYh5JE5jl+IYolhJd4w4NLLMQRC9cYN3vBNjcLk=;\n\tb=CAJTKEGaKsjjd76S3u6f8C8ZS5U1i00PlChFlN5fxcuvSuY8C3L3IMLeYZzpUWRQde\n\tsRiSG/ReAEkr9fZCaziJfVA2zaT07+dJOsqY558qi8FrG9IMz+GVYnCvp5p2ze5wrn4L\n\t7T6gBubWsWe7w/7+2TJHSjJdc3NSkl3a4hi8Ne/keM3CUgG4g2im+fdE8aHPKz89WoBw\n\toXQn+AATyqS3+jI3KwI/rb8Z9O6TmgqWGTvQldT2knG02m1RVVvBS4OYf0FlVIpX1rq2\n\tIX5mBYLZqis2F6Yt0nEO6JqU4N3n5S5rA0Glksj6hXvuO6sGsw0v9LAGGIqISyjLPvJa\n\tGaKw==","X-Gm-Message-State":"AOAM530q42cyn23n0QsGNIMmpLiRzFJ1QQnwYOPKMP8YuDQfRhkfPan8\n\tYOcXzpuDcE8m7O7gPbhm5FZ0gDPFELKcKh1lC1wVZRICBUE=","X-Google-Smtp-Source":"ABdhPJwzpQnZgAqB7JZG2K/XgsBU/nwjjyHXWmHFv/wBxR9pTMJoMXg6CF3CUrPX2jAgdqLgJhKFMyFf89tLFMWa0A0=","X-Received":"by 2002:aa7:c7da:: with SMTP id\n\to26mr1775690eds.244.1616568420789; \n\tTue, 23 Mar 2021 23:47:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323014226.3211412-1-hiroh@chromium.org>\n\t<20210323014226.3211412-4-hiroh@chromium.org>\n\t<YFlMaQsK3d4BABqR@pendragon.ideasonboard.com>\n\t<CAO5uPHO33NOrMgthxaK4LtTiWgBfR7q5pvpdhNwJ0xqKcH+bCw@mail.gmail.com>\n\t<YFn6zgX2aorY0Dzu@pendragon.ideasonboard.com>","In-Reply-To":"<YFn6zgX2aorY0Dzu@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Wed, 24 Mar 2021 15:46:50 +0900","Message-ID":"<CAO5uPHMqEA+JkHMi6o9uex-c7i3B9oRrx1QypO47F4j97hVLTA@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a\n\tfunction call of a moved Camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]