<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 4:52 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</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 Hiro,<br>
<br>
On Tue, May 11, 2021 at 02:27:29PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo, thank you for the patch.<br>
><br>
> On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund <<br>
> <a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>> wrote:<br>
><br>
> > 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>
> ><br>
> I hear since unique_lock has more functionalities than scoped_lock, the<br>
> performance of unique_lock is better than scoped_lock.<br>
<br>
My understanding was that unique_lock being simpler than scoped_lock<br>
:)<br>
<br>
> So I would love to use scoped_lock if it is only used like std::lock_guard.<br>
> On the other hand, unique_lock is needed for std::condition_variable.<br>
> IMO, we should subtilize std::unique_lock and std::scoped_lock.<br>
> But ideally, we should declare our own mutex class to utilize clang thread<br>
> annotation. <a href="https://bugs.libcamera.org/show_bug.cgi?id=23" rel="noreferrer" target="_blank">https://bugs.libcamera.org/show_bug.cgi?id=23</a><br>
<br>
Oh nice you have created a bug already.<br>
<br>
><br>
> I am not strongly against this patch though as unique_lock covers<br>
> scoped_lock and the performance difference should be fraction.<br>
<br>
That was my thinking and also that the less we mix different types of<br>
mutexes the easier will be later to rework them by using a unified<br>
construct...<br>
<br></blockquote><div><br></div><div>I agree. </div><div><br></div><div>Reviewed-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org">hiroh@chromium.org</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> -Hiro<br>
><br>
> Reviewed-by: Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>><br>
<br>
Thanks!<br>
<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<br>
> > 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<br>
> > 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()] =<br>
> > std::move(descriptor);<br>
> > >       }<br>
> > ><br>
> > > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request<br>
> > *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>
> ><br>
</blockquote></div></div>