<div dir="ltr"><div dir="ltr">Hi Jacopo, thank you for the patch.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se">niklas.soderlund@ragnatech.se</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Jacopo,<br>
<br>
Thanks for your patch.<br>
<br>
On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote:<br>
> The CameraDevice class uses std::scoped_lock<> to guard access to the<br>
> class' descriptors_ member.<br>
> <br>
> std::scoped_lock<> provides a set of features that guarantees safety<br>
> when locking multiple mutexes in a critical section, while for single<br>
> locks happening in a scoped block it does not provides benefits compared<br>
> to the simplest std::unique_lock<> which libcamera provides the<br>
> MutexLocker type for.<br>
> <br>
> Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make<br>
> the implementation consistent with the rest of the code base.<br>
> <br>
> Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br></blockquote><div><br></div><div>I hear since unique_lock has more functionalities than scoped_lock, the performance of unique_lock is better than scoped_lock.<br></div><div>So I would love to use scoped_lock if it is only used like std::lock_guard.</div><div>On the other hand, unique_lock is needed for std::condition_variable.</div><div>IMO, we should subtilize std::unique_lock and std::scoped_lock.</div><div>But ideally, we should declare our own mutex class to utilize clang thread annotation. <a href="https://bugs.libcamera.org/show_bug.cgi?id=23">https://bugs.libcamera.org/show_bug.cgi?id=23</a></div><div><br></div><div>I am not strongly against this patch though as unique_lock covers scoped_lock and the performance difference should be fraction.</div><div><br></div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Reviewed-by: Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>><br>
<br>
> ---<br>
>  src/android/camera_device.cpp | 5 +++--<br>
>  1 file changed, 3 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> index bdb5c8ed52aa..ff965a1c8c86 100644<br>
> --- a/src/android/camera_device.cpp<br>
> +++ b/src/android/camera_device.cpp<br>
> @@ -22,6 +22,7 @@<br>
>  <br>
>  #include "libcamera/internal/formats.h"<br>
>  #include "libcamera/internal/log.h"<br>
> +#include "libcamera/internal/thread.h"<br>
>  #include "libcamera/internal/utils.h"<br>
>  <br>
>  #include "system/graphics.h"<br>
> @@ -2016,7 +2017,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques<br>
>       worker_.queueRequest(descriptor.request_.get());<br>
>  <br>
>       {<br>
> -             std::scoped_lock<std::mutex> lock(mutex_);<br>
> +             MutexLocker lock(mutex_);<br>
>               descriptors_[descriptor.request_->cookie()] = std::move(descriptor);<br>
>       }<br>
>  <br>
> @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request *request)<br>
>  {<br>
>       decltype(descriptors_)::node_type node;<br>
>       {<br>
> -             std::scoped_lock<std::mutex> lock(mutex_);<br>
> +             MutexLocker lock(mutex_);<br>
>               auto it = descriptors_.find(request->cookie());<br>
>               if (it == descriptors_.end()) {<br>
>                       /*<br>
> -- <br>
> 2.31.1<br>
> <br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
<br>
-- <br>
Regards,<br>
Niklas Söderlund<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>