Bug 228531 – Missing playing events when the ready state becomes HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA from HAVE_METADATA state

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Tomoki Imai]

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

[Tomoki Imai]

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

[Eric Carlson]

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;

[Tomoki Imai]

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.

[Eric Carlson]

(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.

[Eric Carlson]

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);
        ...

[Tomoki Imai]

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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/81311966>

[Eric Carlson]

(In reply to Tomoki Imai from comment #6)
> 
> Thanks for your suggestion, I applied this to the patch.

Thank you for the fix!