[libcamera-devel] [PATCH v1 04/23] gst: utils: Add a macro to use a GMutexLocker

[Laurent Pinchart]

Hi Nicolas,

On Tue, Feb 11, 2020 at 04:51:30PM -0500, Nicolas Dufresne wrote:
> On mar, 2020-02-11 at 21:19 +0200, Laurent Pinchart wrote:
> > On Tue, Feb 11, 2020 at 01:50:54PM -0500, Nicolas Dufresne wrote:
> > > On mar, 2020-02-11 at 20:00 +0200, Laurent Pinchart wrote:
> > > > On Tue, Jan 28, 2020 at 10:31:51PM -0500, Nicolas Dufresne wrote:
> > > > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > > 
> > > > > The GMutexLocker uses GCC/CLANG C feature to implement a smart lock,
> > > > 
> > > > s/smart/scoped/ ?
> > > > 
> > > > > that unlocks the mutex when the GMutexLocker goes out of scope. This
> > > > > could have been implemented in C++, but as this is already implemented
> > > > > I decided to just reuse it. This is particularly handy to avoid gotos
> > > > > in error handling cases and to prevent unbalanced locking.
> > > > > 
> > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > > ---
> > > > >  src/gstreamer/gstlibcamera-utils.h | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.h
> > > > > b/src/gstreamer/gstlibcamera-utils.h
> > > > > index 4545512..528dc2c 100644
> > > > > --- a/src/gstreamer/gstlibcamera-utils.h
> > > > > +++ b/src/gstreamer/gstlibcamera-utils.h
> > > > > @@ -13,6 +13,8 @@
> > > > >  
> > > > >  #ifndef __GST_LIBCAMERA_UTILS_H_
> > > > >  
> > > > > +#define GST_OBJECT_LOCKER(obj) g_autoptr(GMutexLocker) locker =
> > > > > g_mutex_locker_new(GST_OBJECT_GET_LOCK(obj))
> > > > > +
> > > > 
> > > > The name of the macro would lead me to think it's provided by gstreamer.
> > > > Should it be renamed to GST_LIBCAMERA_OBJECT_LOCKER() ? Or would that
> > > > be too long ?
> > > 
> > > Sorry for the overhead, as these locker are only present in very recent
> > > GLib, it
> > > was agreed to implement C++ helper instead. I agree with you here, sometimes
> > > I
> > > use GST_ namespace for helper I think maybe be worth moving into core of
> > > GStreamer, but it make little sense for this one since this is not portable
> > > (not
> > > supported by MSVC), so unlikely to be added.
> > > 
> > > About the proposal, let's give the general idea, maybe you have better
> > > ideas.
> > > 
> > > class GLibLocker {
> > > 	GLibLocker(GMutex *mutex) : mutex_(mutex)
> > > 	{
> > >     		g_mutex_lock(mutex_);
> > > 	}
> > > 
> > > 	GLibLocker(GstObject *object) : mutex_(GST_OBJECT_GET_LOCK(gst_object),
> > > rec_mutex_(rec_mutex)
> > 
> > I assume s/gst_object/object/. Not sure what rec_mutex is used for :-)
> 
> Indeed, I just wrote this quickly, I initially thought of having both recursive
> mutex and mutex in the same object, but then retracted and didn't cleanup
> properly. Will have two class, it's cleaner.

Seems like a good idea. By the way, if you want to extend the API of
your lockers, could you try to do so in a way that mimicks the C++
lockers (https://en.cppreference.com/w/cpp/thread/lock_guard,
https://en.cppreference.com/w/cpp/thread/unique_lock or
https://en.cppreference.com/w/cpp/thread/scoped_lock) ?

> > > 	{
> > >     		g_mutex_lock(mutex_);
> > > 	}
> > > 
> > > 	
> > > 	~GLibLocker()
> > > 	{
> > > 		g_mutex_unlock(mutex_);
> > > 	}
> > > };
> > > 
> > > some_code()
> > > {
> > > 	GLibLocker(GST_OBJECT(my_object));
> > > } /* unlocked here */
> > 
> > This looks good to me.
> 
> Ok, and that should bring us back to GLib 2.44, I'll test this time before V2,
> just to make sure. Thanks for the feedback.

Thank you for making our life easier with glib 2.44 :-)

> > > > >  GstCaps *gst_libcamera_stream_formats_to_caps(const
> > > > > libcamera::StreamFormats &formats);
> > > > >  
> > > > >  #endif /* __GST_LIBCAMERA_UTILS_H_ */

-- 
Regards,

Laurent Pinchart