Bug 197355

Summary: [GStreamer][MSE] Pausing video sometimes causes skip to finish
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: MediaAssignee: Charlie Turner <cturner>
Severity: Normal CC: aboya, bugs-noreply, commit-queue, cturner, ews-watchlist, mcatanzaro, pnormand, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Description Flags
Patch none

Description Michael Catanzaro 2019-04-28 13:07:12 PDT
Since we enabled MSE, pausing a YouTube video occasionally (but not usually and not often) causes the video to improperly skip to the very end.
Comment 1 Philippe Normand 2019-05-28 11:10:12 PDT
*** Bug 198054 has been marked as a duplicate of this bug. ***
Comment 2 Charlie Turner 2019-06-20 09:50:43 PDT
Created attachment 372562 [details]

WIP/demonstation patch
Comment 3 Charlie Turner 2019-06-20 09:51:58 PDT
This is easily reproducible on trunk. The way to trigger it is to seek very close to the end, let WebCore::MediaSource::endOfStream run, which will in turn call WebCore::MediaSource::streamEndedWithError, which in the GStreamerMSE player will set m_eosMarked = true, which in turn will set m_eosPending in updateStates(), which then breaks MediaPlayerPrivateGStreamerMSE::currentMediaTime, since after these steps it thinks the network resource is fully loaded and that the end has been reached (actually it just buffered to the end), and this in turn sets the cached playback position to the end and we get a jump.

Looking at this code I see all sorts of strange issues. We have a downstream port that doesn't have such blatant problems on YouTube, and I see a large diff in these areas which we should work on getting upstream.

The other complication here is that there's large MSE refactoring Alicia is doing in parallel that probably touches this, so the MSE folks will have a much better idea what is the best way to fix it.
Comment 4 EWS Watchlist 2019-06-20 09:52:52 PDT
Attachment 372562 [details] did not pass style-queue:

ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Charlie Turner 2019-06-20 09:54:06 PDT
To clarify, the WIP/demonstration patch is not for review, I put here to pinpoint where the bug is, this patch fixes the jumping issue for me, but I have no idea what else it may break, and the right thing to do is surely to focus on getting our downstream code up here and/or waiting for the refactoring.
Comment 6 Michael Catanzaro 2019-06-20 11:13:56 PDT
Thanks Charlie!

Alicia, what do you think?
Comment 7 Alicia Boya GarcĂ­a 2019-06-20 14:00:53 PDT
I have no idea why `paused()` was there in that condition, so on first sight the patch LGTM.

In the current state of my rework markEndOfStream() is empty, as it has no business with neither the AppendPipeline nor (directly) the playback pipeline.

Instead I rely on allSamplesInTrackEnqueued(), which SourceBuffer calls when the source is "ended" (.endOfStream() has been called) AND the decode queue is empty. When this method is fired an EOS event is enqueued into WebKitMediaSrc. 

MediaPlayerPrivateGStreamerMSE::currentMediaTime() does not even exist anymore in my rework, so your patch won't cause a lot of trouble other than a rebase conflict.

If it does not cause regressions on LayoutTests or YT tests I'm OK with it landing.
Comment 8 Charlie Turner 2019-06-21 06:44:35 PDT
Created attachment 372627 [details]

Try this patch in the bots commit queues. Locally I see a huge number of failures, but it may be my local environment
Comment 9 Michael Catanzaro 2019-06-21 07:21:44 PDT
None of the test EWS are going to run this code. If you want to see what happens to the bots, you just have to land it.

Safe to say that if it introduces a huge number of failures in your local environment that were not there before, there is a problem.
Comment 10 Charlie Turner 2019-06-21 07:58:33 PDT
(In reply to Michael Catanzaro from comment #9)
> None of the test EWS are going to run this code. If you want to see what
> happens to the bots, you just have to land it.
> Safe to say that if it introduces a huge number of failures in your local
> environment that were not there before, there is a problem.

Obviously I would be aware of that, the issue is that I have so many failures for other reasons with my environment that I can't see the signal for the noise at the moment.
Comment 11 Michael Catanzaro 2019-06-21 08:24:25 PDT
OK, just making sure you understand: neither the EWS nor the commit queue test GTK/WPE.
Comment 12 Charlie Turner 2019-06-21 13:15:24 PDT
I see. This patch doesn't regress any tests in media/ or imported/w3c/web-platform-tests/media-source for me. Let's land it on Monday when I can monitor the bots closely.
Comment 13 WebKit Commit Bot 2019-06-24 04:33:37 PDT
Comment on attachment 372627 [details]

Clearing flags on attachment: 372627

Committed r246731: <https://trac.webkit.org/changeset/246731>
Comment 14 WebKit Commit Bot 2019-06-24 04:33:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-06-24 04:34:32 PDT