GNOME Bugzilla – Bug 752092
baseparse: Passes bogus buffer durations to subclass
Last modified: 2015-08-16 13:38:28 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.
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
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..
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.
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.
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.
Note that even before my change this was the case, just that it now at least happens consistently.
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.
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?
So what exactly is the difference between the buffers? Can you check?
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.
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.
Created attachment 307057 [details] [review] baseparse: put buffer in a correct state after gst_adapter_get_buffer call
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
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
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.