[Media][MSE] Don't emit timeUpdate after play() if currentTime hasn't…

… changed

https://bugs.webkit.org/show_bug.cgi?id=195454

Reviewed by Jer Noble.

Source/WebCore:

This change fixes YouTube 2019 MSE Conformance Tests "26. SFRPausedAccuracy"
and "27. HFRPausedAccuracy".

The first timeUpdate event after play() is omitted, because currentTime
doesn't actually change in that scenario.

Tests 26 and 27 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.

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::playInternal): Don't emit a timeUpdated event unless currentTime has changed.

LayoutTests:

This patch removes expectations for the first timeUpdate event after
play(), because currentTime doesn't actually change in that scenario
and the spec[1] states 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."

[1] https://www.w3.org/TR/html52/semantics-embedded-content.html#eventdef-media-timeupdate

* media/video-paused-0-rate.html: Don't require the timeUpdate event when currentTime=0 to pass the test.
* media/video-play-pause-events-expected.txt: Ditto, and changed test description.
* media/video-play-pause-events.html: Changed test description to reflect the new behaviour.
* media/video-play-pause-exception-expected.txt: Don't require the timeUpdate event.


Canonical link: https://commits.webkit.org/209919@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242791 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,23 @@
+2019-03-12  Enrique Ocaña González  <eocanha@igalia.com>
+
+        [Media][MSE] Don't emit timeUpdate after play() if currentTime hasn't changed
+        https://bugs.webkit.org/show_bug.cgi?id=195454
+
+        Reviewed by Jer Noble.
+
+        This patch removes expectations for the first timeUpdate event after
+        play(), because currentTime doesn't actually change in that scenario
+        and the spec[1] states 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."
+
+        [1] https://www.w3.org/TR/html52/semantics-embedded-content.html#eventdef-media-timeupdate
+
+        * media/video-paused-0-rate.html: Don't require the timeUpdate event when currentTime=0 to pass the test.
+        * media/video-play-pause-events-expected.txt: Ditto, and changed test description.
+        * media/video-play-pause-events.html: Changed test description to reflect the new behaviour.
+        * media/video-play-pause-exception-expected.txt: Don't require the timeUpdate event.
+
 2019-03-11  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, rolling out r242763.

LayoutTests/media/video-paused-0-rate.html

@@ -5,6 +5,7 @@
         <script src=video-test.js></script>
         <script>
             var timeUpdateWasCalled = false;
+            var startTime = NaN;
             function start()
             {
                 findMediaElement();
@@ -17,17 +18,22 @@
             function canPlayThrough()
             {
                 video.playbackRate = 0;
+                startTime = video.currentTime;
+                setTimeout("checkplayback()", 250);
                 video.play();
             }
 
             function timeupdate()
+            {
+                if (video.currentTime != startTime)
+                    failTest("Time was updated, but should not change.");
+            }
+
+            function checkplayback()
             {
                 testExpected('video.currentTime', 0);
                 testExpected('video.paused', false);
-                if(timeUpdateWasCalled)
-                    failTest("Time was updated, but should not change.");
-                timeUpdateWasCalled = true;
-                setTimeout("endTest()", 150);
+                endTest();
             }
         </script>
     </head>

LayoutTests/media/video-play-pause-events-expected.txt

@@ -1,12 +1,11 @@
 
-Test that calling play() and pause() triggers async play, timeupdate and pause events.
+Test that calling play() and pause() triggers async play, no timeupdate event and pause event.
 
 RUN(handlePromise(video.play()))
 RUN(video.pause())
 SCRIPT DONE
 EVENT(play)
 EVENT(waiting)
-EVENT(timeupdate)
 EVENT(pause)
 EXPECTED (video.paused == 'true') OK
 END OF TEST

LayoutTests/media/video-play-pause-events.html

@@ -1,5 +1,5 @@
 <video controls></video>
-<p>Test that calling play() and pause() triggers async play, timeupdate and pause events.</p>
+<p>Test that calling play() and pause() triggers async play, no timeupdate event and pause event.</p>
 <script src=media-file.js></script>
 <script src=video-test.js></script>
 <script>

LayoutTests/media/video-play-pause-exception-expected.txt

@@ -4,7 +4,6 @@ Video has no src. Test that the playing event is not dispatched.
 RUN(handlePromise(video.play()))
 RUN(video.pause())
 EVENT(waiting)
-EVENT(timeupdate)
 EVENT(pause)
 EXPECTED (video.networkState == '0') OK
 END OF TEST

Source/WebCore/ChangeLog

@@ -1,3 +1,25 @@
+2019-03-12  Enrique Ocaña González  <eocanha@igalia.com>
+
+        [Media][MSE] Don't emit timeUpdate after play() if currentTime hasn't changed
+        https://bugs.webkit.org/show_bug.cgi?id=195454
+
+        Reviewed by Jer Noble.
+
+        This change fixes YouTube 2019 MSE Conformance Tests "26. SFRPausedAccuracy"
+        and "27. HFRPausedAccuracy".
+
+        The first timeUpdate event after play() is omitted, because currentTime
+        doesn't actually change in that scenario.
+
+        Tests 26 and 27 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.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::playInternal): Don't emit a timeUpdated event unless currentTime has changed.
+
 2019-03-12  Enrique Ocaña González  <eocanha@igalia.com>
 
         [EME][GStreamer] Speculative build fix

Source/WebCore/html/HTMLMediaElement.cpp

@@ -3587,7 +3587,12 @@ void HTMLMediaElement::playInternal()
     if (m_paused) {
         m_paused = false;
         invalidateCachedTime();
-        m_playbackStartedTime = currentMediaTime().toDouble();
+
+        // This avoids the first timeUpdated event after playback starts, when currentTime is still
+        // the same as it was when the video was paused (and the time hasn't changed yet).
+        m_lastTimeUpdateEventMovieTime = currentMediaTime();
+        m_playbackStartedTime = m_lastTimeUpdateEventMovieTime.toDouble();
+
         scheduleEvent(eventNames().playEvent);
 
 #if ENABLE(MEDIA_SESSION)