GNOME Bugzilla – Bug 633700
streamsynchronizer: regression: hang when pausing near EOS
Last modified: 2018-11-03 11:17:49 UTC
That is, play (audio and video) until about eos, pause and then change to playing again, then pipeline might fail to preroll (and as such never reach playing again). Specifically, if one the streams has already reached eos, whereas the other has not, then streamsynchronizer will not forward eos. So, one of the sinks will preroll (with regular data), but the other never will (as it needs the eos that streamsynchronizer will not send since the other stream is blocked downstream in pause, assuming here any limited downstream queues do not suffice to compensate). Not quite sure what to do about this; there are already some ugly streamsynchronizer hacks that send an empty buffer to trigger/force preroll (which probably won't be digested well by e.g. xvimagesink). So, it seems that if we want to do the tricky/advanced stuff as in streamsynchronizer without breaking the basic stuff (and keep it more or less clean), we need some "force-preroll-event" (or something like that). The latter might also help in some other cases (e.g. handling empty edit in qtdemux). Or maybe that is too much 0.11 futuristic and there are other tricks or different ways to go about what streamsynchronizer/playsink/etc try to achieve ?
Upon further reflection; a "force-preroll-event" would not suffice by itself. That is, the prerolled state would have to persist across basesink state change (as when a basesink receives eos). If not, it would be up to streamsynchronizer to resend these things upon state change (which could be hard also). IIRC, streamsynchronizer delays these eos events so it can actually eat them (if a new gapless stream needs to follow). [If not eaten, the sinks would drop/ignore data following eos.] So from streamsynchronizer perspective, it needs an eos event (with some flag saying it should happily accept new data/events and then carry on normally). That would also look like an event that forces preroll in some way (and also have similar semantics to an update newsegment event saying "something will be around some time later").
Marking as blocker for now. We should really try to figure out if we can do something about this that's nto too invasive.
Created attachment 173817 [details] Test app to reproduce That is, attached app will perform a seek to some initial position in a clip provided by URL and then lots of state changes PAUSED/PLAYING. So by seeking near the end there is a good chance to run into it, though it depends a bit on the clip (and still not deterministic of course).
Created attachment 173941 [details] Test clip to reproduce Hand-crafted avi clip that has a (minor) a/v imbalance (video is 2s longer than audio), which should suffice to reproduce this (e.g. using attached app).
Created attachment 173942 [details] [review] basesink: handle soft eos event Patch introduces a "soft eos" event and proper handling for it in basesink. That is, it has (almost) all effects of an eos event (except eos message posting) and it is "forgotten" about again when new data or newsegment event arrives. [Of course, patch may need splitting, naming is open for discussion, and other tweaks here and there might be in order etc]
Created attachment 173943 [details] [review] baseaudiosink: handle soft-eos event
Created attachment 173944 [details] [review] streamsynchronizer: send soft-eos event when appropriate
Above patches should prevent failing preroll, and the last one also illustrates how it could be used alongside newsegment updates in other (e.g. demuxer) _sync_stream cases to prevent preroll problems (of which there are quite some instances, for instance notably qtdemux empty edit list handling flying around in some patches). Also, it should not be too invasive, and in particular not have any real effect other than in gapless playback, which is currently experimental anyway (and perhaps can hardly get worse problems than it exhibits now, such as present item).
Not really sure what to do about this. We need to make a decision though, so we can get on with the release. It's clearly bad that it's possible to seek somewhere with playbin2 without being able to get back into playing state afterwards. However, I'm wondering how much of a problem this actually poses in practice. How many files have video and audio streams with considerably different lengths? It does pose a problem for people who want to extract the very last frame of a file I guess, but how common/useful is that? (I know I've seen this come up on the list a couple of times) (And do those people use playbin2?) Thanks for coming up with those patches, Mark! It seems though that people are not too keen on this approach, which adds new API/concepts, but would prefer a different solution. That poses the question what to do for the release. There don't seem to be alternative patches / approaches that are less intrusive and/or risky. We could roll playbin2 back to the 0.10.30 version, but the fixes and additions since then seem worthwhile to have and probably outweigh this problem. So maybe we should just release with this bug and get it fixed for the next release? Or is this issue more severe than I think it is?
(In reply to comment #9) > Not really sure what to do about this. We need to make a decision though, so we > can get on with the release. > > It's clearly bad that it's possible to seek somewhere with playbin2 without > being able to get back into playing state afterwards. And would also occur if one simply tries to pause/play at that time (or whenever software decides to do so behind the screen, e.g. to temporarily pause playback to show "media details" info or whatever). > > However, I'm wondering how much of a problem this actually poses in practice. > How many files have video and audio streams with considerably different > lengths? It seems grasping at straws ;) to classify 2s difference (as illustrated by attached example) in audio/video as considerably different (compared to quite possibly many tens of minutes of clip length) [and the 2s can in fact be a lot less for it to still happen]. Seems like a possible thin end of a wedge if we try to discard "by practical impact" ... (especially on regression of basic case/playback in favor of new/rare? feature). > > It does pose a problem for people who want to extract the very last frame of a > file I guess, but how common/useful is that? (I know I've seen this come up on > the list a couple of times) (And do those people use playbin2?) > > Thanks for coming up with those patches, Mark! It seems though that people are > not too keen on this approach, which adds new API/concepts, but would prefer a > different solution. > Hmmmm. Any particular arguments what might go/be wrong with this approach? (as even tons of new API/events/messages have not stopped things before). But anyway, as long as "different solutions" get the job done (and while at it also help to handle some "preroll hell" as indicated earlier). > That poses the question what to do for the release. > > There don't seem to be alternative patches / approaches that are less intrusive > and/or risky. > > We could roll playbin2 back to the 0.10.30 version, but the fixes and additions > since then seem worthwhile to have and probably outweigh this problem. > > So maybe we should just release with this bug and get it fixed for the next > release? Or is this issue more severe than I think it is? I would consider it more severe then it apparently seems rated, but not necessarily so that it needs to hold back release. Then again, lifting it over release might be viewed as a license to never bother again. So, perhaps we should only release if and only if bug is either resolved or we know how we plan to. Releasing with bug as-is, and discarding a possible solution in the process, with no more specific idea/notion than "other solutions" on how to handle seems not The Way To Go.
I do agree with the sentiment, just trying to get things moving and gather some opinions here :) What was the verdict on the make-sinks-preroll-on-newsegment-updates approach? Is that workable, just not a good idea to do 5 minutes before a release, or was there a flaw with that approach? Maybe we can use your approach, but don't make it public API, just use a custom event for now to make it work, and then see if there's a different better solution later?
(In reply to comment #11) > I do agree with the sentiment, just trying to get things moving and gather some > opinions here :) > > What was the verdict on the make-sinks-preroll-on-newsegment-updates approach? > Is that workable, just not a good idea to do 5 minutes before a release, or was > there a flaw with that approach? AFAIK, more is needed in this case; e.g. the prerolled state needs to preserve beyond PAUSE/PLAYING (i.e. similar to eos state). Otherwise upstream (e.g. streamsynchronizer) would be required to (re)act on state change to PLAYING (e.g. sending another newsegment event), which sounds uncomfortable/hackish and possibly tricky/dangerous if that has to be done in state change thread (which other constraints in circumstances likely dictate). > > Maybe we can use your approach, but don't make it public API, just use a custom > event for now to make it work, and then see if there's a different better > solution later? Would work for me for now (assuming then it has been looked over for not likely breaking whatever other stuff).
<__tim> wtay-: did you have any opinions on https://bugzilla.gnome.org/show_bug.cgi?id=633700 ? <__tim> was there a flaw with the newsegment-update approach? I don't remember.. <wtay-> no, that's the way to go forward <wtay-> but I wouldn't block the release on that <__tim> right, and that's not something we want to add/change 5 minutes before a release, right? <wtay-> nope <wtay-> let alone figure out how <wtay-> so you either live with the flaw or revert <wtay-> I wouldn't go for new strange events <__tim> how do you feel about mnauw's patches, but using a custom event as a quick-fix instead of new proper api? <__tim> right <wtay-> a temporary custom event I could live with <wtay-> but not new core api.. <__tim> ok <__tim> could you have a quick look at the patches just to gauge the risk? <wtay-> I can't say, it looks ok but I need more time to verify all of this <wtay-> a new field for the EOS event could work too.. <__tim> right <wtay-> but it's just going to be a temporary hack until we have newsegment preroll <wtay-> or maybe something else
Instead of reverting all changes to playbin2 since 0.10.30 we could also make the streamsynchronizer work in a passthrough mode for this release. Adding the few lines of code to do that shouldn't be that much work.
Created attachment 174640 [details] [review] playbin2: disable streamsynchronizer magic for this release > Instead of reverting all changes to playbin2 since 0.10.30 we could also make > the streamsynchronizer work in a passthrough mode for this release. Adding the > few lines of code to do that shouldn't be that much work. That sounds like a good plan too. Like this?
Committed this and downgraded severity: commit 8f039997f097ac5baa6a59350b2d41942a13232e Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Nov 17 01:01:03 2010 +0000 playbin2: disable streamsynchronizer magic for this release Some things aren't quite right yet and cause problems (0-sized buffers with PREROLL flag set cause crashes in elements that don't expect those; getting pipeline back to preroll/playing again when audio/video streams have different lengths and a seek past the end of one of the stream happens doesn't always work, etc.). Needs further investigation in the next cycle. https://bugzilla.gnome.org/show_bug.cgi?id=633700 https://bugzilla.gnome.org/show_bug.cgi?id=634699
*** Bug 634699 has been marked as a duplicate of this bug. ***
By re-enabling streamsynchronizer in 0.11 (rather than mere passthrough), we are again vulnerable to this problem. And afaics the additional wildlife of events in 0.11 is not helping to resolve the problem here.
Can someone reproduce this problem with latest 1.0?
Yes. Exactly as in the original description using the attached test clip, which exhibits a somewhat artificial imbalance, for ease of reproduction, but the same scenario is possible in natural cases.
Still happens, although stack traces don't look like it's streamsynchronizers fault now:
+ Trace 232256
Still happening with latest git master
Comment on attachment 173942 [details] [review] basesink: handle soft eos event I think this soft-eos event is not the solution for this though, it seems rather hackish.
We can maybe cause downstream to pre-roll with a GAP event
Same issue as #777073 (which as seen more recent activity) ?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/40.