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 https://html.spec.whatwg.org/multipage/media.html#ready-states But the current WebKit implementation doesn't fire playing event these scenarios, which may cause the web compatibility issues on some video services. Reproduced in the following environments: - GTK MiniBrowser r239946 on Ubuntu 20.04 - Safari Technology Preview 124 on macOS 10.15.7 Not reproduced in the following environments: - Google Chrome 91.0.4472.164 on macOS 10.15.7 - Firefox 90.0.2 on macOS 10.15.7
Created attachment 434400 [details] patch to fix the issue This patch do the followings: - Add the testcase for replicate the issue - Add the missing playing events in setReadyStates - Move "notify about playing" from setPlaying to playInternal
Comment on attachment 434400 [details] patch to fix the issue View in context: https://bugs.webkit.org/attachment.cgi?id=434400&action=review > Source/WebCore/ChangeLog:16 > + - 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. Nit: it would be nice to wrap this line to make it possible to read without scrolling on a small screen > Source/WebCore/ChangeLog:22 > + (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. Ditto > Source/WebCore/html/HTMLMediaElement.cpp:2442 > + if (!(m_readyState == HAVE_FUTURE_DATA || m_readyState == HAVE_ENOUGH_DATA)) This could be simplified to `if (m_readyState < HAVE_FUTURE_DATA)` > Source/WebCore/html/HTMLMediaElement.cpp:2446 > + if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady) { Can't you out here break if tracks aren't ready: if (!tracksAreReady) break; > Source/WebCore/html/HTMLMediaElement.cpp:2455 > + if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA && tracksAreReady) { I think this would be easier to read if you break if the condition isn't true instead of indenting: if (m_readyState != HAVE_ENOUGH_DATA || oldState >= HAVE_ENOUGH_DATA) break;
Created attachment 434491 [details] patch (In reply to Eric Carlson from comment #2) Thanks for your review! I reflected your comments except the last one to the patch. Is it acceptable for you? > > Source/WebCore/html/HTMLMediaElement.cpp:2455 > > + if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA && tracksAreReady) { > > I think this would be easier to read if you break if the condition isn't > true instead of indenting: > > if (m_readyState != HAVE_ENOUGH_DATA || oldState >= HAVE_ENOUGH_DATA) > break; In my opinion, we should keep the current style even if there is an additional identing, because the current code reflects the specification structure. I feel it's a bit confusing to apply the different style if condition from the other ones. ``` Apply the first applicable set of substeps from the following list: - If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA ... - If the new ready state is HAVE_ENOUGH_DATA ... ``` https://html.spec.whatwg.org/multipage/media.html#ready-states > Comment on attachment 434400 [details] > patch to fix the issue > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434400&action=review > > > Source/WebCore/ChangeLog:16 > > + - 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. > > Nit: it would be nice to wrap this line to make it possible to read without > scrolling on a small screen Agreed, fixed. > > > Source/WebCore/ChangeLog:22 > > + (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. > > Ditto Fixed. > > > Source/WebCore/html/HTMLMediaElement.cpp:2442 > > + if (!(m_readyState == HAVE_FUTURE_DATA || m_readyState == HAVE_ENOUGH_DATA)) > > This could be simplified to `if (m_readyState < HAVE_FUTURE_DATA)` Thanks for your suggestion, applied. At first I thought the current code might be better because this if condition comes from the specification text. But rethinked that `if (m_readyState < HAVE_FUTURE_DATA)` is better on the performance and there is no confusion because we have the comment about the specification. > > > Source/WebCore/html/HTMLMediaElement.cpp:2446 > > + if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady) { > > Can't you out here break if tracks aren't ready: > > if (!tracksAreReady) > break; Thanks, moving tracksAreReady if condition sounds good. Fixed.
(In reply to Tomoki Imai from comment #3) > Created attachment 434491 [details] > patch > > (In reply to Eric Carlson from comment #2) > > Thanks for your review! > I reflected your comments except the last one to the patch. > > Is it acceptable for you? > > > > Source/WebCore/html/HTMLMediaElement.cpp:2455 > > > + if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA && tracksAreReady) { > > > > I think this would be easier to read if you break if the condition isn't > > true instead of indenting: > > > > if (m_readyState != HAVE_ENOUGH_DATA || oldState >= HAVE_ENOUGH_DATA) > > break; > > In my opinion, we should keep the current style even if there is an > additional identing, because the current code reflects the specification > structure. > I feel it's a bit confusing to apply the different style if condition from > the other ones. > > ``` > Apply the first applicable set of substeps from the following list: > - If the previous ready state was HAVE_CURRENT_DATA or less, and the new > ready state is HAVE_FUTURE_DATA > ... > - If the new ready state is HAVE_ENOUGH_DATA > ... > ``` > https://html.spec.whatwg.org/multipage/media.html#ready-states > That seems fine.
Comment on attachment 434491 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=434491&action=review r=me with one minor suggested change This has an r+, so you don't need another review. When you post your new patch, replace "NOBODY (OOPS!)" with "Eric Carlson" and just ask for a commit and any reviewer can land it: webkit-patch upload --no-review --request-commit ... > Source/WebCore/html/HTMLMediaElement.cpp:2448 > + if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA) { > + if (!tracksAreReady) > + break; I meant you could move the tracks test above this as we don't want to do either test unless track data is available: if (!tracksAreReady) break; if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA) { scheduleEvent(eventNames().canplayEvent); ...
Created attachment 434598 [details] patch (In reply to Eric Carlson from comment #5) > Comment on attachment 434491 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434491&action=review > > r=me with one minor suggested change > > This has an r+, so you don't need another review. When you post your new > patch, replace "NOBODY (OOPS!)" with "Eric Carlson" and just ask for a > commit and any reviewer can land it: webkit-patch upload --no-review > --request-commit ... Thank you for your instruction, I added your name and will cq+ after it passes EWS tests. > > Source/WebCore/html/HTMLMediaElement.cpp:2448 > > + if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA) { > > + if (!tracksAreReady) > > + break; > > I meant you could move the tracks test above this as we don't want to do > either test unless track data is available: > > if (!tracksAreReady) > break; > > if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA) { > scheduleEvent(eventNames().canplayEvent); > ... Thanks for your suggestion, I applied this to the patch.
Committed r280468 (240103@main): <https://commits.webkit.org/240103@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434598 [details].
<rdar://problem/81311966>
(In reply to Tomoki Imai from comment #6) > > Thanks for your suggestion, I applied this to the patch. Thank you for the fix!