GNOME Bugzilla – Bug 617733
[wavparse] handle gst_pad_pull_range() returning less data than requested
Last modified: 2010-05-26 12:39:58 UTC
directsoundsink's ringbuffer was choking when parsing a wave file inside Opera on a slow network. The cause is that wavparse can push puffers that contain incomplete samples if gst_pad_pull_range returns less data than requested. It's not obvious from the documentation if gst_pad_pull_range is allowed to do this, but it has been my impression since my first implementation of Opera's internal source element, which pushes as much data as is available if the request is close to the current download position. (This is a network source so it should normally be a push source, but in practice seeking just isn't very well supported in push mode and we really need that. It has served us well.) If my assumptions are incorrect, then please make it very clear in gst_pad_pull_range and GstBaseSrcClass.create() that elements *must* return the requested amount or return GST_FLOW_ERROR, and add asserts to the surrounding core code ensuring that is actually what happens. I'm not sure how many source and demux-like elements make assumptions about this behavior.
Yes, gst_pad_pull_range() MUST return the requested number of bytes unless an EOF is encountered (in which case GST_FLOW_OK and less bytes are returned and on the next pull_range no buffer and a GST_FLOW_UNEXPECTED) or unless an error occured (in which case no buffer is returned and GST_FLOW_ERROR/WRONG_STATE/.. is returned).
Yeah, due to EOS it's hard to add assertions to the core for this. :/ However, buffers must always be correct wrt the caps they support, so audio buffers must contain complete samples at all times, so wavparse should not ever output half a sample.
OK, then this needs documenting and big red warning flags, as I have been under the wrong impression after looking at documentation and sample code some years ago. From time to time I've wondered if this is supposed to be this way, but never found the answer. (Surprisingly, it has actually works quite well to return less than requested, that's what happens in Opera 10.50 and today is the first time I've seen this issue.)
commit 6e4fde7195ec12495494c8270aa475b5ce824967 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Wed May 5 12:01:50 2010 +0200 docs: clarify the pull_range functions Clarify the gst_pad_pull_range(), GstBaseSrc::create(), gst_pad_get_range() and GstPadGetRange functions a little. Fixes #617733
Wavparse needs fixing to never output invalid output when upstream returns less samples (which can only be because of EOS on a truncated file).
Thanks, I'm fixing Opera's source element to respect this now.
Created attachment 161945 [details] [review] Handle truncated input data at EOS in pull mode
This should go into the release I suppose.
Ok, pushed: commit 3462eed7e05fb4465ac86f4e8f3ed928a296fa6b Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue May 25 15:34:11 2010 +0200 wavparse: handle truncated input data at EOS in pull mode Fixes #617733. diff --git a/gst/wavparse/gstwavparse.c b/gst/wavparse/gstwavparse.c index b7f6937..902edd7 100644 --- a/gst/wavparse/gstwavparse.c +++ b/gst/wavparse/gstwavparse.c @@ -1844,6 +1844,18 @@ iterate_adapter: if ((res = gst_pad_pull_range (wav->sinkpad, wav->offset, desired, &buf)) != GST_FLOW_OK) goto pull_error; + + /* we may get a short buffer at the end of the file */ + if (GST_BUFFER_SIZE (buf) < desired) { + GST_LOG_OBJECT (wav, "Got only %u bytes of data", GST_BUFFER_SIZE (buf)); + if (GST_BUFFER_SIZE (buf) >= wav->blockalign) { + buf = gst_buffer_make_metadata_writable (buf); + GST_BUFFER_SIZE (buf) -= (GST_BUFFER_SIZE (buf) % wav->blockalign); + } else { + gst_buffer_unref (buf); + goto found_eos; + } + } } Reflowed this a little though (to fix debug message, and avoid using 'desired' as temp variable here).