GNOME Bugzilla – Bug 662199
[capsfilter] behavior has changed
Last modified: 2011-12-08 17:15:02 UTC
Hi, * problem: Since commit 341d7a4c0dbd69f86faaf1ffd2e94e99bac6f8c9 ( http://cgit.freedesktop.org/gstreamer/gstreamer/commit/plugins/elements/gstcapsfilter.c?id=341d7a4c0dbd69f86faaf1ffd2e94e99bac6f8c9 ) the behavior changed for some of my applications. Now I get a lot of warning like: GStreamer-WARNING **: src accepted caps video/x-raw-yuv, format=(fourcc)I420, width=(int)1024, height=(int)576, interlaced=(boolean)false, framerate=(fraction)5000000/208333 although they are not a subset of its caps video/x-raw-yuv, width=(int)1024, height=(int)576, framerate=(fraction)5000000/208333, format=(fourcc)I420, interlaced=(boolean)false, pixel-aspect-ratio=(fraction)1/1 that leading to random result. Whereas without the commit pointed above, I do not have those problems. * more infos: I understand the goal of the commit and I am agree with that but I think some work is missing or maybe this is a regression in gstbasetransform.c. I have not a minimal test to reproduce the problem. Do have I to make one ? I think yes, but maybe someone else has already encountered those warnings ? Sincerely Julien
Marking as blocker for now, to make sure we look at this before any release.
Related to 659606, maybe a dupe if it's due to the race condition slomo is talking about.
These warning does not change any behaviour and is only informational... and is racy anyway and should be removed for the next release, even if the original rationale for it is correct. But because this warning is only informational and does not change behaviour there seems to be a real bug with the capsfilter commit. We would need a way to reproduce it though
Created attachment 199526 [details] minimal test to reproduce the problem The attached test (135 lines) allows to reproduce the warning. Moreover the pipeline fails to play. 100% reproductible here and the output is: (test:8547): GStreamer-WARNING **: pad ximagesink0:sink accepted caps video/x-raw-rgb although they are not a subset of its caps video/x-raw-rgb, bpp=(int)32, depth=(int)24, endianness=(int)4321, red_mask=(int)65280, green_mask=(int)16711680, blue_mask=(int)-16777216, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)[ 0/1, 2147483647/1 ], pixel-aspect-ratio=(fraction)1/1 ** (test:8547): CRITICAL **: gst_ffmpeg_caps_to_pixfmt: assertion `ret == TRUE' failed ** (test:8547): WARNING **: ffmpegcsp0: could not get out_size Error: Internal data flow error Returned, stopping playback Deleting pipeline I think the same problem could be obtain for audio (by using audioconvert instead of colorspace ...)
Hi, Is someone able to reproduce the problem with the minimal test attached to this bug ? Sincerely Julien
Does it still happen if you revert 341d7a4c0dbd69f86faaf1ffd2e94e99bac6f8c9 in core ?
Sure it does not happen if I revert 341d7a4c0dbd69f86faaf1ffd2e94e99bac6f8c9 (if yes it would not be the minimal test to reproduce the problem) I tested again this morning with latest gstreamer and plugins from git.
Gah, for some reason I thought you'd referred to the commit that added those warning messages, sorry.
*** Bug 662871 has been marked as a duplicate of this bug. ***
Hi, I do not know if there are several problems here, but with my minimal test I attached to this bug (see #4) you cannot play any video (warning or not).
Do you need more informations ?
Not at the moment, thanks, I see where the bug is.
Any progress on this?
No, I'm not sure how to fix it and haven't looked at it since.
You said that you see where the bug is. Could you describe what is causing the bug and how the behaviour changed? :)
Hmm. The details are a bit fuzzy now. IIRC the capsfilter (I think it was the capsfilter, or basetransform) is accepting caps which it should not, and a subsequent pad alloc gets to use the suggested caps introduced by Sjoerd's patch, which carry a size of 0, instead of the correct size.
Let's subscribe Sjoerd to this bug then :) Maybe he knows something
Ok, some details now finally :) The problem here is, that capsfilter gets video/x-raw-rgb as caps and pad_alloc on the capsfilter returns a buffer with exactly these caps for the first buffer after Sjoerd's change. This zero-sized buffer of size 0 is then confusing ffmpegcolorspace, which is understandable. It can't calculate the real size of buffers with these caps because the caps are simply wrong. This happens after Sjoerd's change because the caps are suggested to basetransform immediately although no buffer flow has happened yet. In gst_base_transform_buffer_alloc() this will cause the first buffer to be allocated with the suggested caps then, the suggested caps are then taken as is (in gst_base_transform_find_transform()) and not intersected with the peer caps because they're fixed and then returned immediately on the buffer. Without Sjoerd's change there would be no suggested caps and the correct caps would be sorted out during the gst_base_transform_transform_caps() calls, which then takes the peercaps and everything into account. Not sure how to fix this though, maybe find_transform() should always intersect with the peercaps, even if the caps are already fixed.
Another problem here is also, that xvimagesink accepts video/x-raw-rgb as caps instead of requiring all the necessary fields to be set.
commit e6d2da7cf85c6fdd6b1c458d8a2c526246060051 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Wed Nov 30 13:59:46 2011 +0100 basetransform: Always intersect the suggested sink caps with the peer caps This makes sure that we get correct and complete caps. The suggested caps could be incomplete, e.g. video/x-raw-rgb without any fields, and by intersecting with the peer caps we get something usable. Fixes bug #662199.
Created attachment 202919 [details] [review] patch to fix the Sebastian's fix ( commit e6d2da7cf85c6fdd6b1c458d8a2c526246060051 ) There is still a problem with the Sebastian's patch when reconfiguring the element. Handle the case where intersected caps is empty.
Hm, if the intersected caps are empty negotiation will fail later anyway.
Hi, in my patch, if "intersect" is empty then do not execute "sink_suggest = intersect"
I see, yes this code should ignore the sink suggestion if it's not compatible with the peer caps or our own template caps. I've done this a bit different and more consistent with the intersection with the template caps and also added some explaining comments. commit 7fb67e9d6fb5eadc8423c45cce44f3a355c8ccd7 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Wed Dec 7 09:50:40 2011 +0100 basetransform: If suggested caps are not compatible with upstream try to come up with compatible caps Fixes bug #662199.
Hi, thanks for those patches. The last one has a different result than mine. With yours, in case of renegociation, sink_suggest is totally lost whereas it should be taken into account (even if intersect is empty). For example, if the number of audio channels changed (and other stuffs in the caps are the same)
How would upstream take the buffer into account if the caps are incompatible anyway?
My usecase is using audioconvert. Output of audioconvert is forced to 2 nb audio channels. Input can be 2 or 6 nb audio channels.
Some more unit tests would be nice..
(In reply to comment #27) > My usecase is using audioconvert. Output of audioconvert is forced to 2 nb > audio channels. Input can be 2 or 6 nb audio channels. Ok, but then get_caps() on the input srcpad should give 2 and 6 channels. What's your complete pipeline?
Is gst-launch-0.10 audiotestsrc ! "audio/x-raw-int, channels=6" ! audioconvert ! \ "audio/x-raw-int, channels=2" ! fakesink supposed to work ? Because it's actually not working. (Whereas gst-launch-0.10 audiotestsrc ! "audio/x-raw-int, channels=2" ! audioconvert ! \ "audio/x-raw-int, channels=6" ! fakesink is working) Then, then first usecase is: gst-launch-0.10 audiotestsrc ! "audio/x-raw-int, channels=2" ! queue ! funnel \ name=f ! audioconvert ! "audio/x-raw-int, channels=2" ! fakesink audiotestsrc ! \ "audio/x-raw-int, channels=6" ! audioconvert ! queue ! f.
> Is > gst-launch-0.10 audiotestsrc ! "audio/x-raw-int, channels=6" ! audioconvert ! \ > "audio/x-raw-int, channels=2" ! fakesink > supposed to work ? Because it's actually not working. Not entirely surprising given that audiotestsrc only supports mono and stereo at the moment.
oh sorry. Try the following: gst-launch-0.10 audiotestsrc ! "audio/x-raw-int, channels=2" ! queue ! funnel name=f \ ! audioconvert ! "audio/x-raw-int, channels=2" ! fakesink audiotestsrc ! \ "audio/x-raw-int, channels=1" ! queue ! f. Result: ** (gst-launch-0.10:1290): CRITICAL **: gst_audio_convert_fixate_caps: assertion `gst_caps_is_fixed (caps)' failed and streaming task paused, reason not-negotiated (-4)
What should this pipeline do? Anyway, this looks like a bug in the code that was already there before. ::fixate_caps() must be called with fixed caps for the first caps parameter while it isn't and usually the allowed caps of the srcpad will not be fixed. This code looks wrong on multiple levels
(In reply to comment #33) > What should this pipeline do? > Sure this is not a usefull pipeline (except to reproduce the problem). The usfull usecase is the following (but no command line): My audio source is retrieving audio from TV. The pipeline starts, we are on a TV channel where audio is stereo then we zap and audio is 5.1 then zap and audio is back to stereo etc... So the audio buffer that come from my audio source are sometimes channels=2 and sometimes channels=6, for a same run. And I am recording the whole audio into an asf or something else. We are doing a lot of other stuffs but I do not mention them to be simple as possible. > > Anyway, this looks like a bug in the code that was already there before. > ::fixate_caps() must be called with fixed caps for the first caps parameter > while it isn't and usually the allowed caps of the srcpad will not be fixed. > This code looks wrong on multiple levels Which code exactly ?
(In reply to comment #34) > (In reply to comment #33) > > What should this pipeline do? > > > > Sure this is not a usefull pipeline (except to reproduce the problem). The > usfull usecase is the following (but no command line): > > My audio source is retrieving audio from TV. The pipeline starts, we are on a > TV channel where audio is stereo then we zap and audio is 5.1 then zap and > audio is back to stereo etc... > So the audio buffer that come from my audio source are sometimes channels=2 and > sometimes channels=6, for a same run. Ok, but that's something completely different than this pipeline (which I don't see why it would work at all). In your case you have a source that sometimes generates 2 and sometimes 6 channels. Then comes an audioconvert and then you seem to have a capsfilter, which forces 2 channels. Correct? In that case the current code should work, if it doesn't could you provide a testcase that has the same behaviour? > > Anyway, this looks like a bug in the code that was already there before. > > ::fixate_caps() must be called with fixed caps for the first caps parameter > > while it isn't and usually the allowed caps of the srcpad will not be fixed. > > This code looks wrong on multiple levels > > Which code exactly ? The code where your assertion is triggered, gstbasetransform.c:1973
Also, I forgot to mention that your patch had the problem that it takes the sink_suggest caps as is. Which again causes the problem that buffers with incomplete caps could be returned upstream (e.g. video/x-raw-rgb without any fields, which was the original bug here). I think this code here should be reorganized a bit. It should first try the sink_suggest caps, if these are not possible it should try the caps passed to the bufferalloc function (currently it only tries one of them) and only if this fails it should try to guess any possible caps. Too bad that this is dead code that is already gone in 0.11 ;)
This should all be fixed now and makes sense again... if there's still a problem please provide a real testcase :) ommit a6bb6d0fc31da908b26ec8157f922e091b54c9b3 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu Dec 8 18:00:00 2011 +0100 basetransform: Fix code path to come up with possible caps if incompatible caps are provided to buffer_alloc() Previous code could almost never work and this should be slightly better. commit 57573d8705d717e6dc8c3bf25eae95f59b62dd51 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu Dec 8 17:21:30 2011 +0100 basetransform: Fall back to upstream provided caps if suggested caps are not supported by the sinkpad commit aad7225eb5201ac5df1ed1044a44df9e5284e224 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu Dec 8 17:07:05 2011 +0100 basetransform: Fall back to upstream provided caps if fixation of suggested caps failed commit 26a1ac0ce700e7b62bbb12673afedee9c4f6b42b Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu Dec 8 17:02:28 2011 +0100 basetransform: Refactor gst_base_transform_buffer_alloc() code Don't check if upstream provided caps are compatible with upstream and don't try to fixate these caps. They must be fixated in any case.