Summary: | Missing playing events when the ready state becomes HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA from HAVE_METADATA state | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tomoki Imai <tomoki.imai> | ||||||||
Component: | Media | Assignee: | Tomoki Imai <tomoki.imai> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | calvaris, cdumez, changseok, don.olmstead, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, philipj, sergio, stephan.szabo, tomoki.imai, toshio.ogasawara, webkit-bug-importer, Yousuke.Kimoto | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Tomoki Imai
2021-07-27 21:30:35 PDT
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]. (In reply to Tomoki Imai from comment #6) > > Thanks for your suggestion, I applied this to the patch. Thank you for the fix! |