[libcamera-devel,v1,04/23] gst: utils: Add a macro to use a GMutexLocker
diff mbox series

Message ID 20200129033210.278800-5-nicolas@ndufresne.ca
State Superseded
Headers show
Series
  • GStreamer Element for libcamera
Related show

Commit Message

Nicolas Dufresne Jan. 29, 2020, 3:31 a.m. UTC 
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

The GMutexLocker uses GCC/CLANG C feature to implement a smart lock,
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@collabora.com>
---
 src/gstreamer/gstlibcamera-utils.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Laurent Pinchart Feb. 11, 2020, 6 p.m. UTC | #1 
Hi Nicolas,

Thank you for the patch.

On Tue, Jan 28, 2020 at 10:31:51PM -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@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@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 ?

>  GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);
>  
>  #endif /* __GST_LIBCAMERA_UTILS_H_ */
Nicolas Dufresne Feb. 11, 2020, 6:50 p.m. UTC | #2 
On mar, 2020-02-11 at 20:00 +0200, Laurent Pinchart wrote:
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Tue, Jan 28, 2020 at 10:31:51PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@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@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)
	{
    		g_mutex_lock(mutex_);
	}

	
	~GLibLocker()
	{
		g_mutex_unlock(mutex_);
	}
};

some_code()
{
	GLibLocker(GST_OBJECT(my_object));
} /* unlocked here */

> 
> >  GstCaps *gst_libcamera_stream_formats_to_caps(const
> > libcamera::StreamFormats &formats);
> >  
> >  #endif /* __GST_LIBCAMERA_UTILS_H_ */
Laurent Pinchart Feb. 11, 2020, 7:19 p.m. UTC | #3 
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@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@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_ */
Nicolas Dufresne Feb. 11, 2020, 9:51 p.m. UTC | #4 
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@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@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_ */
Laurent Pinchart Feb. 11, 2020, 11:18 p.m. UTC | #5 
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@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@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_ */

Patch
diff mbox series 

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))
+
 GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);
 
 #endif /* __GST_LIBCAMERA_UTILS_H_ */