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
Created attachment 364006 [details] Patch
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
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
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.
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();"
(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.
Created attachment 364017 [details] Patch
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.
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.
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.
Created attachment 364250 [details] Patch
Comment on attachment 364250 [details] Patch Clearing flags on attachment: 364250 Committed r242791: <https://trac.webkit.org/changeset/242791>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48803810>