Bug 197355 – [GStreamer][MSE] Pausing video sometimes causes skip to finish

• Component: Media
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Philippe Normand]

*** Bug 198054 has been marked as a duplicate of this bug. ***

[Charlie Turner]

Created attachment 372562 [details]
Patch

WIP/demonstation patch

[Charlie Turner]

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.

[EWS Watchlist]

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.

[Charlie Turner]

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.

[Michael Catanzaro]

Thanks Charlie!

Alicia, what do you think?

[Alicia Boya García]

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.

[Charlie Turner]

Created attachment 372627 [details]
Patch

Try this patch in the bots commit queues. Locally I see a huge number of failures, but it may be my local environment

[Michael Catanzaro]

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.

[Charlie Turner]

(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.

[Michael Catanzaro]

OK, just making sure you understand: neither the EWS nor the commit queue test GTK/WPE.

[Charlie Turner]

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.

[WebKit Commit Bot]

Comment on attachment 372627 [details]
Patch

Clearing flags on attachment: 372627

Committed r246731: <https://trac.webkit.org/changeset/246731>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52049850>