Title: [291683] branches/safari-614.1.7-branch
Revision
291683
Author
repst...@apple.com
Date
2022-03-22 11:37:26 -0700 (Tue, 22 Mar 2022)

Log Message

Cherry-pick r291629. rdar://problem/89495774

    YouTube.com - Clicking anywhere on the progress bar pauses the video
    https://bugs.webkit.org/show_bug.cgi?id=237750
    <rdar://problem/90364846>

    Reviewed by Eric Carlson.

    Source/WebCore:

    Test: media/media-source/media-source-unexpected-pause.html

    When calling play() or pause() on a MediaPlayerPrivateMediaSourceAVFObjC, the object will
    respond by calling m_player->playbackStateChanged(), which will in turn call
    HTMLMediaElement::mediaPlayerPlaybackStateChanged() with the new state. However, HTMLMediaElement
    expects this to be called only for unanticipated state changes, not expected state changes. And
    when that method is called and the reported state does not match the element's own expected state,
    the element calls its own play() or pause() function to update its own state to match the player's.
    And because MediaPlayerPrivateMediaSourceAVFObjC calls this method on the next run loop, there is
    an opportunity for those states to get out of sync, which happens when YouTube responds to a tap
    in its timeline.

    Remove the unnecessary "call on next run loop" behavior of MediaPlayerPrivateMediaSourceAVFObjC::
    play() and ::pause(). Also, remove the unnecessary notification that the play state has changed. In
    the future, this can be accomplished by adding a callback parameter to MediaPlayer::play() rather than
    relying on a state change notification.

    * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:
    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::play):
    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playInternal):
    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pause):
    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseInternal):
    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playAtHostTime):
    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseAtHostTime):

    LayoutTests:

    * media/media-source/media-source-unexpected-pause-expected.txt: Added.
    * media/media-source/media-source-unexpected-pause.html: Added.

    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291629 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Modified Paths

Added Paths

Diff

Modified: branches/safari-614.1.7-branch/LayoutTests/ChangeLog (291682 => 291683)


--- branches/safari-614.1.7-branch/LayoutTests/ChangeLog	2022-03-22 18:37:22 UTC (rev 291682)
+++ branches/safari-614.1.7-branch/LayoutTests/ChangeLog	2022-03-22 18:37:26 UTC (rev 291683)
@@ -1,3 +1,59 @@
+2022-03-22  Russell Epstein  <repst...@apple.com>
+
+        Cherry-pick r291629. rdar://problem/89495774
+
+    YouTube.com - Clicking anywhere on the progress bar pauses the video
+    https://bugs.webkit.org/show_bug.cgi?id=237750
+    <rdar://problem/90364846>
+    
+    Reviewed by Eric Carlson.
+    
+    Source/WebCore:
+    
+    Test: media/media-source/media-source-unexpected-pause.html
+    
+    When calling play() or pause() on a MediaPlayerPrivateMediaSourceAVFObjC, the object will
+    respond by calling m_player->playbackStateChanged(), which will in turn call
+    HTMLMediaElement::mediaPlayerPlaybackStateChanged() with the new state. However, HTMLMediaElement
+    expects this to be called only for unanticipated state changes, not expected state changes. And
+    when that method is called and the reported state does not match the element's own expected state,
+    the element calls its own play() or pause() function to update its own state to match the player's.
+    And because MediaPlayerPrivateMediaSourceAVFObjC calls this method on the next run loop, there is
+    an opportunity for those states to get out of sync, which happens when YouTube responds to a tap
+    in its timeline.
+    
+    Remove the unnecessary "call on next run loop" behavior of MediaPlayerPrivateMediaSourceAVFObjC::
+    play() and ::pause(). Also, remove the unnecessary notification that the play state has changed. In
+    the future, this can be accomplished by adding a callback parameter to MediaPlayer::play() rather than
+    relying on a state change notification.
+    
+    * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::play):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playInternal):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pause):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseInternal):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playAtHostTime):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseAtHostTime):
+    
+    LayoutTests:
+    
+    * media/media-source/media-source-unexpected-pause-expected.txt: Added.
+    * media/media-source/media-source-unexpected-pause.html: Added.
+    
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291629 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    2022-03-22  Jer Noble  <jer.no...@apple.com>
+
+            YouTube.com - Clicking anywhere on the progress bar pauses the video
+            https://bugs.webkit.org/show_bug.cgi?id=237750
+            <rdar://problem/90364846>
+
+            Reviewed by Eric Carlson.
+
+            * media/media-source/media-source-unexpected-pause-expected.txt: Added.
+            * media/media-source/media-source-unexpected-pause.html: Added.
+
 2022-03-18  Ryan Haddad  <ryanhad...@apple.com>
 
         [macOS arm64] webrtc/vp9-profile2.html is consistently timing out

Added: branches/safari-614.1.7-branch/LayoutTests/media/media-source/media-source-unexpected-pause-expected.txt (0 => 291683)


--- branches/safari-614.1.7-branch/LayoutTests/media/media-source/media-source-unexpected-pause-expected.txt	                        (rev 0)
+++ branches/safari-614.1.7-branch/LayoutTests/media/media-source/media-source-unexpected-pause-expected.txt	2022-03-22 18:37:26 UTC (rev 291683)
@@ -0,0 +1,17 @@
+
+RUN(video.src = ""
+EVENT(sourceopen)
+RUN(source.duration = loader.duration())
+RUN(sourceBuffer = source.addSourceBuffer(loader.type()))
+RUN(sourceBuffer.appendBuffer(loader.initSegment()))
+EVENT(update)
+Append a media segment.
+RUN(sourceBuffer.appendBuffer(loader.mediaSegment(0)))
+EVENT(update)
+RUN(video.play())
+EVENT(playing)
+RUN(video.pause())
+RUN(video.play())
+EVENT(playing)
+END OF TEST
+

Added: branches/safari-614.1.7-branch/LayoutTests/media/media-source/media-source-unexpected-pause.html (0 => 291683)


--- branches/safari-614.1.7-branch/LayoutTests/media/media-source/media-source-unexpected-pause.html	                        (rev 0)
+++ branches/safari-614.1.7-branch/LayoutTests/media/media-source/media-source-unexpected-pause.html	2022-03-22 18:37:26 UTC (rev 291683)
@@ -0,0 +1,62 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>media-source-unexpected-pause</title>
+    <script src=""
+    <script src=""
+    <script>
+    var loader;
+    var source;
+    var sourceBuffer;
+
+    function loaderPromise(loader) {
+        return new Promise((resolve, reject) => {
+            loader._onload_ = resolve;
+            loader._onerror_ = reject;
+        });
+    }
+
+    async function runTest() {
+        findMediaElement();
+        loader = new MediaSourceLoader('content/test-fragmented-manifest.json');
+        await loaderPromise(loader);
+
+        source = new MediaSource();
+        run('video.src = ""
+        await waitFor(source, 'sourceopen');
+        waitFor(source, 'error').then(endTest);
+        waitForEventAndFail('error');
+
+        run('source.duration = loader.duration()');
+        run('sourceBuffer = source.addSourceBuffer(loader.type())');
+        run('sourceBuffer.appendBuffer(loader.initSegment())');
+
+        await waitFor(sourceBuffer, 'update');
+
+        consoleWrite('Append a media segment.')
+        run('sourceBuffer.appendBuffer(loader.mediaSegment(0))');
+
+        await waitFor(sourceBuffer, 'update');
+        await sleepFor(100);
+
+        run('video.play()');
+        await waitFor(video, 'playing');
+        await sleepFor(100);
+        run('video.pause()');
+        await sleepFor(0);
+        await sleepFor(0);
+        run('video.play()');
+        waitForAndFail(video, 'pause');
+        await waitFor(video, 'playing');
+        await sleepFor(100);
+    }
+
+    window.addEventListener('load', event => {
+        runTest().then(endTest).catch(failTest);
+    });
+    </script>
+</head>
+<body>
+    <video muted controls></video>
+</body>
+</html>
\ No newline at end of file

Modified: branches/safari-614.1.7-branch/Source/WebCore/ChangeLog (291682 => 291683)


--- branches/safari-614.1.7-branch/Source/WebCore/ChangeLog	2022-03-22 18:37:22 UTC (rev 291682)
+++ branches/safari-614.1.7-branch/Source/WebCore/ChangeLog	2022-03-22 18:37:26 UTC (rev 291683)
@@ -1,3 +1,81 @@
+2022-03-22  Russell Epstein  <repst...@apple.com>
+
+        Cherry-pick r291629. rdar://problem/89495774
+
+    YouTube.com - Clicking anywhere on the progress bar pauses the video
+    https://bugs.webkit.org/show_bug.cgi?id=237750
+    <rdar://problem/90364846>
+    
+    Reviewed by Eric Carlson.
+    
+    Source/WebCore:
+    
+    Test: media/media-source/media-source-unexpected-pause.html
+    
+    When calling play() or pause() on a MediaPlayerPrivateMediaSourceAVFObjC, the object will
+    respond by calling m_player->playbackStateChanged(), which will in turn call
+    HTMLMediaElement::mediaPlayerPlaybackStateChanged() with the new state. However, HTMLMediaElement
+    expects this to be called only for unanticipated state changes, not expected state changes. And
+    when that method is called and the reported state does not match the element's own expected state,
+    the element calls its own play() or pause() function to update its own state to match the player's.
+    And because MediaPlayerPrivateMediaSourceAVFObjC calls this method on the next run loop, there is
+    an opportunity for those states to get out of sync, which happens when YouTube responds to a tap
+    in its timeline.
+    
+    Remove the unnecessary "call on next run loop" behavior of MediaPlayerPrivateMediaSourceAVFObjC::
+    play() and ::pause(). Also, remove the unnecessary notification that the play state has changed. In
+    the future, this can be accomplished by adding a callback parameter to MediaPlayer::play() rather than
+    relying on a state change notification.
+    
+    * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::play):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playInternal):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pause):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseInternal):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playAtHostTime):
+    (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseAtHostTime):
+    
+    LayoutTests:
+    
+    * media/media-source/media-source-unexpected-pause-expected.txt: Added.
+    * media/media-source/media-source-unexpected-pause.html: Added.
+    
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291629 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    2022-03-22  Jer Noble  <jer.no...@apple.com>
+
+            YouTube.com - Clicking anywhere on the progress bar pauses the video
+            https://bugs.webkit.org/show_bug.cgi?id=237750
+            <rdar://problem/90364846>
+
+            Reviewed by Eric Carlson.
+
+            Test: media/media-source/media-source-unexpected-pause.html
+
+            When calling play() or pause() on a MediaPlayerPrivateMediaSourceAVFObjC, the object will
+            respond by calling m_player->playbackStateChanged(), which will in turn call
+            HTMLMediaElement::mediaPlayerPlaybackStateChanged() with the new state. However, HTMLMediaElement
+            expects this to be called only for unanticipated state changes, not expected state changes. And
+            when that method is called and the reported state does not match the element's own expected state,
+            the element calls its own play() or pause() function to update its own state to match the player's.
+            And because MediaPlayerPrivateMediaSourceAVFObjC calls this method on the next run loop, there is
+            an opportunity for those states to get out of sync, which happens when YouTube responds to a tap
+            in its timeline.
+
+            Remove the unnecessary "call on next run loop" behavior of MediaPlayerPrivateMediaSourceAVFObjC::
+            play() and ::pause(). Also, remove the unnecessary notification that the play state has changed. In
+            the future, this can be accomplished by adding a callback parameter to MediaPlayer::play() rather than
+            relying on a state change notification.
+
+            * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:
+            (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::play):
+            (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playInternal):
+            (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pause):
+            (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseInternal):
+            (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::playAtHostTime):
+            (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::pauseAtHostTime):
+
 2022-03-21  Russell Epstein  <repst...@apple.com>
 
         Cherry-pick r291564. rdar://problem/90463946

Modified: branches/safari-614.1.7-branch/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm (291682 => 291683)


--- branches/safari-614.1.7-branch/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm	2022-03-22 18:37:22 UTC (rev 291682)
+++ branches/safari-614.1.7-branch/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm	2022-03-22 18:37:26 UTC (rev 291683)
@@ -314,11 +314,7 @@
 void MediaPlayerPrivateMediaSourceAVFObjC::play()
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }] {
-        if (!weakThis)
-            return;
-        weakThis.get()->playInternal();
-    });
+    playInternal();
 }
 
 void MediaPlayerPrivateMediaSourceAVFObjC::playInternal(std::optional<MonotonicTime>&& hostTime)
@@ -352,18 +348,12 @@
     UNUSED_PARAM(hostTime);
     [m_synchronizer setRate:m_rate];
 #endif
-
-    m_player->playbackStateChanged();
 }
 
 void MediaPlayerPrivateMediaSourceAVFObjC::pause()
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }] {
-        if (!weakThis)
-            return;
-        weakThis.get()->pauseInternal();
-    });
+    pauseInternal();
 }
 
 void MediaPlayerPrivateMediaSourceAVFObjC::pauseInternal(std::optional<MonotonicTime>&& hostTime)
@@ -384,8 +374,6 @@
     UNUSED_PARAM(hostTime);
     [m_synchronizer setRate:0];
 #endif
-
-    m_player->playbackStateChanged();
 }
 
 bool MediaPlayerPrivateMediaSourceAVFObjC::paused() const
@@ -479,11 +467,7 @@
 bool MediaPlayerPrivateMediaSourceAVFObjC::playAtHostTime(const MonotonicTime& time)
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }, time = time] {
-        if (!weakThis)
-            return;
-        weakThis.get()->playInternal(time);
-    });
+    playInternal(time);
     return true;
 }
 
@@ -490,11 +474,7 @@
 bool MediaPlayerPrivateMediaSourceAVFObjC::pauseAtHostTime(const MonotonicTime& time)
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }, time = time] {
-        if (!weakThis)
-            return;
-        weakThis.get()->pauseInternal(time);
-    });
+    pauseInternal(time);
     return true;
 }
 #endif
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to