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 752092 - baseparse: Passes bogus buffer durations to subclass
baseparse: Passes bogus buffer durations to subclass
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-07-07 18:06 UTC by Arnaud Vrac
Modified: 2015-08-16 13:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to fix get buffer issue (1.31 KB, patch)
2015-07-08 05:24 UTC, Vineeth
needs-work Details | Review
baseparse: put buffer in a correct state after gst_adapter_get_buffer call (1.28 KB, patch)
2015-07-08 10:05 UTC, Arnaud Vrac
committed Details | Review

Description Arnaud Vrac 2015-07-07 18:06:30 UTC
The following file played fine in gstreamer 1.4.3, but after updating to git master, audio now skips when playing in push mode:

gst-play-1.0  http://absolut.zogzog.org/share/samples/avi/s6e24%20Coming%20Out%2c%20Getting%20Out%2c%20Going%20Out.avi

I also have another file exhibiting the same bug if needed.
Comment 1 Arnaud Vrac 2015-07-07 18:20:11 UTC
Actually the output on the avidemux audio pad is identical between gstreamer 1.4 and 1.5, so the bug might not be in avidemux.

I've compared the output of the following pipeline:

gst-launch-1.0 -v http://... ! avidemux .audio_0 ! fakesink silent=false
Comment 2 Vineeth 2015-07-08 01:46:56 UTC
This is happening due to the below patch
http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=c3bcbadd5452d5b3450f70e49dad3e64f14de00a

If i revert this and check, then there is no audio skip..
Comment 3 Vineeth 2015-07-08 04:18:19 UTC
doing some analysis on this.. These are my observations..
The above commit replaces gst_buffer_new_wrapped_full 
with gst_adapter_get_buffer, which is supposed to do the same as the previous..

The main problem seems to happen in gst_adapter_get_buffer(), the below piece of code.

  if (skip == 0 && hsize == nbytes) {
    GST_LOG_OBJECT (adapter, "providing buffer of %" G_GSIZE_FORMAT " bytes"
        " as head buffer", nbytes);
    buffer = gst_buffer_ref (cur);
    goto done;
  }

just doing a ref of the cur buffer is not working properly.. But if i just extract the data and use the same buffer, then there is no issue. Like the below piece of code...

data = g_malloc (nbytes);
gst_buffer_extract (cur, skip, data, nbytes);
buffer = gst_buffer_new_wrapped (data, nbytes);



strangely even if i do copy of buffer "buffer = gst_buffer_copy (cur);", there is same issue.
I was thinking copy should work same as the above 3 statements. But something seems to be missing.
Comment 4 Vineeth 2015-07-08 05:24:23 UTC
Created attachment 307050 [details] [review]
patch to fix get buffer issue

Even with deep copy of buffer, the issue still exists.

Hence replacing the same by extracting the data from the existing buffer and creating a new buffer using the data.

This is anyways being done for most of the conditions..

But is there a better way to fix this?
i am still not sure why buffer_copy does not work properly.
Comment 5 Sebastian Dröge (slomo) 2015-07-08 08:16:20 UTC
Review of attachment 307050 [details] [review]:

::: libs/gst/base/gstadapter.c
@@ +936,3 @@
+    data = g_malloc (hsize);
+    gst_buffer_extract (cur, skip, data, hsize);
+    buffer = gst_buffer_new_wrapped (data, hsize);

The difference here is that your version is a new buffer, the old code is the old buffer with the old metadata (timestamp, duration, GstMeta).

To me this looks like avidemux or whatever does not consider the GstAdapter documentation: the buffers returned can have arbitrary metadata set on them, and it's the caller's job to make sure that it is correct.
Comment 6 Sebastian Dröge (slomo) 2015-07-08 08:17:55 UTC
Note that even before my change this was the case, just that it now at least happens consistently.
Comment 7 Arnaud Vrac 2015-07-08 09:01:19 UTC
avidemux outputs exactly the same on the audio pad in 1.4.3 and git master, the difference seems to be in the mpegaudioparse output.
Comment 8 Vineeth 2015-07-08 09:02:44 UTC
In cases other than the if and else if condition, anways we are creating a new buffer right and copying the metadata as well to the buffer i guess..

If i skip the if condition and let the code below it handle the buffer creation and copy, even then there is no issue..

I mean there we are still copying all the metadata from the old buffer to the new buffer right?
Comment 9 Sebastian Dröge (slomo) 2015-07-08 09:17:26 UTC
So what exactly is the difference between the buffers? Can you check?
Comment 10 Arnaud Vrac 2015-07-08 09:22:13 UTC
Setting the buffer duration to GST_CLOCK_TIME_NONE after the gst_adapter_get_buffer fixes the issue. Baseparse probably uses the upstream duration instead of using the frame duration, which doesn't happen if the duration is reset.
Comment 11 Sebastian Dröge (slomo) 2015-07-08 09:32:55 UTC
Problem is that before setting the duration, you also need to ensure that the buffer is actually writable with gst_buffer_make_writable(). But that indeed makes sense, yes.
Comment 12 Arnaud Vrac 2015-07-08 10:05:42 UTC
Created attachment 307057 [details] [review]
baseparse: put buffer in a correct state after gst_adapter_get_buffer call
Comment 13 Sebastian Dröge (slomo) 2015-07-08 10:33:58 UTC
commit ea8cabe084e9f287ebe808c16ba4761c086391c6
Author: Arnaud Vrac <avrac@freebox.fr>
Date:   Wed Jul 8 12:00:56 2015 +0200

    baseparse: put buffer in a correct state after gst_adapter_get_buffer call
    
    We must make the buffer writable to write its PTS and DTS, and also
    reset its duration.
    
    The behaviour is now the same as before commit c3bcbadd, except metas
    might still be attached to the buffer extracted from the adapter.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752092
Comment 14 Vineeth 2015-07-09 00:59:40 UTC
This change is ok..


But my concern is, when we get buffer from adapter,
dont we need to get proper duration for all conditions? or rather all other buffer values?
base parse does not need proper duration. So it is fine i guess..

But
  data = gst_adapter_get_internal (adapter, nbytes);

  buffer = gst_buffer_new_wrapped (data, nbytes);
is not copying all the buffer values i guess right?

i mean how else was the duration was coming as some finite value, when we just ref the buffer and return
and for other conditions, we are creating a new buffer and all the values are not copied i guess. Duration was coming as GST_CLOCK_TIME_NONE. So guessing none of the other values are being copied to the new buffer?

Should we copy all the values to the new buffer similar to how we are copying metadata?

Not sure if i am making sense :D
Comment 15 Sebastian Dröge (slomo) 2015-07-09 14:15:00 UTC
gst_buffer_new_wrapped() just wraps the raw memory, it does not take over any timestamp, duration or other metadata. gst_buffer_ref(), gst_buffer_copy(), etc are not modifying any of these things.

So what we got here was the duration that upstream for whatever reason put on the buffers.