GNOME Bugzilla – Bug 614296
[basetransform] Try suggestion on bad pad alloc
Last modified: 2012-09-28 09:08:49 UTC
Created attachment 157415 [details] Test toy app When basetransform gets a pad alloc with a caps it doesn't support it simply rejects with not-negotiated. When dynamically switching pipeline elements, upstream might issue a pad_alloc to a basetransform element that was just added and linked in place of another element. This pad_alloc will probably have the same caps that was being used with the previous element and might not be supported by the newly added basetransform. I propose that basetransform instead of failing, suggested upstream a caps that they could agree and keep the pipeline flowing. I'll attach a test case in python.
Created attachment 157416 [details] [review] basetransform: Try suggesting caps on bad caps pad_alloc When basetransform received an unsupported caps on pad_alloc it just returned not-negotiated. This patch makes it query the allowed caps between his sinkpad and upstream's srcpad to find a caps to suggest. This happens when dinamically switching pipeline elements and upstream pad_allocs with the previous caps that was being used. Fixes #614296
I'm not sure about the fixating part of this patch, without using the previous caps fields I ended up picking framerate=0/1 in the default fixate function and that made the pipeline stop. Basetransform has its own fixate_caps function, but in this case we don't know what the other caps will be, so I don't know if we can use it. In any case, it can't get worse than failing.
+ while ((size = gst_caps_get_size (allowed)) > 1) + gst_caps_remove_structure (allowed, size - 1); You can use gst_caps_truncate() here, it does exactly that ;) Why can't we know the downstream caps in this case? Can't you simply call gst_pad_peer_get_caps() to get possible caps, intersect them, and do all the standard basetransform fixating magic?
Created attachment 157983 [details] [review] basetransform: Try suggesting caps on bad caps pad_alloc Updated with Sebastian's sugestions, but I still can't make it work without the 'fixate_from_last_caps' hack. It still picks 0 framerate with the default fixation procedure.
Any suggestions?
Can someone change the severity of this bug to normal? I think it is not just an enhancement, since the bug causes the pipeline to halt with a "not negotiated" error. The patch fixed the problem for me. If noone else has comments, maybe you can apply the patch with the hack and fix it when you someone has a better idea.
Setting it to Normal instead of Enhancement. We should get a fix for this, and we already figured out the problem and have a patch (a bit hacky, though).
Review of attachment 157983 [details] [review]: ::: libs/gst/base/gstbasetransform.c @@ +1654,3 @@ + /* do a little fixation of our own + * Ideally, if the pad has a fixate function, we should use it, + * but we don't have how to know */ The end of the comment sounds weird. @@ +1667,3 @@ + * renegotiation */ + gst_structure_foreach (gst_caps_get_structure (caps, 0), + fixate_from_last_caps, gst_caps_get_structure (allowed, 0)); So if you don't do this, you end up with e.g. framerate=0? Is the '0' framerate caused because behind the scenes gst_structure_fixate_field_nearest_int() is called? Would it make sense to call: allowed = gst_base_transform_find_transform(trans, pad. caps); instead of most of the code here.
(In reply to comment #8) > Review of attachment 157983 [details] [review]: > > ::: libs/gst/base/gstbasetransform.c > @@ +1654,3 @@ > + /* do a little fixation of our own > + * Ideally, if the pad has a fixate function, we should use it, > + * but we don't have how to know */ > > The end of the comment sounds weird. Fixed this. > > @@ +1667,3 @@ > + * renegotiation */ > + gst_structure_foreach (gst_caps_get_structure (caps, 0), > + fixate_from_last_caps, gst_caps_get_structure (allowed, 0)); > > So if you don't do this, you end up with e.g. framerate=0? > Is the '0' framerate caused because behind the scenes > gst_structure_fixate_field_nearest_int() is called? > > Would it make sense to call: > allowed = gst_base_transform_find_transform(trans, pad. caps); > instead of most of the code here. Not really, because you need a fixed caps on one size, and we don't have it.
Pushed the partial fix below. Module: gstreamer Branch: master Commit: 574e6ab423f0bba58c3bc88085d6c406677f76e4 URL: http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=574e6ab423f0bba58c3bc88085d6c406677f76e4 it is a simplification of the patch that doesn't do hackish fixation. The problem persists because it renegotiates to framerate=0, because videotestsrc allows that and the capsfilter isn't fixating it. With this patch and if you put a fixed framerate on the capsfilter it works. So we now need to find something to do about picking 0 as a framerate (or do nothing?). I have no good idea here.
Added a patch that makes basetransform handle these weird suggestions a little better. commit f8abf35000570a8afe64268b430e998e2d735e61 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Thu Aug 26 15:12:49 2010 +0200 basetransform: recover from invalid downstream suggestions When we are handling a buffer and need to allocate an output buffer, handle the case when downstream suggests us a format that we can't convert the input buffer to. In that case, check if there is another format available downstream instead of failing. Fixes #621332 and see also #614296
Is anything still needed here? This shouldn't be a problem anymore in 0.11
Probably safe to assume that noone's going to touch basetransform in 0.10 any more. Feel free to re-open if there's still something left to do.