[{"id":16867,"web_url":"https://patchwork.libcamera.org/comment/16867/","msgid":"","date":"2021-05-10T20:29:21","subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote:\n> The CameraDevice class uses std::scoped_lock<> to guard access to the\n> class' descriptors_ member.\n> \n> std::scoped_lock<> provides a set of features that guarantees safety\n> when locking multiple mutexes in a critical section, while for single\n> locks happening in a scoped block it does not provides benefits compared\n> to the simplest std::unique_lock<> which libcamera provides the\n> MutexLocker type for.\n> \n> Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make\n> the implementation consistent with the rest of the code base.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/android/camera_device.cpp | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index bdb5c8ed52aa..ff965a1c8c86 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -22,6 +22,7 @@\n> \n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/thread.h\"\n> #include \"libcamera/internal/utils.h\"\n> \n> #include \"system/graphics.h\"\n> @@ -2016,7 +2017,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tworker_.queueRequest(descriptor.request_.get());\n> \n> \t{\n> -\t\tstd::scoped_lock lock(mutex_);\n> +\t\tMutexLocker lock(mutex_);\n> \t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> \t}\n> \n> @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request *request)\n> {\n> \tdecltype(descriptors_)::node_type node;\n> \t{\n> -\t\tstd::scoped_lock lock(mutex_);\n> +\t\tMutexLocker lock(mutex_);\n> \t\tauto it = descriptors_.find(request->cookie());\n> \t\tif (it == descriptors_.end()) {\n> \t\t\t/*\n> -- \n> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 452BDBF829\n\tfor ;\n\tMon, 10 May 2021 20:29:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0970C688E5;\n\tMon, 10 May 2021 22:29:24 +0200 (CEST)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0396602BE\n\tfor ;\n\tMon, 10 May 2021 22:29:22 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id m11so8876390lfg.3\n\tfor ;\n\tMon, 10 May 2021 13:29:22 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tf17sm2346231lfu.215.2021.05.10.13.29.21\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 10 May 2021 13:29:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"T8AYPbYK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=gQIUk3Ej4XKYstF3O1ZKX0hWpIJ501dSrH60uC8cd8M=;\n\tb=T8AYPbYKrsMLAB/ykdQyV/XbpHfz/ZUF69wKTwx0G9yGZ9AfVxZkcCG7exX5jm7pLm\n\t5bopnWNgU7s/q0ZtVtNEJK+Ype/UUi8l3kX4qMEc+tHJVcAZHaDtD+5G9ZcVm0+EM5xn\n\tZ35C4/nwrfCjQcoYz3t/F88Xy2QYlZ37MvHo4wqFrp+yJl6Tq48Ab5qJoaMOI5P4jMF2\n\t+gjyg1Epe5c+eYXGcswpVBP4P0gHOAzqedKwzGkWZleg69aOmu4nqbcF8kUX/A+8S8YG\n\tXuFXCWD9a/m0n6LE3S/0cK1QpKgC5q92Hbj1oUOkwaAr/oMJuu7/G0gE35aL+0VzF2bI\n\toWOg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=gQIUk3Ej4XKYstF3O1ZKX0hWpIJ501dSrH60uC8cd8M=;\n\tb=Cn16PsIs6b4MyPhhjKBVTPdgPR+semOkIzD02LLBX+E9gNZLdcBIrRHW88q4wwZQIp\n\tZmEjGkJMmLDH57hiVlF95/mUvZfPjncB5pVuIDEfifG2kbSYXhOsHfx1YJUZz6YE6lpe\n\trwdNo0bFTwm1vK6yTkMPd+hYeoY77I3ccznM7kAYwQ78+PsyGjg82X7KRiafErAvfqHk\n\tVKF+/lhJ+QPFOYBX9/8y8LnLZk+Z+A1zaSc6u9Jr7f2MN8424Q1HjEhJxr7S2bRinOmK\n\txaTQ6O/45nE1Os8B6UlrjQweXyVczcRMryONOmosdvMhlX6qMb33RX9nqN6UpdK0Hsat\n\t3+Hg==","X-Gm-Message-State":"AOAM530LPtpwBd0wTAH/89sgUQroXvHZw2o9mmG3/raiE8fQggG6JjmO\n\thp66fdRocaChr+epdL8+d0gA+w==","X-Google-Smtp-Source":"ABdhPJwewwAEhbm8BO6dKsY1yOQ+aUf6NTgJevRJFYb7g4Ff5jz873Sgxdd8UNg6eFmrEOpcLE8P2Q==","X-Received":"by 2002:a19:385c:: with SMTP id\n\td28mr18044772lfj.13.1620678562150; \n\tMon, 10 May 2021 13:29:22 -0700 (PDT)","Date":"Mon, 10 May 2021 22:29:21 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210510105235.28319-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16881,"web_url":"https://patchwork.libcamera.org/comment/16881/","msgid":"","date":"2021-05-11T05:27:29","subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch.\n\nOn Tue, May 11, 2021 at 5:29 AM Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote:\n> > The CameraDevice class uses std::scoped_lock<> to guard access to the\n> > class' descriptors_ member.\n> >\n> > std::scoped_lock<> provides a set of features that guarantees safety\n> > when locking multiple mutexes in a critical section, while for single\n> > locks happening in a scoped block it does not provides benefits compared\n> > to the simplest std::unique_lock<> which libcamera provides the\n> > MutexLocker type for.\n> >\n> > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make\n> > the implementation consistent with the rest of the code base.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n>\nI hear since unique_lock has more functionalities than scoped_lock, the\nperformance of unique_lock is better than scoped_lock.\nSo I would love to use scoped_lock if it is only used like std::lock_guard.\nOn the other hand, unique_lock is needed for std::condition_variable.\nIMO, we should subtilize std::unique_lock and std::scoped_lock.\nBut ideally, we should declare our own mutex class to utilize clang thread\nannotation. https://bugs.libcamera.org/show_bug.cgi?id=23\n\nI am not strongly against this patch though as unique_lock covers\nscoped_lock and the performance difference should be fraction.\n\n-Hiro\n\nReviewed-by: Niklas Söderlund \n>\n> > ---\n> > src/android/camera_device.cpp | 5 +++--\n> > 1 file changed, 3 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n> > index bdb5c8ed52aa..ff965a1c8c86 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -22,6 +22,7 @@\n> >\n> > #include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/log.h\"\n> > +#include \"libcamera/internal/thread.h\"\n> > #include \"libcamera/internal/utils.h\"\n> >\n> > #include \"system/graphics.h\"\n> > @@ -2016,7 +2017,7 @@ int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > worker_.queueRequest(descriptor.request_.get());\n> >\n> > {\n> > - std::scoped_lock lock(mutex_);\n> > + MutexLocker lock(mutex_);\n> > descriptors_[descriptor.request_->cookie()] =\n> std::move(descriptor);\n> > }\n> >\n> > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request\n> *request)\n> > {\n> > decltype(descriptors_)::node_type node;\n> > {\n> > - std::scoped_lock lock(mutex_);\n> > + MutexLocker lock(mutex_);\n> > auto it = descriptors_.find(request->cookie());\n> > if (it == descriptors_.end()) {\n> > /*\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 B4187BF839\n\tfor ;\n\tTue, 11 May 2021 05:27:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0461B68920;\n\tTue, 11 May 2021 07:27:42 +0200 (CEST)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B802C68915\n\tfor ;\n\tTue, 11 May 2021 07:27:40 +0200 (CEST)","by mail-ed1-x530.google.com with SMTP id s6so21318401edu.10\n\tfor ;\n\tMon, 10 May 2021 22:27:40 -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=\"XJlCO7RL\"; 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=B7PEyUBvB+auA/CSkIiVLHUF5fnjmNsHwTr/BqO3tEE=;\n\tb=XJlCO7RLY1P+8cQ5Th4AVHDJxY0fDpPG1w2rqPeHgDwxujtAs5XSaAf9DXxfUn1MpB\n\tYsYmOAPLGYOMjXygXVJZ/tlP39ZoC5GwarPU1qhFbIH+Htn5rucUqG4TNBy5I/bwaJ8x\n\tZhE/O+IylDD6k6hBE5PxCjjg6Uyk51hxBe4HU=","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=B7PEyUBvB+auA/CSkIiVLHUF5fnjmNsHwTr/BqO3tEE=;\n\tb=HHxzb0yj7yGFYW5Txh52nb1z1kOBC/tHmdryc0zxrOmT0+G15nMOxxH7S023T9SCui\n\t1WBb6d3oWb7yzRSQ3al7JkF9Whf0dQ2B237LW7O2YHmS8F7XaWjUKhcEY/0jJ2Nw47/r\n\tilOYxltIqsFdxHitxe2OZQPv11n9/2FKOpQpencPMUeGNs+aq0s7tiap3TO4uZ0ovxfe\n\tHcKdaSiRlt+QoRoUvdTbJipREdYno7wCS2Fd5UZLnBN2oYlCeBNTxuhBK0521+ZxT64O\n\tqK2dVqK4vUg2b7d1y/MlHdzV296jEc/V6XgH0Ah9n8PEedGU1pHQi1tKnPWL37BzhsnO\n\tbvCw==","X-Gm-Message-State":"AOAM530OkHj3gD+ew//D2GZk0+zllzGHw/qFAdxllfctQ9QeKdWmGwFD\n\tDwVF5GMV3x3H5WZnMBz3d5gcilipRjNOtfBbqWu91w==","X-Google-Smtp-Source":"ABdhPJyIsSLEh7ziOa65b2/4Ywg33PlwZfsGE7bb3SDz+pG8QIBC48pOsPiXQ/2F85hbhQ8gdvF5RwrTZImyhwnjync=","X-Received":"by 2002:aa7:d40e:: with SMTP id\n\tz14mr28783622edq.73.1620710860420; \n\tMon, 10 May 2021 22:27:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-6-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 14:27:29 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","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":"multipart/mixed;\n\tboundary=\"===============7385044590220373973==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16885,"web_url":"https://patchwork.libcamera.org/comment/16885/","msgid":"<20210511075253.jnz4r3vj2kz25q4v@uno.localdomain>","date":"2021-05-11T07:52:53","subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, May 11, 2021 at 02:27:29PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch.\n>\n> On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n>\n> > Hi Jacopo,\n> >\n> > Thanks for your patch.\n> >\n> > On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote:\n> > > The CameraDevice class uses std::scoped_lock<> to guard access to the\n> > > class' descriptors_ member.\n> > >\n> > > std::scoped_lock<> provides a set of features that guarantees safety\n> > > when locking multiple mutexes in a critical section, while for single\n> > > locks happening in a scoped block it does not provides benefits compared\n> > > to the simplest std::unique_lock<> which libcamera provides the\n> > > MutexLocker type for.\n> > >\n> > > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make\n> > > the implementation consistent with the rest of the code base.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> >\n> >\n> I hear since unique_lock has more functionalities than scoped_lock, the\n> performance of unique_lock is better than scoped_lock.\n\nMy understanding was that unique_lock being simpler than scoped_lock\n:)\n\n> So I would love to use scoped_lock if it is only used like std::lock_guard.\n> On the other hand, unique_lock is needed for std::condition_variable.\n> IMO, we should subtilize std::unique_lock and std::scoped_lock.\n> But ideally, we should declare our own mutex class to utilize clang thread\n> annotation. https://bugs.libcamera.org/show_bug.cgi?id=23\n\nOh nice you have created a bug already.\n\n>\n> I am not strongly against this patch though as unique_lock covers\n> scoped_lock and the performance difference should be fraction.\n\nThat was my thinking and also that the less we mix different types of\nmutexes the easier will be later to rework them by using a unified\nconstruct...\n\n>\n> -Hiro\n>\n> Reviewed-by: Niklas Söderlund \n\nThanks!\n\n> >\n> > > ---\n> > > src/android/camera_device.cpp | 5 +++--\n> > > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp\n> > b/src/android/camera_device.cpp\n> > > index bdb5c8ed52aa..ff965a1c8c86 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -22,6 +22,7 @@\n> > >\n> > > #include \"libcamera/internal/formats.h\"\n> > > #include \"libcamera/internal/log.h\"\n> > > +#include \"libcamera/internal/thread.h\"\n> > > #include \"libcamera/internal/utils.h\"\n> > >\n> > > #include \"system/graphics.h\"\n> > > @@ -2016,7 +2017,7 @@ int\n> > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > worker_.queueRequest(descriptor.request_.get());\n> > >\n> > > {\n> > > - std::scoped_lock lock(mutex_);\n> > > + MutexLocker lock(mutex_);\n> > > descriptors_[descriptor.request_->cookie()] =\n> > std::move(descriptor);\n> > > }\n> > >\n> > > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request\n> > *request)\n> > > {\n> > > decltype(descriptors_)::node_type node;\n> > > {\n> > > - std::scoped_lock lock(mutex_);\n> > > + MutexLocker lock(mutex_);\n> > > auto it = descriptors_.find(request->cookie());\n> > > if (it == descriptors_.end()) {\n> > > /*\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 A454BBF829\n\tfor ;\n\tTue, 11 May 2021 07:52:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16EC06891B;\n\tTue, 11 May 2021 09:52:13 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BEF55602B7\n\tfor ;\n\tTue, 11 May 2021 09:52:10 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B0F9D40005;\n\tTue, 11 May 2021 07:52:09 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Tue, 11 May 2021 09:52:53 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210511075253.jnz4r3vj2kz25q4v@uno.localdomain>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-6-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16888,"web_url":"https://patchwork.libcamera.org/comment/16888/","msgid":"","date":"2021-05-11T09:21:51","subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, May 11, 2021 at 4:52 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Tue, May 11, 2021 at 02:27:29PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch.\n> >\n> > On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your patch.\n> > >\n> > > On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote:\n> > > > The CameraDevice class uses std::scoped_lock<> to guard access to the\n> > > > class' descriptors_ member.\n> > > >\n> > > > std::scoped_lock<> provides a set of features that guarantees safety\n> > > > when locking multiple mutexes in a critical section, while for single\n> > > > locks happening in a scoped block it does not provides benefits\n> compared\n> > > > to the simplest std::unique_lock<> which libcamera provides the\n> > > > MutexLocker type for.\n> > > >\n> > > > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to\n> make\n> > > > the implementation consistent with the rest of the code base.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > >\n> > I hear since unique_lock has more functionalities than scoped_lock, the\n> > performance of unique_lock is better than scoped_lock.\n>\n> My understanding was that unique_lock being simpler than scoped_lock\n> :)\n>\n> > So I would love to use scoped_lock if it is only used like\n> std::lock_guard.\n> > On the other hand, unique_lock is needed for std::condition_variable.\n> > IMO, we should subtilize std::unique_lock and std::scoped_lock.\n> > But ideally, we should declare our own mutex class to utilize clang\n> thread\n> > annotation. https://bugs.libcamera.org/show_bug.cgi?id=23\n>\n> Oh nice you have created a bug already.\n>\n> >\n> > I am not strongly against this patch though as unique_lock covers\n> > scoped_lock and the performance difference should be fraction.\n>\n> That was my thinking and also that the less we mix different types of\n> mutexes the easier will be later to rework them by using a unified\n> construct...\n>\n>\nI agree.\n\nReviewed-by: Hirokazu Honda \n\n\n> >\n> > -Hiro\n> >\n> > Reviewed-by: Niklas Söderlund \n>\n> Thanks!\n>\n> > >\n> > > > ---\n> > > > src/android/camera_device.cpp | 5 +++--\n> > > > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp\n> > > b/src/android/camera_device.cpp\n> > > > index bdb5c8ed52aa..ff965a1c8c86 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -22,6 +22,7 @@\n> > > >\n> > > > #include \"libcamera/internal/formats.h\"\n> > > > #include \"libcamera/internal/log.h\"\n> > > > +#include \"libcamera/internal/thread.h\"\n> > > > #include \"libcamera/internal/utils.h\"\n> > > >\n> > > > #include \"system/graphics.h\"\n> > > > @@ -2016,7 +2017,7 @@ int\n> > > CameraDevice::processCaptureRequest(camera3_capture_request_t\n> *camera3Reques\n> > > > worker_.queueRequest(descriptor.request_.get());\n> > > >\n> > > > {\n> > > > - std::scoped_lock lock(mutex_);\n> > > > + MutexLocker lock(mutex_);\n> > > > descriptors_[descriptor.request_->cookie()] =\n> > > std::move(descriptor);\n> > > > }\n> > > >\n> > > > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request\n> > > *request)\n> > > > {\n> > > > decltype(descriptors_)::node_type node;\n> > > > {\n> > > > - std::scoped_lock lock(mutex_);\n> > > > + MutexLocker lock(mutex_);\n> > > > auto it = descriptors_.find(request->cookie());\n> > > > if (it == descriptors_.end()) {\n> > > > /*\n> > > > --\n> > > > 2.31.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 E1795BF829\n\tfor ;\n\tTue, 11 May 2021 09:22:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3C9B6891B;\n\tTue, 11 May 2021 11:22:03 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1CB5C602B7\n\tfor ;\n\tTue, 11 May 2021 11:22:02 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id j10so1663876ejb.3\n\tfor ;\n\tTue, 11 May 2021 02:22:02 -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=\"GGhqmeni\"; 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=275qQKY2gWANPhzpOjK5dM/PpsO0F/UG5fQoD703Iqo=;\n\tb=GGhqmeniOKnlxLUMA1b8t2dBFMNfjT3EaHmqW29/zhDGcSVES/OQwXgpDpr+hLuiZS\n\tx2CW9coYk96SRI24dzN8N2OsT8jg6BlsUF4sSH6PBPQozOZiByKvGj/mpqhg9o34+bci\n\tVsUJjF+LLvbaqVMhpgulpBKfc3Jje4IAddnOE=","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=275qQKY2gWANPhzpOjK5dM/PpsO0F/UG5fQoD703Iqo=;\n\tb=jdZyWnDru86fky9q+s9F2tjDsaM5iD/rjOQWqIjtkUYRDcNPANMuHBflCsFE0xsG++\n\tMYUKQaQuct2EcR+BpYiW8gVsf9Wvu8U/Us8MpnCfHrFA+h7iGrNxKOxMfkFHT269RrJj\n\tyq4lrKcXBYcTqYEaVmc7LR43h3hjyGARDt08hhTN6rrhEhrdpJuPn7ZoO4LfofcyMGnf\n\tS/0Z5KxcMx0oWbdhimF5AjWWlI/E3dpPd4c2/Mr3ENyt5I+lY06WTlYeIsqBXOmAzuGK\n\tvjPrj6TbZahoXQGZO/rxNPSb/90Qby04HxRaG8yv4qh0mJZf7tBA2sMH4o2VYBNuBrsV\n\tmauA==","X-Gm-Message-State":"AOAM531MIN4MuoGD4kCXsj2MBEkJNtJBXyGTZ7rgiPNz2PJQIByjg42L\n\tpEH11LZ1mhz2Xr6wZBRI5Ub0kc4jofEXSvz3dADaabE4tKQ=","X-Google-Smtp-Source":"ABdhPJyLVDxlD/d9ttOFFofG5CBWT0AxqlBMDUN6pWKZbuwAf84C8kCxWOHUhHyonF0GMT+WWCRgMs2dcb9aUYFvsrM=","X-Received":"by 2002:a17:906:538a:: with SMTP id\n\tg10mr5538245ejo.243.1620724921808; \n\tTue, 11 May 2021 02:22:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-6-jacopo@jmondi.org>\n\t\n\t\n\t<20210511075253.jnz4r3vj2kz25q4v@uno.localdomain>","In-Reply-To":"<20210511075253.jnz4r3vj2kz25q4v@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 18:21:51 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<>\n\twith libcamera::MutexLocker","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":"multipart/mixed;\n\tboundary=\"===============4097299768332836919==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]