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

[Hirokazu Honda]

Hi Jacopo,

On Tue, May 11, 2021 at 4:52 PM Jacopo Mondi <jacopo at jmondi.org> wrote:

> 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...
>
>
I agree.

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>


> >
> > -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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210511/80255e13/attachment.htm>