Skip to content
Permalink
Browse files
Missing playing events when the ready state becomes HAVE_FUTURE_DATA/…
…HAVE_ENOUGH_DATA from HAVE_METADATA state

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

Reviewed by Eric Carlson.

Source/WebCore:

The main issue is that missing playing event when the ready state becomes HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA from HAVE_METADATA.
According to the specification, we need to "notify about playing" in the following cases:
- If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA, and it's not paused.
- If the new ready state is HAVE_ENOUGH_DATA, and it's eligible for autoplay
The implementation didn't cover these cases and had web compatibility issues.

We also should move scheduleNotifyAboutPlaying from setPlaying to playInternal and checks the ready state.
- Without this change, playing event is fired twice. The first one is fired by setReadyState, and the second is called from setPlaying.
- According to the specification, scheduleNotifyAboutPlaying should be in  "internal play steps" and check the ready state.
  Checking ready state fixes the issue where playing event is fired twice.

Test: media/media-source/media-source-monitor-playing-event.html

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::setReadyState): Added missing scheduleNotifyAboutPlaying calls. Added do-while to make the implementation similar to the specification text
(WebCore::HTMLMediaElement::playInternal): Added scheduleNotifyAboutPlaying call.
                                           According to the specification, "internal play steps" should "notify about playing" when
                                           the ready state is HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA.
(WebCore::HTMLMediaElement::setPlaying): Removed scheduleNotifyAboutPlaying call because playInternal now calls scheduleNotifyAboutPlaying instead.

Reference:
- https://html.spec.whatwg.org/multipage/media.html#internal-play-steps
- https://html.spec.whatwg.org/multipage/media.html#ready-states

LayoutTests:

Added the testcase to checks if the playing event is fired correctly on the ready state changes.

* media/media-source/media-source-monitor-playing-event-expected.txt: Added.
* media/media-source/media-source-monitor-playing-event.html: Added.


Canonical link: https://commits.webkit.org/240103@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280468 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
tomoki committed Jul 30, 2021
1 parent f11022e commit 8c01c82de4ced778766076fc24f78ac634fe6ec4
Showing 5 changed files with 256 additions and 45 deletions.
@@ -1,3 +1,15 @@
2021-07-29 Tomoki Imai <Tomoki.Imai@sony.com>

Missing playing events when the ready state becomes HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA from HAVE_METADATA state
https://bugs.webkit.org/show_bug.cgi?id=228531

Reviewed by Eric Carlson.

Added the testcase to checks if the playing event is fired correctly on the ready state changes.

* media/media-source/media-source-monitor-playing-event-expected.txt: Added.
* media/media-source/media-source-monitor-playing-event.html: Added.

2021-07-29 Myles C. Maxfield <mmaxfield@apple.com>

Stop building WebGPU and the WHLSL compiler to decrease binary size
@@ -0,0 +1,33 @@
This test checks if the playing event fires when the ready state changes from HAVE_METADATA to HAVE_FUTURE_DATA or higher.

EXPECTED (source.readyState == 'closed') OK
EVENT(loadstart)
EVENT(sourceopen)
RUN(sourceBuffer.appendBuffer(initSegment))
EVENT(loadedmetadata)
EVENT(updateend)
video.readyState : HAVE_METADATA
RUN(sourceBuffer.appendBuffer(sample))
EVENT(updateend)
video.readyState : HAVE_FUTURE_DATA
RUN(sourceBuffer.appendBuffer(sample))
EVENT(loadeddata)
EVENT(canplay)
EVENT(updateend)
EVENT(canplaythrough)
EVENT(playing)
video.readyState : HAVE_ENOUGH_DATA
RUN(sourceBuffer.remove(0,10))
EVENT(updateend)
EVENT(waiting)
video.readyState : HAVE_METADATA
RUN(sourceBuffer.appendBuffer(sample))
EVENT(updateend)
video.readyState : HAVE_METADATA
RUN(sourceBuffer.appendBuffer(sample))
EVENT(updateend)
EVENT(canplay)
EVENT(playing)
video.readyState : HAVE_ENOUGH_DATA
END OF TEST

@@ -0,0 +1,92 @@
<!DOCTYPE html>
<html>
<head>
<title>media-source-monitor-playing-event</title>
<script src="mock-media-source.js"></script>
<script src="../video-test.js"></script>
<script>
var source;
var sourceBuffer;
var initSegment;
var sample;
var handleVideoEvents = [
"loadstart",
"waiting",
"loadedmetadata",
"loadeddata",
"canplay",
"canplaythrough",
"pause",
"ended",
];
var readyStateString = [
"HAVE_NOTHING",
"HAVE_METADATA",
"HAVE_CURRENT_DATA",
"HAVE_FUTURE_DATA",
"HAVE_ENOUGH_DATA"
];

if (window.internals)
internals.initializeMockMediaSource();

window.addEventListener('load', async() => {
findMediaElement();

for (var i = 0; i < handleVideoEvents.length ; i++)
waitForEvent(handleVideoEvents[i]);

source = new MediaSource();
testExpected('source.readyState', 'closed');

const videoSource = document.createElement('source');
videoSource.type = 'video/mock; codecs=mock';
videoSource.src = URL.createObjectURL(source);
video.appendChild(videoSource);
await waitFor(source, 'sourceopen');

sourceBuffer = source.addSourceBuffer("video/mock; codecs=mock");
initSegment = makeAInit(100, [makeATrack(1, 'mock', TRACK_KIND.VIDEO)]);
run('sourceBuffer.appendBuffer(initSegment)');
await waitFor(sourceBuffer, 'updateend');

consoleWrite('video.readyState : ' + readyStateString[video.readyState]);
sample = makeASample(0, 0, 1, 1, 1, SAMPLE_FLAG.SYNC, 1);
// This append changes ready state to HAVE_FUTURE_DATA.
run('sourceBuffer.appendBuffer(sample)');
await waitFor(sourceBuffer, 'updateend');

consoleWrite('video.readyState : ' + readyStateString[video.readyState]);
sample = makeASample(1, 1, 9, 1, 1, SAMPLE_FLAG.SYNC, 1);
// This append changes the ready state to HAVE_ENOUGH_DATA and fires the playing event.
run('sourceBuffer.appendBuffer(sample)');
await Promise.all([waitFor(mediaElement, 'playing'), waitFor(sourceBuffer, 'updateend')]);

consoleWrite('video.readyState : ' + readyStateString[video.readyState]);
// This remove changes ready state to HAVE_METADATA.
run('sourceBuffer.remove(0,10)');
await waitFor(sourceBuffer, 'updateend');

await sleepFor(1000);

consoleWrite('video.readyState : ' + readyStateString[video.readyState]);
sample = makeASample(0, 0, 1, 1, 1, SAMPLE_FLAG.SYNC, 1);
run('sourceBuffer.appendBuffer(sample)');
await waitFor(sourceBuffer, 'updateend');

consoleWrite('video.readyState : ' + readyStateString[video.readyState]);
sample = makeASample(1, 1, 9, 1, 1, SAMPLE_FLAG.SYNC, 1);
// This append changes the ready state to HAVE_ENOUGH_DATA and fires the playing event.
run('sourceBuffer.appendBuffer(sample)');
await Promise.all([waitFor(mediaElement, 'playing'), waitFor(sourceBuffer, 'updateend')]);

consoleWrite('video.readyState : ' + readyStateString[video.readyState]);
endTest();
});
</script>
</head>
<body>
<div>This test checks if the playing event fires when the ready state changes from HAVE_METADATA to HAVE_FUTURE_DATA or higher.</div>
<video autoplay></video>
</body>
</html>
@@ -1,3 +1,34 @@
2021-07-29 Tomoki Imai <Tomoki.Imai@sony.com>

Missing playing events when the ready state becomes HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA from HAVE_METADATA state
https://bugs.webkit.org/show_bug.cgi?id=228531

Reviewed by Eric Carlson.

The main issue is that missing playing event when the ready state becomes HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA from HAVE_METADATA.
According to the specification, we need to "notify about playing" in the following cases:
- If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA, and it's not paused.
- If the new ready state is HAVE_ENOUGH_DATA, and it's eligible for autoplay
The implementation didn't cover these cases and had web compatibility issues.

We also should move scheduleNotifyAboutPlaying from setPlaying to playInternal and checks the ready state.
- Without this change, playing event is fired twice. The first one is fired by setReadyState, and the second is called from setPlaying.
- According to the specification, scheduleNotifyAboutPlaying should be in "internal play steps" and check the ready state.
Checking ready state fixes the issue where playing event is fired twice.

Test: media/media-source/media-source-monitor-playing-event.html

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::setReadyState): Added missing scheduleNotifyAboutPlaying calls. Added do-while to make the implementation similar to the specification text
(WebCore::HTMLMediaElement::playInternal): Added scheduleNotifyAboutPlaying call.
According to the specification, "internal play steps" should "notify about playing" when
the ready state is HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA.
(WebCore::HTMLMediaElement::setPlaying): Removed scheduleNotifyAboutPlaying call because playInternal now calls scheduleNotifyAboutPlaying instead.

Reference:
- https://html.spec.whatwg.org/multipage/media.html#internal-play-steps
- https://html.spec.whatwg.org/multipage/media.html#ready-states

2021-07-29 Myles C. Maxfield <mmaxfield@apple.com>

Stop building WebGPU and the WHLSL compiler to decrease binary size
@@ -2403,65 +2403,105 @@ void HTMLMediaElement::setReadyState(MediaPlayer::ReadyState state)
}
}

if (m_readyState >= HAVE_METADATA && oldState < HAVE_METADATA) {
prepareMediaFragmentURI();
scheduleEvent(eventNames().durationchangeEvent);
scheduleResizeEvent();
scheduleEvent(eventNames().loadedmetadataEvent);
// Apply the first applicable set of substeps from the following list:
do {
// FIXME: The specification seems to only say HAVE_METADATA
// explicitly (rather than or higher) for this state. It's unclear
// if/how things like loadedmetadataEvent should happen if
// we go directly from below HAVE_METADATA to higher than
// HAVE_METADATA.
if (m_readyState >= HAVE_METADATA && oldState < HAVE_METADATA) {
prepareMediaFragmentURI();
scheduleEvent(eventNames().durationchangeEvent);
scheduleResizeEvent();
scheduleEvent(eventNames().loadedmetadataEvent);
#if ENABLE(WIRELESS_PLAYBACK_TARGET)
if (hasEventListeners(eventNames().webkitplaybacktargetavailabilitychangedEvent))
enqueuePlaybackTargetAvailabilityChangedEvent();
if (hasEventListeners(eventNames().webkitplaybacktargetavailabilitychangedEvent))
enqueuePlaybackTargetAvailabilityChangedEvent();
#endif
m_initiallyMuted = m_volume < 0.05 || muted();
m_initiallyMuted = m_volume < 0.05 || muted();

updateRenderer();
updateRenderer();

if (is<MediaDocument>(document()))
downcast<MediaDocument>(document()).mediaElementNaturalSizeChanged(expandedIntSize(m_player->naturalSize()));
if (is<MediaDocument>(document()))
downcast<MediaDocument>(document()).mediaElementNaturalSizeChanged(expandedIntSize(m_player->naturalSize()));

logMediaLoadRequest(document().page(), m_player->engineDescription(), String(), true);
logMediaLoadRequest(document().page(), m_player->engineDescription(), String(), true);

#if ENABLE(WIRELESS_PLAYBACK_TARGET)
scheduleUpdateMediaState();
scheduleUpdateMediaState();
#endif

mediaSession().clientCharacteristicsChanged();
}
mediaSession().clientCharacteristicsChanged();

if (m_readyState >= HAVE_CURRENT_DATA && oldState < HAVE_CURRENT_DATA) {
if (!m_haveFiredLoadedData) {
m_haveFiredLoadedData = true;
scheduleEvent(eventNames().loadeddataEvent);
// FIXME: It's not clear that it's correct to skip these this operation just
// because m_haveFiredLoadedData is already true. At one time we were skipping
// the call to setShouldDelayLoadEvent, which was definitely incorrect.
applyMediaFragmentURI();
// As the spec only mentiones HAVE_METADATA, run the later
// steps if we are moving to a higher state.
if (m_readyState == HAVE_METADATA)
break;
}
setShouldDelayLoadEvent(false);
}

if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady)
scheduleEvent(eventNames().canplayEvent);
if (m_readyState >= HAVE_CURRENT_DATA && oldState < HAVE_CURRENT_DATA) {
if (!m_haveFiredLoadedData) {
m_haveFiredLoadedData = true;
scheduleEvent(eventNames().loadeddataEvent);
// FIXME: It's not clear that it's correct to skip these this operation just
// because m_haveFiredLoadedData is already true. At one time we were skipping
// the call to setShouldDelayLoadEvent, which was definitely incorrect.
applyMediaFragmentURI();
}
setShouldDelayLoadEvent(false);

// If the new ready state is HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA, then the relevant steps below must then be run also.
if (m_readyState < HAVE_FUTURE_DATA)
break;
}

if (!tracksAreReady)
break;

if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA && tracksAreReady) {
if (oldState <= HAVE_CURRENT_DATA)
if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA) {
scheduleEvent(eventNames().canplayEvent);

scheduleEvent(eventNames().canplaythroughEvent);
// If the element’s paused attribute is false, the user agent must queue a task to fire a simple event named playing at the element.
if (!paused())
scheduleNotifyAboutPlaying();
break;
}

auto canTransition = canTransitionFromAutoplayToPlay();
if (canTransition) {
m_paused = false;
setShowPosterFlag(false);
invalidateCachedTime();
setAutoplayEventPlaybackState(AutoplayEventPlaybackState::StartedWithoutUserGesture);
m_playbackStartedTime = currentMediaTime().toDouble();
scheduleEvent(eventNames().playEvent);
} else if (canTransition.error() == MediaPlaybackDenialReason::UserGestureRequired) {
ALWAYS_LOG(LOGIDENTIFIER, "Autoplay blocked, user gesture required");
setAutoplayEventPlaybackState(AutoplayEventPlaybackState::PreventedAutoplay);
if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA) {
// If the previous ready state was HAVE_CURRENT_DATA or less,
// the user agent must queue a media element task given the media element to fire an event named canplay at the element,
// and, if the element's paused attribute is false, notify about playing for the element.
if (oldState <= HAVE_CURRENT_DATA) {
scheduleEvent(eventNames().canplayEvent);
if (!paused())
scheduleNotifyAboutPlaying();
}

// The user agent must queue a media element task given the media element to fire an event named canplaythrough at the element.
scheduleEvent(eventNames().canplaythroughEvent);

// If the element is not eligible for autoplay, then the user agent must abort these substeps.
// The user agent may run the following substeps:
// Set the paused attribute to false.
// If the element's show poster flag is true, set it to false and run the time marches on steps.
// Queue a media element task given the element to fire an event named play at the element.
// Notify about playing for the element.
auto canTransition = canTransitionFromAutoplayToPlay();
if (canTransition) {
m_paused = false;
setShowPosterFlag(false);
invalidateCachedTime();
setAutoplayEventPlaybackState(AutoplayEventPlaybackState::StartedWithoutUserGesture);
m_playbackStartedTime = currentMediaTime().toDouble();
scheduleEvent(eventNames().playEvent);
scheduleNotifyAboutPlaying();
} else if (canTransition.error() == MediaPlaybackDenialReason::UserGestureRequired) {
ALWAYS_LOG(LOGIDENTIFIER, "Autoplay blocked, user gesture required");
setAutoplayEventPlaybackState(AutoplayEventPlaybackState::PreventedAutoplay);
}
}
}
} while (false);

// If we transition to the Future Data state and we're about to begin playing, ensure playback is actually permitted first,
// honoring any playback denial reasons such as the requirement of a user gesture.
@@ -3545,8 +3585,14 @@ void HTMLMediaElement::playInternal()

scheduleEvent(eventNames().playEvent);

// If the media element's readyState attribute has the value HAVE_NOTHING, HAVE_METADATA, or HAVE_CURRENT_DATA,
// queue a media element task given the media element to fire an event named waiting at the element.
// Otherwise, the media element's readyState attribute has the value HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA:
// notify about playing for the element.
if (m_readyState <= HAVE_CURRENT_DATA)
scheduleEvent(eventNames().waitingEvent);
else
scheduleNotifyAboutPlaying();
} else if (m_readyState >= HAVE_FUTURE_DATA)
scheduleResolvePendingPlayPromises();

@@ -5482,9 +5528,6 @@ void HTMLMediaElement::setPlaying(bool playing)

m_playing = playing;

if (m_playing)
scheduleNotifyAboutPlaying();

document().updateIsPlayingMedia();

#if ENABLE(WIRELESS_PLAYBACK_TARGET)

0 comments on commit 8c01c82

Please sign in to comment.