[libcamera-devel] [PATCH 5/8] android: Replace scoped_lock<> with libcamera::MutexLocker

[Hirokazu Honda]

Hi Jacopo, thank you for the patch.

On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund <
niklas.soderlund at ragnatech.se> wrote:

> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote:
> > The CameraDevice class uses std::scoped_lock<> to guard access to the
> > class' descriptors_ member.
> >
> > std::scoped_lock<> provides a set of features that guarantees safety
> > when locking multiple mutexes in a critical section, while for single
> > locks happening in a scoped block it does not provides benefits compared
> > to the simplest std::unique_lock<> which libcamera provides the
> > MutexLocker type for.
> >
> > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make
> > the implementation consistent with the rest of the code base.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>
>
I hear since unique_lock has more functionalities than scoped_lock, the
performance of unique_lock is better than scoped_lock.
So I would love to use scoped_lock if it is only used like std::lock_guard.
On the other hand, unique_lock is needed for std::condition_variable.
IMO, we should subtilize std::unique_lock and std::scoped_lock.
But ideally, we should declare our own mutex class to utilize clang thread
annotation. https://bugs.libcamera.org/show_bug.cgi?id=23

I am not strongly against this patch though as unique_lock covers
scoped_lock and the performance difference should be fraction.

-Hiro

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > ---
> >  src/android/camera_device.cpp | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp
> b/src/android/camera_device.cpp
> > index bdb5c8ed52aa..ff965a1c8c86 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -22,6 +22,7 @@
> >
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/log.h"
> > +#include "libcamera/internal/thread.h"
> >  #include "libcamera/internal/utils.h"
> >
> >  #include "system/graphics.h"
> > @@ -2016,7 +2017,7 @@ int
> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >       worker_.queueRequest(descriptor.request_.get());
> >
> >       {
> > -             std::scoped_lock<std::mutex> lock(mutex_);
> > +             MutexLocker lock(mutex_);
> >               descriptors_[descriptor.request_->cookie()] =
> std::move(descriptor);
> >       }
> >
> > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request
> *request)
> >  {
> >       decltype(descriptors_)::node_type node;
> >       {
> > -             std::scoped_lock<std::mutex> lock(mutex_);
> > +             MutexLocker lock(mutex_);
> >               auto it = descriptors_.find(request->cookie());
> >               if (it == descriptors_.end()) {
> >                       /*
> > --
> > 2.31.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210511/2817817d/attachment-0001.htm>