GNOME Bugzilla – Bug 661262
[pulseaudiosink] Playbin2 fails playing mkv with ac3 as pulseaudiosink to accept ac3
Last modified: 2011-11-03 14:25:52 UTC
Created attachment 198596 [details] matroska with ac3 Attached file doesn't play with playbin2 as it will stop decoding because pulseaudiosink reports to support ac3, but then fails to accept it.
Created attachment 198597 [details] *:5 log of the failure
Same for mkv with dts audio stream. Marking as blocker, this needs to be fixed before the release, or pulseaudiosink needs to be set to rank NONE.
Created attachment 199203 [details] [review] playsink: handle NULL ghost pad target For the src pad anyway.
Created attachment 199204 [details] [review] playsink: fix passthrough mode (hopefully) The code was doing counterintuitive rewiring of pads when the bin did not contain any elements. We now add an identity element in that case, which makes it simpler, and should fix the AC3 passthrough mode when using pulseaudio (but I don't see the bug here so can't test).
I'm not seeing the issue, so could you check if that fixes it please ?
After a chat with slomo, this still needs patching for the case where the bin is set up in the wrong way for incoming caps in getcaps. Looking at it...
Created attachment 199232 [details] [review] playsink: refactor the converter bins since they are almost identical
Would fit better on a separate bug, but it depends on my previous patches here :)
Created attachment 199242 [details] [review] playsink: only compare against the media type we expect ie, audio/x-raw- for audio, video/x-raw- for video. Add a trailing - to be more specific. I doubt there's anything like audio/x-rawhide or something, but you never know.
Created attachment 199243 [details] [review] playsink: keep both raw and non raw pipelines at all times and switch between them as needed.
Created attachment 199244 [details] [review] playsink: cache inner converter bin caps
Created attachment 199245 [details] [review] playsink: consider both passthrough and converter caps in getcaps Since we can switch between both modes.
Created attachment 199246 [details] [review] playsink: handle NULL cached caps in getcaps
Created attachment 199275 [details] new log Still fails, attaching new log.
Created attachment 199276 [details] [review] playsink: handle after-the-fact changes in converters/volume booleans The playsink was nastily poking a boolean in the structure. Make those booleans properties, so we are told when they change, and rebuild the conversion bin when they do. Some cleanup to go with it too.
I just found out that: "gst-launch audiotestsrc num-buffers=100 ! ffenc_ac3 ! pulseaudiosink" works here. So my pulse version can handle ac3. $ pulseaudio --version pulseaudio 1.0
Created attachment 199284 [details] [review] playsink: don't forget to reset pad targets after reconfiguration
Comment on attachment 199243 [details] [review] playsink: keep both raw and non raw pipelines at all times When always keeping the pipelines you should make sure to send flush-start and flush-stop to the new pipeline if you change them before sending anything else. Otherwise you'll keep running time information from previous time it was used around and problems arise. Everything else looks good
Since I can't seem to nail it down and I'm not seeing the issue in the first place, I'll let Thiago investigate for now since he started looking.
As we have too many patches already, I've put everything on git.collabora.co.uk/git/user/thiagoss/gst-plugins-base.git (playsink branch). This line on pulseaudiosink's proxypad_blocked_cb seems to be the remaining problem: caps = gst_pad_get_caps_reffed (pad); this caps is not the one that is fixed and set on the pad, so using them to test if pulsesink supports it is meaningless.
The following fix has been pushed: a7790ef pulse: Get caps correctly on pad block This fixes the original problem of pulseaudiosink picking up wrong caps when deciding whether to plug in a decodebin2 or not. Keeping the bug open for other playsink fixes.
Created attachment 199332 [details] [review] pulse: Get caps correctly on pad block Instead of always going upstream, we should first see if already got caps from a setcaps() call.
Created attachment 199432 [details] [review] playsink: lock the new {set,get}_property functions
Created attachment 199433 [details] [review] playsink: re-add identity where appropriate
Created attachment 199434 [details] [review] playsink: send flush start/stop event when we switch elements
Review of attachment 199203 [details] [review]: This is the wrong patch, the gst_proxy_pad_getcaps_default() should do the right thing, you don't have to try and implement your own. Also, the gst_ghost_pad_get_target() should never return NULL unless linked. I'll post patches today for those issues.
(In reply to comment #26) > Review of attachment 199203 [details] [review]: > > This is the wrong patch, the gst_proxy_pad_getcaps_default() should do the > right thing, you don't have to try and implement your own. Also, the > gst_ghost_pad_get_target() should never return NULL unless linked. I'll post > patches today for those issues. I mean never return NULL if linked. Is it possible to wait a bit before adding all those patches, because I don't think they are all required really. Many workaround the target() issue I'm fixing in bug #658517 . I've been testing my patch yesterday, I need to finish the unit test, but should post soon.
Created attachment 199978 [details] [review] Use default proxy pad getcaps implementation The default proxy pad getcaps should just work, any other implementation should be based on it. I don't pretend using identity is bad, but having empty bins in the playbin is already soso, thus adding 1 more object to proxy everything makes it worst. I didn't sorted out which of the other patches should be rebased, this one at least supersedes the one I marked rejected. cheers, Nicolas Note this patch depends on bug https://bugzilla.gnome.org/show_bug.cgi?id=658517 to work without the proposed hacks here.
+1 on the comment in bug #660816 about the current getcaps() change reversing the caps request direction. It was causing a deadlock in some silly code in deinterlace because a downstream getcaps() ends up reflected back inside playsinkvideoconvert and calling back into deinterlace.
Review of attachment 199204 [details] [review]: I would rather drop this one as it work around the target issue, which is being fixed in Bug 658517
I disagree with this. The juggling of ghost pads was counterintuitive and error prone, as shown by my first patch where I hadn't realized it. Adding an identity for passthrough removes the need for that juggling.
Let's explain what is the current solution and yours. So assuming the scenario where we don't have any elements in the bin (nothing to convert), the current solution is simply to link the two internal proxy pads together, while yours it to put the identity element in between. Current: SinkGhostPad SrcGhostPAd internal (a src) <--------> internal (a sink) Code: gst_ghost_pad_set_target (gsrc, gst_proxy_pad_get_internal (gsink)); Identity: SinkGhostPad Identity SrcGhostPad internal (a src) <-------> sink src <--------> internal (a sink) Code: gst_ghost_pad_set_target (gsink, isink); gst_ghost_pad_set_target (gsrc, isrc); One might be slightly easier to understand as it's the normal element linking scenario, but (for me) it's not counter intuitive enough to invalidate the solution. Also what I want to be considered is the overhead. I think the current one is slightly lower overhead as calls (event, message, queries, buffers, etc) will have to go through 4 pads in current approach and 6 in the identity approach plus the fact the identity is a GstBaseTranform, which also add to the overhead.
I acknowledge your point about the counter intuitivity of the method, it is indeed subjective, and my lack of knowledge about ghost and proxy pads may be the most significant reason for me thinking it is counter intuitive. As for the overhead, I can't see anything in what you describe being even barely significant. The extra caps nego just might (though after 662777 and the basetranform caching, likely much less of an issue either). Don't forget this is a bin inside playsink, there's lots of other stuff buffers/queries will go through here. That said, if you feel strongly about it being better with the current system, then fine. Just keep the refactoring of both bins.
Review of attachment 199232 [details] [review]: This one looks like a good idea, I'll try to rebase it.
Created attachment 200233 [details] [review] Rewored playsink converter refactoring I've rebase/reworked the refactoring on top of the proxy pad getcaps stuff. I skipped the caps cache and the raw/no-raw optimisation part as I don't personally need that for my work. Also I noticed that Arun patch is enough for this bug, so we are already hijacking it with some re-factoring work. I suggest we should create seperate bugs for it. Changed from your initial work is: - Using GQueue to do optimal appending operation (future proof as the chain is very small) - Renamed to GstPlaySinkBaseConvert, to reflect it's a base class and nicely do thelink with the subclass names GstPlaySinkVideoConvert and GstPlaySinkAudioConvert.
Created attachment 200234 [details] [review] Check only audio/video raw string base on context
Review of attachment 199232 [details] [review]: Superseeded
Review of attachment 199242 [details] [review]: Superseeded
Review of attachment 199244 [details] [review]: I don't understand why a cache is needed. The bins contains few elements, and is placed in front of only 1 other element, a sink ...
Comment on attachment 199978 [details] [review] Use default proxy pad getcaps implementation This is not the way to go. The getcaps function in the converter bins must do something more. They must combine the downstream/upstream caps and the converter element caps.
commit b29a3d3cfffa418f8cdfd4c83a1750dc96039695 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu Nov 3 10:07:27 2011 +0100 playsinkconvertbin: Don't add identity multiple times commit 7eb8a9aaf6c0bf92a35e2c3f35ba7398eb8f6988 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Oct 19 14:13:39 2011 +0100 playsink: send flush start/stop event when we switch elements https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit 0cac680faccd6d2c1396339f1994a63d971697d2 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Oct 19 14:13:30 2011 +0100 playsink: re-add identity where appropriate https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit c3e94d1c0890f5dcc139d29692bb588ae6f5d48e Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Oct 19 14:12:01 2011 +0100 playsink: lock the new {set,get}_property functions https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit 0a07701164a35f736ba8ba881bbcc12e9f9140ce Author: Thiago Santos <thiago.sousa.santos@collabora.com> Date: Mon Oct 17 23:14:54 2011 +0000 playsinkconvertbin: Be more consistent with ghostpad targets Set up targets on READY->PAUSED state change to passthrough by default. This prevents the targets from being unset on the first run, while the 'raw' variable would mean that some target is set. commit f9ea3fdda8c391afc17cb7b110a998d333d12059 Author: Thiago Santos <thiago.sousa.santos@collabora.com> Date: Mon Oct 17 22:41:49 2011 +0000 playsinkconvertbin: No need to remove the identity The identity element should be handled by the GstBin's cleanup, removing it on the remove_elements function might remove it too soon, as this function can be called directly from playsink commit 34f72da9cc41e4b1e94dc175efdc386751b266ce Author: Thiago Santos <thiago.sousa.santos@collabora.com> Date: Mon Oct 17 22:41:11 2011 +0000 playsinkconvertbin: Adding some debug messages Adds a couple debug messages and some g_assert to make debugging easier commit 80971a3b337f8083afd06aa63cb01eb5f3f67585 Author: Thiago Santos <thiago.sousa.santos@collabora.com> Date: Mon Oct 17 22:02:03 2011 +0000 playsink-videoconvert: Fix warning on build Remove unused variable commit ae3ba533914d7961248190bc56734d7b6fe2b716 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 21:05:30 2011 +0000 playsink: handle after-the-fact changes in converters/volume booleans The playsink was nastily poking a boolean in the structure. Make those booleans properties, so we are told when they change, and rebuild the conversion bin when they do. Some cleanup to go with it too. https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit c08a23169d85b47c28ce8ded06a80b90c02ef8cc Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 18:43:06 2011 +0000 playsink: handle NULL cached caps in getcaps https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit 3939457b00f3ae81833c41b8fbab2bed5099aee9 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 18:06:00 2011 +0000 playsink: consider both passthrough and converter caps in getcaps Since we can switch between both modes. https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit b34dac9a8737eb5b9e16e86ba2553c9758990e3a Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 17:54:27 2011 +0000 playsink: cache inner converter bin caps https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit 6925c02bc23091a250bfbf2aba68e469b9cf2e4e Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 17:26:48 2011 +0000 playsink: keep both raw and non raw pipelines at all times and switch between them as needed. https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit 69d98d08c193c5ee21775b0cc8ef6b0a6c6d0089 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 17:29:50 2011 +0000 playsink: only compare against the media type we expect ie, audio/x-raw- for audio, video/x-raw- for video. Add a trailing - to be more specific. I doubt there's anything like audio/x-rawhide or something, but you never know. https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit fd27e34582fb8785e741785e265380497bf15e14 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 16:55:30 2011 +0000 playsink: refactor the converter bins since they are almost identical https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit c8e0d215cbaf9ffc0713fe8c060b52af7ffff958 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Oct 17 13:00:05 2011 +0000 playsink: fix passthrough mode (hopefully) The code was doing counterintuitive rewiring of pads when the bin did not contain any elements. We now add an identity element in that case, which makes it simpler, and should fix the AC3 passthrough mode when using pulseaudio (but I don't see the bug here so can't test). https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit 2b84b328b1cac2c083fb322f4abb91bf439fb944 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Oct 7 11:16:44 2011 +0000 playsink: handle NULL ghost pad target For the src pad anyway. https://bugzilla.gnome.org/show_bug.cgi?id=661262 commit a583b63722d2397914ad2e774df0ea6ba2b9d8ba Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu Nov 3 09:56:14 2011 +0100 Revert "playsinkaudioconvert: Fix warning when there is no target pad yet" This reverts commit f35c51c14915729f0fdf2b348f351ea7e81027cc. Better patch coming soon.
(In reply to comment #40) > (From update of attachment 199978 [details] [review]) > This is not the way to go. The getcaps function in the converter bins must do > something more. They must combine the downstream/upstream caps and the > converter element caps. I May be missing something, but from what I've read so far if you have a chain of element and let getcaps() follow the chain, you do get the combination at the end. What the default proxy pad getcaps() does is correctly follow the chain (as if there was no ghost pad). This was actually broken on the ghostpad side because the get_target() method was broken.