GNOME Bugzilla – Bug 526768
Basetransform bufferalloc passing through too easily
Last modified: 2008-05-23 14:28:48 UTC
If the Basetransform child is in passthrough mode and gets an allocation request for a buffer with different caps it just passes this through. Which normall makes sense but not really when the klass has passthrough_on_same_caps enabled. A pratical example i've got a pipeline looking like customsrcbinthingy ! videoscale ! queue ! xvimagesink.. Under some circumstances the customsrcbinthingy changes it's resolution to something larger then the xvimagesink can handle. But the videoscale just passes through the buffer allocation for this, causing it to fail. Not passing through buffer allocation when the caps are different and passthrough_on_same_caps is used fixes this issue.. I'll attach a patch in a bit.
Created attachment 108797 [details] [review] proposed patch
Created attachment 109475 [details] [review] updated patch
The patch breaks the videocrop unit test: Running suite(s): videocrop 80%: Checks: 5, Failures: 1, Errors: 0 elements/videocrop.c:245:F:general:test_crop_to_1x1:0: Assertion 'buf != NULL' failed FAIL: elements/videocrop
Created attachment 110515 [details] [review] bufferalloc.diff What about this one? It fixes the unit test... hope it still fixes your test case ;)
Created attachment 110519 [details] [review] Testcase Add a testcase where the source of a audioresample element changes caps from something that can be passed through to something that can't, while playing. Fails if the basetransform bufferalloc function doesn't handle this correctly. It also makes the audioresample element use the normal buffer allocation stuff of the basetransform by not setting the buffer allocation function to NULL
bufferalloc.diff does indeed fix both the testcase i just added and the videocrop test. Good work! :)
2008-05-08 Sebastian Dröge <slomo@circular-chaos.org> Based on a patch by: Sjoerd Simons <sjoerd at luon dot net> * libs/gst/base/gstbasetransform.c: (gst_base_transform_buffer_alloc): Don't passthrough buffer allocation too easily if the caps change. This breaks when working in passthrough mode and upstream changes it's caps. Fixes bug #526768. 2008-05-08 Sebastian Dröge <slomo@circular-chaos.org> Patch by: Sjoerd Simons <sjoerd at luon dot net> * gst/audioresample/gstaudioresample.c: (gst_audioresample_init): Let audioresample use the buffer allocation of basetransform instead of it's own stuff. * tests/check/elements/audioresample.c: (alloc_only_48000), (GST_START_TEST), (audioresample_suite): Add unit test for the recent basetransform bugfix, where upstream changes caps to something that can't be passed through anymore.
This patch breaks playback in totem for me (avi, mpeg-4, ximagesink).
Created attachment 110676 [details] debug log Debug log - grep for not-negotiated.
Full debug log (no colours, sorry) can be found here: http://people.freedesktop.org/~tpm/526768.log.bz2 This is with a clean GStreamer checkout.
Created attachment 110727 [details] [review] fall back to the default allocater when we didn't get the caps we expected This happens because the not_configured case didn't cope when the buffer caps it requested and the one on the buffer it actually got differed. Should be fixed by this patch..
The patch makes things work for me once I do GST_BUFFER_CAPS (buf) => GST_BUFFER_CAPS (*buf) Would it be too much to ask for a unit test for this? :)
Created attachment 110754 [details] [review] Fix a caps dereference
Created attachment 110755 [details] [review] Updated testcase This adds a test asserting that when the base transform hits the not_configured case it will always return a buffer with the requested caps. Leaving the actual renegotiation to be done by other parts of the code.
Sjoerd, your updated unit test runs fine even without your new patch. It doesn't trigger the bug that Tim found it seems... Committed the other part though as it's correct and fixes Tim's bug: 2008-05-13 Sebastian Dröge <slomo@circular-chaos.org> Patch by: Sjoerd Simons <sjoerd at luon dot net> * libs/gst/base/gstbasetransform.c: (gst_base_transform_buffer_alloc): Check the caps of the buffer returned by gst_pad_alloc_buffer() and fall back to default negotiation in the chain function if the caps are different from what was requested. Fixes bug #526768.
No, the unit test was fine... committed too :)
res = gst_pad_alloc_buffer (trans->srcpad, offset, size, caps, buf); if (res == GST_FLOW_OK && !gst_caps_is_equal (caps, GST_BUFFER_CAPS (*buf))) { how can the caps on the resulting buffer not be the caps we passed to gst_pad_alloc_buffer()?
Comment #17: reverse negotiation.