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

[Jacopo Mondi]

Hi Hiro,

On Tue, May 11, 2021 at 02:27:29PM +0900, Hirokazu Honda wrote:
> 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.

My understanding was that unique_lock being simpler 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

Oh nice you have created a bug already.

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

That was my thinking and also that the less we mix different types of
mutexes the easier will be later to rework them by using a unified
construct...

>
> -Hiro
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

Thanks!

> >
> > > ---
> > >  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
> >