Changeset 291629 in webkit

Timestamp:

Mar 22, 2022 10:42:01 AM (4 months ago)

Author:

jer.noble@apple.com

Message:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/media/media-source/media-source-unexpected-pause-expected.txt (added)
• LayoutTests/media/media-source/media-source-unexpected-pause.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm (modified) (6 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2022-03-22  Jer Noble  <jer.noble@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-22  Ricky Mondello  <rmondello@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-03-22  Jer Noble  <jer.noble@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-22  J Pascoe  <j_pascoe@apple.com>
 

trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm

@@ -315 +315 @@
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }] {
-        if (!weakThis)
-            return;
-        weakThis.get()->playInternal();
-    });
+    playInternal();
 }
 
@@ -353 +349 @@
     [m_synchronizer setRate:m_rate];
 #endif
-
-    m_player->playbackStateChanged();
 }
 
@@ -360 +354 @@
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }] {
-        if (!weakThis)
-            return;
-        weakThis.get()->pauseInternal();
-    });
+    pauseInternal();
 }
 
@@ -385 +375 @@
     [m_synchronizer setRate:0];
 #endif
-
-    m_player->playbackStateChanged();
 }
 
@@ -480 +468 @@
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }, time = time] {
-        if (!weakThis)
-            return;
-        weakThis.get()->playInternal(time);
-    });
+    playInternal(time);
     return true;
 }
@@ -491 +475 @@
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    callOnMainThread([weakThis = WeakPtr { *this }, time = time] {
-        if (!weakThis)
-            return;
-        weakThis.get()->pauseInternal(time);
-    });
+    pauseInternal(time);
     return true;
 }