GNOME Bugzilla – Bug 666174
[basetransform] causes segfaults because gst_pad_alloc_buffer_and_set_caps return a buffer with a size of 0
Last modified: 2011-12-19 11:42:20 UTC
Created attachment 203461 [details] flv video sample to reproduce the problem Steps to reproduce: - build the attached 100 lines code test.c (gcc -Wall test.c -o test $(pkg-config --cflags --libs gstreamer-0.10) - download the attached sample_small.flv file - run./test file:///home/julien/Videos/flv/sample_small.flv Actual result: Segmentation fault: because gst_pad_alloc_buffer_and_set_caps (gstffmpegdec:line 958) return a buffer with size of 0 but the return code is GST_FLOW_OK so alloc_output_buffer (gstffmpegdec:line 1617) does not fail. Finally, av_picture_copy (gstffmpegdec:line 1634) just crashes. Other informations: Maybe tt's similar to the recent segfault in a52dec (see: https://bugzilla.gnome.org/show_bug.cgi?id=665989) but I think this time gstffmpegdec is supposed to handle renegociation) I just noticed that you can reproduce the problem using gst-launch: gst-launch-0.10 uridecodebin uri=file:///home/julien/Videos/flv/sample_small.flv ! "video/x-raw-yuv, format=(fourcc)I420" ! xvimagesink
Created attachment 203462 [details] uni test to reproduce the problem
I see, this (and probably the a52dec crash) isn't simply fixed by using gst_pad_use_fixed_caps() on the srcpad because capsfilter will still allocate the first buffer itself now after one of the capsfilter changes recently. a52dec and ffdec must check the size of the buffer in any case, checking the caps is not required though. Want to provide a patch? If the size is < the expected size a new buffer should be allocated manually.
go ahead because I am not sure to understand why it'll work "manually" (As far as I understand, it's a 0.10 behavior that gst_pad_alloc_buffer_and_set_caps can return both GST_FLOW_OK and a buffer with a size of 0. The user has to handle this. But what's the difference with allocating the buffer manually) I'll see in your patch.
(In reply to comment #2) > I see, this (and probably the a52dec crash) isn't simply fixed by using > gst_pad_use_fixed_caps() on the srcpad because capsfilter will still allocate > the first buffer itself now after one of the capsfilter changes recently. While this may still be in 0.10 API specifications, capsfilter persistently coming up with a size 0 buffer could surprise and crash a whole lot of (fixed src pad caps) elements out there (that usually do not check for the size of the returned buffer). In particular, it might be wondered if the checks in core could not be tightened instead (and provide fallback allocation) since no element could do anything useful with a size 0 buffer (and certainly not fixed caps ones which are not interested in caps either).
Yes, I don't think it's acceptable for downstream to return a buffer for the requested caps but with a smaller size.
FTR, "recent capsfilter change" mentioned above is (afaics) the following: commit 341d7a4c0dbd69f86faaf1ffd2e94e99bac6f8c9 Author: Sjoerd Simons <sjoerd.simons@collabora.co.uk> Date: Wed Jul 20 14:05:27 2011 +0200 capsfilter: don't assume _get_caps still has to be _set_caps only gets called when the buffer is actually pushed, so there is a reasonably big window between when the initial caps are retrieved and when the caps are set on our src pad. So we can't assume the not having negotiated caps on our src pad means _get_caps still has to be called. Instead simply always suggest the new caps on buffer_alloc.
Created attachment 203853 [details] [review] basetransform: suggestion compatible with upstream is not much of a suggestion Problem with current suggest mechanics in basetransform seems to be that it bypasses the core test in a silly way. That is, where core tests for caps == newcaps as a check for renegotiation, it is well possible to have newcaps equivalent/compatible (but obviously different pointer) and as such to slip by. Attached patch modifies some suggestion mechanics in basetransform to ignore the suggestion if suggested caps are compatible with (i.e. can intersect) with upstream requested caps (rather than having to be exactly equal). Not sure about original semantics, but seems to make sense that the suggestion could/should hardly coax upstream into doing something "new" if it is already doing stuff that is compatible with the suggestion. So, for a typical fixed src pad scenario that would then mean the suggested stuff is either compatible and essentially ignored (thereby fixing this), or it is not compatible and would run into some failure to negotiate (arranged for by _alloc_and_set_caps).
commit b3886b793544664de93e23286109dfb21ba0f5eb Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Mon Dec 19 12:33:18 2011 +0100 basetransform: suggestion compatible with upstream is not much of a suggestion ... in that upstream is already complying with that suggestion. Fixes #666174. and in 0.10 release branch: commit 5209cdd4d60b19345d3161ba8e2e6067e9008214 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Mon Dec 19 12:33:18 2011 +0100 basetransform: suggestion compatible with upstream is not much of a suggestion ... in that upstream is already complying with that suggestion. Fixes #666174.