Bug 195454 – [Media][MSE] Don't emit timeUpdate after play() if currentTime hasn't changed

• Component: Media
• Status: RESOLVED FIXED

[Enrique Ocaña]

MSE 2019[1] tests "26. SFRPausedAccuracy" and "27. HFRPausedAccuracy" fail on WPE on Raspberry Pi. Those tests measure the time drift (real time vs. media time) on playback and start counting since the first timeUpdate event. In WebKit, that event happens at play(), before the pipeline has completed the transition to playing. Therefore, the real time inherits this startup delay and the test thinks that the player has drifted.

I tried to convince the YouTube MSE Conformance Tests authors to ignore that initial timeUpdate event which reports no time change and actually start measuring the time drift after the first timeUpdate greater than zero[2]. However, their response quoting the spec[3] and stating that a timeupdate event is fired if "The current playback position changed as part of normal playback or in an especially interesting way, for example discontinuously." makes sense. I'm going to submit a patch to change this behaviour in WebKit, the 3 tests that depend on the old behaviour and to improve currentTime accuracy in the GStreamer media player private implementation by forcing a currentTime update on play() and pause().

[1] https://ytlr-cert.appspot.com/2019/main.html
[2] https://github.com/youtube/js_mse_eme/pull/37
[3] https://www.w3.org/TR/html52/semantics-embedded-content.html#eventdef-media-timeupdate

[Enrique Ocaña]

Created attachment 364006 [details]
Patch

[EWS Watchlist]

Comment on attachment 364006 [details]
Patch

Attachment 364006 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11426043

New failing tests:
compositing/video/video-clip-change-src.html

[EWS Watchlist]

Created attachment 364009 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Jer Noble]

Comment on attachment 364006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364006&action=review

This looks fine to me with a minor nit which could be addressed before landing.

> Source/WebCore/html/HTMLMediaElement.cpp:3594
> +        m_lastTimeUpdateEventMovieTime = currentMediaTime();

There is a slight inefficiency here, in that calling currentMediaTime() will go down all the way to the MediaPlayerPrivate again (and could have a different answer the second time!). So I would slightly refactor this so that m_lstTimeUpdateEventMovieTime was set before m_playbackStartedTime, and the playback value was just generated from the update value.

[Eric Carlson]

Comment on attachment 364006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364006&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:3594
>> +        m_lastTimeUpdateEventMovieTime = currentMediaTime();
> 
> There is a slight inefficiency here, in that calling currentMediaTime() will go down all the way to the MediaPlayerPrivate again (and could have a different answer the second time!). So I would slightly refactor this so that m_lstTimeUpdateEventMovieTime was set before m_playbackStartedTime, and the playback value was just generated from the update value.

Or "m_lastTimeUpdateEventMovieTime = m_playbackStartedTime.toDouble();"

[Jer Noble]

(In reply to Eric Carlson from comment #5)
> Comment on attachment 364006 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364006&action=review
> 
> >> Source/WebCore/html/HTMLMediaElement.cpp:3594
> >> +        m_lastTimeUpdateEventMovieTime = currentMediaTime();
> > 
> > There is a slight inefficiency here, in that calling currentMediaTime() will go down all the way to the MediaPlayerPrivate again (and could have a different answer the second time!). So I would slightly refactor this so that m_lstTimeUpdateEventMovieTime was set before m_playbackStartedTime, and the playback value was just generated from the update value.
> 
> Or "m_lastTimeUpdateEventMovieTime = m_playbackStartedTime.toDouble();"

I think m_lastTimeUpdateEventMovieTime is a MediaTime, and m_playbackStartedTime is a double, so it has to be the other way around.

[Enrique Ocaña]

Created attachment 364017 [details]
Patch

[Philippe Normand]

Comment on attachment 364017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364017&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:476
> +    // This will force a position update next time playbackPosition() is called.
> +    m_lastQueryTime = WTF::Seconds(0);
> +    playbackPosition();

Is it really necessary to call playbackPosition()?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:494
> +    // This will force a position update next time playbackPosition() is called.
> +    m_lastQueryTime = WTF::Seconds(0);
> +    playbackPosition();

Ditto :) Also if we end-up with copy-paste code it might be worth factoring it out to a private method, something like clearCachedPosition() maybe? It could include a GST_DEBUG_OBJECT() call as well.

[Enrique Ocaña]

Comment on attachment 364017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364017&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:476
>> +    playbackPosition();
> 
> Is it really necessary to call playbackPosition()?

Yes, not only because that method updates the position, but because it has the logic that manages the cached time and takes decisions in some corner cases such as seeks. The m_lastQueryTime thing is just to force a cache update when playbackPosition() is called ("the cache was last updated at the begining of epoch", so it's too old now and must be updated), it has nothing to do with setting currentTime=0 or anything like that.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:494
>> +    playbackPosition();
> 
> Ditto :) Also if we end-up with copy-paste code it might be worth factoring it out to a private method, something like clearCachedPosition() maybe? It could include a GST_DEBUG_OBJECT() call as well.

Thanks for the suggestion.

[Enrique Ocaña]

After refactoring the cached position refresh, trying the patch again on Raspberry Pi and analyzing the accuracy increase refreshing the cache vs. doing nothing (only the timeUpdate change in HTMLMediaElement) I've realized that the cached position refresh not only isn't needed but actually makes things worse.

That cached position refresh was my first attempt to fix the YouTube test, but the actual fix was the timeUpdate one. Therefore, I think it's better to keep things simple and submit only the timeUpdate part of the patch. I've also changed the bug name to reflect that.

[Enrique Ocaña]

Created attachment 364250 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364250 [details]
Patch

Clearing flags on attachment: 364250

Committed r242791: <https://trac.webkit.org/changeset/242791>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48803810>