After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 794794 - rtpssrcdemux: add the support of buffer list
rtpssrcdemux: add the support of buffer list
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-03-29 07:07 UTC by Eloi BAIL
Modified: 2018-11-03 15:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
first version of the patch adding the buffer list support only for RTP streams (6.70 KB, patch)
2018-03-29 08:42 UTC, Eloi BAIL
none Details | Review
fix the leak in rtpssrcdemux (1.33 KB, patch)
2018-03-29 11:58 UTC, Eloi BAIL
none Details | Review
fix Olivier's comments (6.12 KB, patch)
2018-03-30 13:14 UTC, Eloi BAIL
none Details | Review
Use of flowcombiner (3.14 KB, patch)
2018-03-30 13:14 UTC, Eloi BAIL
none Details | Review
squashed patch (8.11 KB, patch)
2018-04-03 09:25 UTC, Eloi BAIL
none Details | Review

Description Eloi BAIL 2018-03-29 07:07:26 UTC
Add the support of buffer list in rtpssrcdemux so that we can handle buffer list on sink pad.
Each buffer list on the sink pad may contain different RTP SSRC. For each SSRC, a different buffer list will be created and buffers matching this SSRC will be added.
Those buffer lists will be then pushed on the src pad.
Comment 1 Eloi BAIL 2018-03-29 08:42:50 UTC
Created attachment 370285 [details] [review]
first version of the patch adding the buffer list support only for RTP streams
Comment 2 Eloi BAIL 2018-03-29 09:34:19 UTC
I have a test case:
appsrc->rtpssrcdemux->queue0->fakesink0

If I use buffer list in appsrc rather than single buffers, I notice a leak.

Here is the way I am handling buffers in my app:

----
         gst_buf = gst_buffer_new_and_alloc (packet_datalen);                    
         gst_buffer_map (gst_buf, &gst_map, GST_MAP_WRITE);                      
         memcpy (gst_map.data, ptr_packet, packet_datalen);     
         gst_buffer_unmap (gst_buf, &gst_map);                                   
                                                                                 
         if (gst_buf_list == NULL)                                               
             gst_buf_list = gst_buffer_list_new();
                               
         gst_buffer_list_add(gst_buf_list, gst_buf);                             
                                                                                 
         if (gst_buffer_list_length(gst_buf_list) >= BUF_LIST_MAX)) {            
             gst_app_src_push_buffer_list (
                  (GstAppSrc*)data.source, gst_buf_list);
             gst_buf_list = NULL;                                                
         }                                                                       
----

Even with BUF_LIST_MAX to 1, I notice a leak.
When gst_app_src_push_buffer_list is called, the ownership of the list is taken. So I am wondering what is wrong.
Comment 3 Eloi BAIL 2018-03-29 11:58:35 UTC
Created attachment 370290 [details] [review]
fix the leak in rtpssrcdemux
Comment 4 Olivier Crête 2018-03-29 22:36:32 UTC
Review of attachment 370285 [details] [review]:

::: gst/rtpmanager/gstrtpssrcdemux.c
@@ +159,3 @@
+  GstPad *src_pad;
+  GstBufferList *buffer_list;
+};

This seems duplicative from the GstRtpSsrcDemuxPad struct, you can just put the GstBufferList* in there I think.

@@ +181,3 @@
+{
+  GstRtpSsrcBufferList *elem_buf_list;
+  elem_buf_list = g_new0 (GstRtpSsrcBufferList, 1);

Maybe g_slice_new0()

@@ +222,3 @@
+  for (walk = demux->ssrc_buffer_list; walk; walk = g_slist_next (walk)) {
+    GstRtpSsrcBufferList *list = (GstRtpSsrcBufferList *) walk->data;
+    GST_INFO_OBJECT (demux, "SSRC %08x: pushing %u buffers", list->ssrc,

This should be at the LOG level, it's done for every buffer list

@@ +224,3 @@
+    GST_INFO_OBJECT (demux, "SSRC %08x: pushing %u buffers", list->ssrc,
+        gst_buffer_list_length (list->buffer_list));
+    gst_pad_push_list (list->src_pad, list->buffer_list);

You need to aggregate the return values and return that to the caller, use GstFlowCombiner for that

@@ +756,3 @@
+
+  push_all_buffer_list (demux);
+  return ((ret == TRUE) ? GST_FLOW_OK : GST_FLOW_ERROR);

You need to return the aggregated value.

@@ +767,3 @@
+  demux = GST_RTP_SSRC_DEMUX (parent);
+  ret = gst_rtp_ssrc_demux_handle_buffer (demux, buf, FALSE);
+  return ret;

You can do this in one line

return gst_rtp_ssrc_demux_handle_buffer (GST_RTP_SSRC_DEMUX(parent), buf, FALSE);

::: gst/rtpmanager/gstrtpssrcdemux.h
@@ +43,3 @@
   GRecMutex padlock;
   GSList *srcpads;
+  GSList *ssrc_buffer_list;

Don't put this in the struct, you can just pass it in every call somehow.

You could instead put it in the GstRtpSsrcDemuxPad struct or something like that.
Comment 5 Olivier Crête 2018-03-29 22:37:32 UTC
Review of attachment 370290 [details] [review]:

::: gst/rtpmanager/gstrtpssrcdemux.c
@@ +698,3 @@
     ret = gst_pad_push (srcpad, buf);
   else
+    ret = add_to_ssrc_buffer_list (demux, ssrc, srcpad, gst_buffer_ref (buf));

Why are you taking an extra ref here ? Won't a ref be taken when it's added to the new bufferlist?
Comment 6 Eloi BAIL 2018-03-30 13:12:28 UTC
Review of attachment 370285 [details] [review]:

::: gst/rtpmanager/gstrtpssrcdemux.c
@@ +159,3 @@
+  GstPad *src_pad;
+  GstBufferList *buffer_list;
+};

yes. Please see my fix in 0002-fixup-rtpssrcdemux-add-the-support-of-buffer-list.patch

@@ +181,3 @@
+{
+  GstRtpSsrcBufferList *elem_buf_list;
+  elem_buf_list = g_new0 (GstRtpSsrcBufferList, 1);

This code is now removed because GstRtpSsrcDemuxPad struct is used

@@ +222,3 @@
+  for (walk = demux->ssrc_buffer_list; walk; walk = g_slist_next (walk)) {
+    GstRtpSsrcBufferList *list = (GstRtpSsrcBufferList *) walk->data;
+    GST_INFO_OBJECT (demux, "SSRC %08x: pushing %u buffers", list->ssrc,

please see the fix in 0002-fixup-rtpssrcdemux-add-the-support-of-buffer-list.patch

@@ +224,3 @@
+    GST_INFO_OBJECT (demux, "SSRC %08x: pushing %u buffers", list->ssrc,
+        gst_buffer_list_length (list->buffer_list));
+    gst_pad_push_list (list->src_pad, list->buffer_list);

please see the fix in 0004-rtpssrcdemux-use-flowcombiner-for-gst_pad_push_list.patch

@@ +756,3 @@
+
+  push_all_buffer_list (demux);
+  return ((ret == TRUE) ? GST_FLOW_OK : GST_FLOW_ERROR);

please see the fix in 0002-fixup-rtpssrcdemux-add-the-support-of-buffer-list.patch

@@ +767,3 @@
+  demux = GST_RTP_SSRC_DEMUX (parent);
+  ret = gst_rtp_ssrc_demux_handle_buffer (demux, buf, FALSE);
+  return ret;

fixed in 0002-fixup-rtpssrcdemux-add-the-support-of-buffer-list.patch

::: gst/rtpmanager/gstrtpssrcdemux.h
@@ +43,3 @@
   GRecMutex padlock;
   GSList *srcpads;
+  GSList *ssrc_buffer_list;

it is now in GstRtpSsrcDemuxPad. See 0002-fixup-rtpssrcdemux-add-the-support-of-buffer-list.patch
Comment 7 Eloi BAIL 2018-03-30 13:14:06 UTC
Created attachment 370342 [details] [review]
fix Olivier's comments
Comment 8 Eloi BAIL 2018-03-30 13:14:36 UTC
Created attachment 370343 [details] [review]
Use of flowcombiner
Comment 9 Eloi BAIL 2018-03-30 13:18:45 UTC
Review of attachment 370290 [details] [review]:

::: gst/rtpmanager/gstrtpssrcdemux.c
@@ +698,3 @@
     ret = gst_pad_push (srcpad, buf);
   else
+    ret = add_to_ssrc_buffer_list (demux, ssrc, srcpad, gst_buffer_ref (buf));

buf will be actually moved from the buffer list received on the sink pad to the buffer list sent to a ssrc pad (the one for the SSRC).
Once the buffer is moved to the ssrc list, the buffer list of the sink pad is unref.
So I would believe that buffers would be released. Am I misunderstanding something ?
Comment 10 Olivier Crête 2018-04-02 17:42:19 UTC
Review of attachment 370342 [details] [review]:

Can you also squash all the patches, it's really hard to work with fixup patches with bugzilla.

::: gst/rtpmanager/gstrtpssrcdemux.c
@@ +183,1 @@
+      gst_buffer_list_add (demuxpad->buffer_list, buf);

This takes a reference on buf.

@@ +207,3 @@
+      GST_WARNING_OBJECT (demux, "failing to push data on ssrc %08x src pad",
+          demuxpad->ssrc);
+      return GST_FLOW_ERROR;

That's probably GST_FLOW_FLUSHING if the pad is not active. If you were to return GST_FLOW_ERROR, you also want to post a message on the bus with GST_ELEMENT_ERROR()

@@ +737,3 @@
 
+  if (!gst_buffer_list_foreach (buf_list, process_buffer_list_item, demux))
+    return GST_FLOW_ERROR;

If you really want to return GST_FLOW_ERROR, you need to post an error on the bus, but I don't think you want to do that here.
Comment 11 Eloi BAIL 2018-04-03 09:23:47 UTC
Review of attachment 370342 [details] [review]:

Sure, I will squash all my changes.

::: gst/rtpmanager/gstrtpssrcdemux.c
@@ +183,1 @@
+      gst_buffer_list_add (demuxpad->buffer_list, buf);

Ok it is not so clear to me. If I remove gst_buffer_ref, I need also to remove the call to gst_buffer_list_unref.
Otherwise, I see lots of "GStreamer-CRITICAL **: gst_mini_object_unref: assertion 'GST_MINI_OBJECT_REFCOUNT_VALUE (mini_object) > 0' failed" error messages.

@@ +207,3 @@
+      GST_WARNING_OBJECT (demux, "failing to push data on ssrc %08x src pad",
+          demuxpad->ssrc);
+      return GST_FLOW_ERROR;

ok I will return GST_FLOW_FLUSHING

@@ +737,3 @@
 
+  if (!gst_buffer_list_foreach (buf_list, process_buffer_list_item, demux))
+    return GST_FLOW_ERROR;

ok according to other elements, this case is never handled. I would then ignore the return value
Comment 12 Eloi BAIL 2018-04-03 09:25:02 UTC
Created attachment 370476 [details] [review]
squashed patch
Comment 13 GStreamer system administrator 2018-11-03 15:27:45 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/455.