GNOME Bugzilla – Bug 626621
[playbin2] streamsynchronizer regressions
Last modified: 2010-08-12 09:25:12 UTC
For example, EOS handling in streamsynchronizer leads to a few problems. If a stream reaches EOS without having seen data, EOS event is not immediately sent downstream, which may therefore fail to preroll, reach PAUSED etc If all streams reach EOS "at once", and then EOS is pushed downstream in a loop, this may deadlock if the EOS makes the sink preroll and wait in PAUSED. Some patches follow.
Created attachment 167585 [details] [review] streamsynchronizer: clear stream eos state on FLUSH and new stream
Created attachment 167586 [details] [review] streamsynchronizer: drop lock when pushing eos downstream
Created attachment 167587 [details] [review] playsink: always have a queue in chain head to aid streamsynchronizer
Created attachment 167588 [details] [review] playbin2/playsink: update subtitle handling for streamsynchronizer This patch should handle the hanging that occurs as mentioned in bug #626463.
Comment on attachment 167586 [details] [review] streamsynchronizer: drop lock when pushing eos downstream The GArray here is not optimal because of the allocations, reallocations and everything. Better use a GSList here, it will use GSlice and for the local-only, single-thread allocations here it will be much faster and cause less memory fragmentation. Other than that this is what I had in mind too yesterday when you told me about this problem :)
Review of attachment 167587 [details] [review]: Looks good in general and shouldn't hurt to add these queues ::: gst/playback/gstplaysink.c @@ +1219,3 @@ + /* NOTE streamsynchronizer needs streams decoupled */ + if (TRUE) { Just remove this large if block here @@ +1438,3 @@ + GST_ELEMENT_WARNING (playsink, CORE, MISSING_PLUGIN, + (_("Missing element '%s' - check your GStreamer installation."), + "queue"), ("video rendering might be suboptimal")); vqueue => subqueue or something like that and The warning talks about video, not text @@ +1642,3 @@ + /* NOTE streamsynchronizer always need a queue to decouple its EOS sending + * (which might otherwise hang in downstream prerolling on EOS) */ + if (TRUE) { Remove the complete if block @@ +2149,3 @@ + /* NOTE although there is some nifty heuristic regarding whether or not to + * include a queue in some chain, streamsynchronizer mechanics need + * subsequent streams decoupled to prevent hangs */ Better remove these heuristics then where necessary, should make some code a bit easier to understan
Review of attachment 167588 [details] [review]: Looks good too but should maybe be two different commits ::: gst/playback/gstplaysink.c @@ +1522,3 @@ + GST_ELEMENT_WARNING (playsink, CORE, MISSING_PLUGIN, + (_("Missing element '%s' - check your GStreamer installation."), + "queue"), ("video rendering might be suboptimal")); Talks again about video but should be text or subtitle ;)
(In reply to comment #5) > (From update of attachment 167586 [details] [review]) > The GArray here is not optimal because of the allocations, reallocations and > everything. Better use a GSList here, it will use GSlice and for the > local-only, single-thread allocations here it will be much faster and cause > less memory fragmentation. > Optimality is a bit relative here, since all streams having EOS does not happen X times/second, and there should only be one (pre-)allocation for the array, but I don't mind any particular way.
Created attachment 167697 [details] [review] streamsynchronizer: drop lock when pushing eos downstream As before, now using GSList.
Created attachment 167698 [details] [review] playsink: always have a queue in chain head to aid streamsynchronizer As before, a few text message altered. Note that if (TRUE) etc are removed in a separate commit later, so as not to cloud/obscure changes here.
Created attachment 167699 [details] [review] playbin2/playsink: update subtitle handling for streamsynchronizer Again as before, text messages / names altered. On the one hand, they might be separate commits (and elements), but on the other hand, the changes go together, which then fits an "atomic commit".
Created attachment 167700 [details] [review] playsink: remove some heuristic in chain configuration code As referred to earlier, this commit reflows the code to remove if (TRUE) and some decisions regarding queue insertion, as the latter is always the case now.
Created attachment 167701 [details] [review] streamsynchronizer: send preroll buffer when delaying preroll eos As mentioned in the original report, if an incoming eos is not forwarded which may actually cater for prerolling downstream (if no data has passed yet), then a replacement dummy buffer is sent to cater for preroll.
Review of attachment 167697 [details] [review]: ::: gst/playback/gststreamsynchronizer.c @@ +489,1 @@ + ret = TRUE; Hm, ret would only be set if there are pads. But it's impossible to get here without pads so that's fine ;)
commit c32b5a47f200205b390e7516658f973072e04c6e Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue Aug 10 11:19:59 2010 +0200 streamsynchronizer: clear stream eos state on FLUSH and new stream Fixes #626621. commit 97ae2749f19481776262c00a7ac74bb3b74d82cc Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue Aug 10 11:28:43 2010 +0200 streamsynchronizer: drop lock when pushing eos downstream ... to prevent deadlock (e.g. upon seek) when downstream waits in preroll. Fixes #626621. commit f18635d685a8e8c36ee9d778fde3b8918e2b92d9 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue Aug 10 13:06:32 2010 +0200 playsink: always have a queue in chain head to aid streamsynchronizer Specifically, as the latter may have one thread pushing EOS to several streams, that needs to be decoupled into various thread to prevent preroll hanging problems. Fixes #626621. commit 9fdda29965dc368979251626730b8c9d1bb0da65 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Wed Aug 11 10:27:35 2010 +0200 playbin2/playsink: update subtitle handling for streamsynchronizer Streamsynchronizer excepts to see stream-changed msg for all streams, but to arrange for this, video and subtitle streams need to be decoupled by means of queues (due to pad blocks that may occur). Fixes #626463. commit 2eae6edac2187632b98ba0097a78f45eac589c06 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Thu Aug 12 10:01:03 2010 +0200 playsink: remove some heuristic in chain configuration code .. since queues are now inserted unconditionally. Fixes #626621. commit 0c084f72963ed92c411b8d7ef5a863f9eb5664fe Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Thu Aug 12 10:02:56 2010 +0200 streamsynchronizer: send preroll buffer when delaying preroll eos That is, if eos is received which will not be forwarded, and the stream has not yet seen any data, then send a buffer to preroll downstream (which might otherwise be accomplished by the eos event). Fixes #626621.