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 685213 - rtph264pay: pushing unmapped data.
rtph264pay: pushing unmapped data.
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal blocker
: 1.0.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-10-01 13:10 UTC by Patricia Muscalu
Modified: 2012-10-04 08:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
do not push unmapped data (9.76 KB, patch)
2012-10-01 13:18 UTC, Patricia Muscalu
needs-work Details | Review
updated patch rtph264pay: do not push unmapped data. (13.47 KB, patch)
2012-10-02 19:54 UTC, Olivier Crête
committed Details | Review

Description Patricia Muscalu 2012-10-01 13:10:04 UTC
The payloader calls gst_buffer_map (in _handle_buffer()) and creates an RTSP buffer with the payload that points into this mapped area (in _payload_nal()). The buffer is added to the buffer list and the mapped area is released after the buffer list has been pushed.  
Result: the sink tries to render the data that is not necessary valid anymore.
Comment 1 Patricia Muscalu 2012-10-01 13:18:02 UTC
Created attachment 225486 [details] [review]
do not push unmapped data
Comment 2 Olivier Crête 2012-10-01 20:33:51 UTC
Review of attachment 225486 [details] [review]:

::: gst/rtp/gstrtph264pay.c
@@ +949,3 @@
+      gst_buffer_append_memory (outbuf,
+          gst_memory_new_wrapped (GST_MEMORY_FLAG_READONLY, (guint8 *) data,
+              size, 0, size, NULL, NULL));

I don't think using  gst_memory_new_wrapped () here is correct either, as it relies on the buffer staying in the payloader, it should probably also be sub-buffered.

@@ +951,3 @@
+              size, 0, size, NULL, NULL));
+    else
+      outbuf = gst_buffer_append (outbuf, paybuf);

Here it eats a ref to the buffer...

@@ +1026,3 @@
+        sub =
+            gst_buffer_copy_region (paybuf, GST_BUFFER_COPY_MEMORY, pos,
+            limitedSize);

But here it doesn't eat the ref, so I guess paybuf is leaked
Comment 3 Patricia Muscalu 2012-10-02 07:26:16 UTC
Yes, I agree that using gst_memory_new_wrapped () is not correct. I do not really know how to solve it: in that particular case we do not have any buffer.
Should gst_memory_copy () be used instead?

paybuf is unreffed at line 1044. Am I missing something here?
Comment 4 Olivier Crête 2012-10-02 19:54:00 UTC
Created attachment 225614 [details] [review]
updated patch rtph264pay: do not push unmapped data.

I modified it a bit to also create subbuffers in other cases by using the adapter
correctly.

Also do not use a GstBuffer after it has been pushed into the adapter
Comment 5 Olivier Crête 2012-10-02 19:54:59 UTC
I think my updated patch is correct in all cases, can some please double-check it and we can put it in 1.0.1.
Comment 6 Tim-Philipp Müller 2012-10-03 11:20:08 UTC
Patricia: does Olivier's patch work fine for you as well?
Comment 7 Patricia Muscalu 2012-10-04 07:56:39 UTC
The patch works fine. Thank you!
Comment 8 Tim-Philipp Müller 2012-10-04 08:27:38 UTC
Thanks

 commit 7a863e4d8d52344dda95434bc2e9e0b427eab6b0
 Author: Patricia Muscalu <patricia@axis.com>
 Date:   Mon Oct 1 15:11:05 2012 +0200

    rtph264pay: do not push unmapped data
    
    Also do not use a GstBuffer after it has been pushed into the adapter.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=685213