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

[Nicolas Dufresne]

On mar, 2020-02-11 at 21:19 +0200, Laurent Pinchart wrote:
> Hi Nicolas,
> 
> 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.

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

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