Bug 228531

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: MediaAssignee: 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 Flags
patch to fix the issue
eric.carlson: review+
patch
eric.carlson: review+
patch none

Description Tomoki Imai 2021-07-27 21:30:35 PDT
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
Comment 1 Tomoki Imai 2021-07-27 21:35:50 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 2 Eric Carlson 2021-07-28 07:20:40 PDT
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;
Comment 3 Tomoki Imai 2021-07-28 19:56:45 PDT
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.
Comment 4 Eric Carlson 2021-07-29 08:10:46 PDT
(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 5 Eric Carlson 2021-07-29 08:17:20 PDT
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);
        ...
Comment 6 Tomoki Imai 2021-07-29 19:21:58 PDT
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.
Comment 7 EWS 2021-07-29 23:43:55 PDT
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].
Comment 8 Radar WebKit Bug Importer 2021-07-29 23:44:18 PDT
<rdar://problem/81311966>
Comment 9 Eric Carlson 2021-07-30 08:36:31 PDT
(In reply to Tomoki Imai from comment #6)
> 
> Thanks for your suggestion, I applied this to the patch.

Thank you for the fix!