[libcamera-devel,v1,13/23] gst: libcamerasrc: Send stream start event

2747 diff mbox series

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Prior to sending caps, we need to send a stream-start event. This requires
generating a stream and a group id. The stream id is random for live sources and
the group id is shared across all pads.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 src/gstreamer/gstlibcamerasrc.cpp | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Hi Nicolas,

On 29/01/2020 03:32, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> Prior to sending caps, we need to send a stream-start event. This requires
> generating a stream and a group id. The stream id is random for live sources and
> the group id is shared across all pads.

Interestingly, I can compile against gstreamer1.0 1.14 up until this
patch, but then I hit another dependency.

The target I'm building on has glib-2.0 version 2.59, and
GRecMutexLocker was introduced in 2.60 :S


Perhaps this patch might also need to add something like the (untested)
following:

diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
index 9d273437..58cf0009 100644
--- a/src/gstreamer/meson.build
+++ b/src/gstreamer/meson.build
@@ -11,6 +11,9 @@ libcamera_gst_c_args = [
     '-DPACKAGE="@0@"'.format(meson.project_name()),
 ]

+glib_dep = dependency('glib-2.0', version : '>=2.60',
+    required : get_option('gstreamer'))
+
 gst_dep = dependency('gstreamer-video-1.0', version : '>=1.14',
     required : get_option('gstreamer'))


> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index be32a11..b1a21dc 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -18,6 +18,8 @@ using namespace libcamera;
>  GST_DEBUG_CATEGORY_STATIC(source_debug);
>  #define GST_CAT_DEFAULT source_debug
>  
> +#define STREAM_LOCKER(obj) g_autoptr(GRecMutexLocker) stream_locker = g_rec_mutex_locker_new(&GST_LIBCAMERA_SRC(obj)->stream_lock)
> +
>  /* Used for C++ object with destructors */
>  struct GstLibcameraSrcState {
>  	std::shared_ptr<CameraManager> cm;
> @@ -127,9 +129,20 @@ gst_libcamera_src_task_run(gpointer user_data)
>  static void
>  gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)
>  {
> +	STREAM_LOCKER(user_data);
>  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> +	GstLibcameraSrcState *state = self->state;
>  
>  	GST_DEBUG_OBJECT(self, "Streaming thread has started");
> +
> +	guint group_id = gst_util_group_id_next();
> +	for (GstPad *srcpad : state->srcpads) {
> +		/* Create stream-id and push stream-start */
> +		g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr);
> +		GstEvent *event = gst_event_new_stream_start(stream_id);
> +		gst_event_set_group_id(event, group_id);
> +		gst_pad_push_event(srcpad, event);
> +	}
>  }
>  
>  static void
>

Hello again :-)

On 29/01/2020 12:35, Kieran Bingham wrote:
> Hi Nicolas,
> 
> On 29/01/2020 03:32, Nicolas Dufresne wrote:
>> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>
>> Prior to sending caps, we need to send a stream-start event. This requires
>> generating a stream and a group id. The stream id is random for live sources and
>> the group id is shared across all pads.
> 
> Interestingly, I can compile against gstreamer1.0 1.14 up until this
> patch, but then I hit another dependency.
> 
> The target I'm building on has glib-2.0 version 2.59, and
> GRecMutexLocker was introduced in 2.60 :S
> 

Hrm, ok so my rootfs is Debian 10 stable ("Buster"), which is probably a
reasonable target to aim to support:

With Buster:
libglib-2.0 - 2.58 https://packages.debian.org/buster/libglib2.0-0
Gstreamer - 1.14.4 https://packages.debian.org/buster/libgstreamer1.0-0


I wonder if this can be worked around easily to define the functionality
needed when it's not in glib. It would be a pain to have to rewrite
something ...

--
Kieran


> Perhaps this patch might also need to add something like the (untested)
> following:
> 
> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> index 9d273437..58cf0009 100644
> --- a/src/gstreamer/meson.build
> +++ b/src/gstreamer/meson.build
> @@ -11,6 +11,9 @@ libcamera_gst_c_args = [
>      '-DPACKAGE="@0@"'.format(meson.project_name()),
>  ]
> 
> +glib_dep = dependency('glib-2.0', version : '>=2.60',
> +    required : get_option('gstreamer'))
> +
>  gst_dep = dependency('gstreamer-video-1.0', version : '>=1.14',
>      required : get_option('gstreamer'))
> 
> 
>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>> ---
>>  src/gstreamer/gstlibcamerasrc.cpp | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
>> index be32a11..b1a21dc 100644
>> --- a/src/gstreamer/gstlibcamerasrc.cpp
>> +++ b/src/gstreamer/gstlibcamerasrc.cpp
>> @@ -18,6 +18,8 @@ using namespace libcamera;
>>  GST_DEBUG_CATEGORY_STATIC(source_debug);
>>  #define GST_CAT_DEFAULT source_debug
>>  
>> +#define STREAM_LOCKER(obj) g_autoptr(GRecMutexLocker) stream_locker = g_rec_mutex_locker_new(&GST_LIBCAMERA_SRC(obj)->stream_lock)
>> +
>>  /* Used for C++ object with destructors */
>>  struct GstLibcameraSrcState {
>>  	std::shared_ptr<CameraManager> cm;
>> @@ -127,9 +129,20 @@ gst_libcamera_src_task_run(gpointer user_data)
>>  static void
>>  gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)
>>  {
>> +	STREAM_LOCKER(user_data);
>>  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
>> +	GstLibcameraSrcState *state = self->state;
>>  
>>  	GST_DEBUG_OBJECT(self, "Streaming thread has started");
>> +
>> +	guint group_id = gst_util_group_id_next();
>> +	for (GstPad *srcpad : state->srcpads) {
>> +		/* Create stream-id and push stream-start */
>> +		g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr);
>> +		GstEvent *event = gst_event_new_stream_start(stream_id);
>> +		gst_event_set_group_id(event, group_id);
>> +		gst_pad_push_event(srcpad, event);
>> +	}
>>  }
>>  
>>  static void
>>
>

Le mercredi 29 janvier 2020 à 12:35 +0000, Kieran Bingham a écrit :
> Hi Nicolas,
> 
> On 29/01/2020 03:32, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > Prior to sending caps, we need to send a stream-start event. This requires
> > generating a stream and a group id. The stream id is random for live sources
> > and
> > the group id is shared across all pads.
> 
> Interestingly, I can compile against gstreamer1.0 1.14 up until this
> patch, but then I hit another dependency.
> 
> The target I'm building on has glib-2.0 version 2.59, and
> GRecMutexLocker was introduced in 2.60 :S

Good catch, 2.60 is a bit aggressive. I have considered just implementing my own
locker in C++, since this is trivial, no I have a rationale to do so. With C++ I
can use the same locker for Mutex and RecMutex.

> 
> Perhaps this patch might also need to add something like the (untested)
> following:
> 
> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> index 9d273437..58cf0009 100644
> --- a/src/gstreamer/meson.build
> +++ b/src/gstreamer/meson.build
> @@ -11,6 +11,9 @@ libcamera_gst_c_args = [
>      '-DPACKAGE="@0@"'.format(meson.project_name()),
>  ]
> 
> +glib_dep = dependency('glib-2.0', version : '>=2.60',
> +    required : get_option('gstreamer'))
> +

I wanted to rely on GStreamer own restriction, to keep things simplier. So I'll
check what 1.14 requires, and craft a test with such an old GLib.

>  gst_dep = dependency('gstreamer-video-1.0', version : '>=1.14',
>      required : get_option('gstreamer'))
> 
> 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > b/src/gstreamer/gstlibcamerasrc.cpp
> > index be32a11..b1a21dc 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -18,6 +18,8 @@ using namespace libcamera;
> >  GST_DEBUG_CATEGORY_STATIC(source_debug);
> >  #define GST_CAT_DEFAULT source_debug
> >  
> > +#define STREAM_LOCKER(obj) g_autoptr(GRecMutexLocker) stream_locker =
> > g_rec_mutex_locker_new(&GST_LIBCAMERA_SRC(obj)->stream_lock)
> > +
> >  /* Used for C++ object with destructors */
> >  struct GstLibcameraSrcState {
> >  	std::shared_ptr<CameraManager> cm;
> > @@ -127,9 +129,20 @@ gst_libcamera_src_task_run(gpointer user_data)
> >  static void
> >  gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer
> > user_data)
> >  {
> > +	STREAM_LOCKER(user_data);
> >  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> > +	GstLibcameraSrcState *state = self->state;
> >  
> >  	GST_DEBUG_OBJECT(self, "Streaming thread has started");
> > +
> > +	guint group_id = gst_util_group_id_next();
> > +	for (GstPad *srcpad : state->srcpads) {
> > +		/* Create stream-id and push stream-start */
> > +		g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad,
> > GST_ELEMENT(self), nullptr);
> > +		GstEvent *event = gst_event_new_stream_start(stream_id);
> > +		gst_event_set_group_id(event, group_id);
> > +		gst_pad_push_event(srcpad, event);
> > +	}
> >  }
> >  
> >  static void
> >

Hi Nicolas,

Thank you for the patch.

On Tue, Jan 28, 2020 at 10:32:00PM -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> Prior to sending caps, we need to send a stream-start event. This requires
> generating a stream and a group id. The stream id is random for live sources and
> the group id is shared across all pads.

Apart from the GRecMutexLocker issue pointed out by Kieran,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index be32a11..b1a21dc 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -18,6 +18,8 @@ using namespace libcamera;
>  GST_DEBUG_CATEGORY_STATIC(source_debug);
>  #define GST_CAT_DEFAULT source_debug
>  
> +#define STREAM_LOCKER(obj) g_autoptr(GRecMutexLocker) stream_locker = g_rec_mutex_locker_new(&GST_LIBCAMERA_SRC(obj)->stream_lock)
> +
>  /* Used for C++ object with destructors */
>  struct GstLibcameraSrcState {
>  	std::shared_ptr<CameraManager> cm;
> @@ -127,9 +129,20 @@ gst_libcamera_src_task_run(gpointer user_data)
>  static void
>  gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)
>  {
> +	STREAM_LOCKER(user_data);
>  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> +	GstLibcameraSrcState *state = self->state;
>  
>  	GST_DEBUG_OBJECT(self, "Streaming thread has started");
> +
> +	guint group_id = gst_util_group_id_next();
> +	for (GstPad *srcpad : state->srcpads) {
> +		/* Create stream-id and push stream-start */
> +		g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr);
> +		GstEvent *event = gst_event_new_stream_start(stream_id);
> +		gst_event_set_group_id(event, group_id);
> +		gst_pad_push_event(srcpad, event);
> +	}
>  }
>  
>  static void