[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 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 :-)

> 	{
>     		g_mutex_lock(mutex_);
> 	}
> 
> 	
> 	~GLibLocker()
> 	{
> 		g_mutex_unlock(mutex_);
> 	}
> };
> 
> some_code()
> {
> 	GLibLocker(GST_OBJECT(my_object));
> } /* unlocked here */

This looks good to me.

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

-- 
Regards,

Laurent Pinchart