Bug 55198 – LayoutTests/http/tests/media/video-play-suspend is now hanging on chromium

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Steve Lacey]

This just started hanging for chrome due to a cl of mine (http://codereview.chromium.org/6581012/) which causes the MediaPlayer states to transition from 'loading -> loaded ->idle' instead of 'loading -> idle'. This appears to be the correct spec thing to do as a progress event is now fired when the media has completed loading (which it wasn't before), but a side effect is that the 'suspend' event is now not being fired as the network doesn't actually suspend (indicated by a 'loading -> idle' transition).

I ran the test under safari and the test also hangs there too.

So it seems to me that the test is actually not testing for correct behaviour.

Thoughts?

[Eric Carlson]

(In reply to comment #0)
> This just started hanging for chrome due to a cl of mine (http://codereview.chromium.org/6581012/) which causes the MediaPlayer states to transition from 'loading -> loaded ->idle' instead of 'loading -> idle'. This appears to be the correct spec thing to do as a progress event is now fired when the media has completed loading (which it wasn't before), but a side effect is that the 'suspend' event is now not being fired as the network doesn't actually suspend (indicated by a 'loading -> idle' transition).
> 
> I ran the test under safari and the test also hangs there too.
> 
> So it seems to me that the test is actually not testing for correct behaviour.
> 
> Thoughts?

'suspend' is to be fired when the UA stops downloading for some reason:

    When a media element's download has been suspended, the user agent must set the 
    networkState to NETWORK_IDLE and queue a task to fire a simple event named suspend 
    at the element.

so HTMLMediaElement fires it when the media engine signals a network state change from 'loading' or 'loaded' to MediaPlayer::Idle.  

I think the problem is in HTMLMediaElement::setNetworkState, where MediaPlayer::Loaded results in m_networkState being set to NETWORK_IDLE without firing a 'suspended' event. I think it should fire a 'suspend' event just before the final 'progress' event, though we will have to see if it causes other problems.

[Steve Lacey]

(In reply to comment #1)
> (In reply to comment #0)
> > This just started hanging for chrome due to a cl of mine (http://codereview.chromium.org/6581012/) which causes the MediaPlayer states to transition from 'loading -> loaded ->idle' instead of 'loading -> idle'. This appears to be the correct spec thing to do as a progress event is now fired when the media has completed loading (which it wasn't before), but a side effect is that the 'suspend' event is now not being fired as the network doesn't actually suspend (indicated by a 'loading -> idle' transition).
> > 
> > I ran the test under safari and the test also hangs there too.
> > 
> > So it seems to me that the test is actually not testing for correct behaviour.
> > 
> > Thoughts?
> 
> 'suspend' is to be fired when the UA stops downloading for some reason:
> 
>     When a media element's download has been suspended, the user agent must set the 
>     networkState to NETWORK_IDLE and queue a task to fire a simple event named suspend 
>     at the element.
> 
> so HTMLMediaElement fires it when the media engine signals a network state change from 'loading' or 'loaded' to MediaPlayer::Idle.  
> 
> I think the problem is in HTMLMediaElement::setNetworkState, where MediaPlayer::Loaded results in m_networkState being set to NETWORK_IDLE without firing a 'suspended' event. I think it should fire a 'suspend' event just before the final 'progress' event, though we will have to see if it causes other problems.

I also see this in the spec for the suspend event:

      The user agent is intentionally not currently fetching media data, but does not have the entire
      media resource downloaded.

Which implies to me that you get a suspend event if download stops but there's more data to download.

The sentences before and after the paragraph you list also imply (to me) that if download is suspended but there's more to download, that's when you get a suspend event. There's no wordage that implies you get a suspend event in any other situation...

Confusion (in my mind at least :-) abounds.

[Eric Carlson]

(In reply to comment #2)
> (In reply to comment #1)
> 
> I also see this in the spec for the suspend event:
> 
>       The user agent is intentionally not currently fetching media data, but does not have the entire
>       media resource downloaded.
> 
> Which implies to me that you get a suspend event if download stops but there's more data to download.

  Indeed, it does say that in the summary. I guess I must have read the spec more carefully when I wrote setNetworkState - good thing! 

> 
> Confusion (in my mind at least :-) abounds.

So propose a fix.

[Steve Lacey]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > 
> > I also see this in the spec for the suspend event:
> > 
> >       The user agent is intentionally not currently fetching media data, but does not have the entire
> >       media resource downloaded.
> > 
> > Which implies to me that you get a suspend event if download stops but there's more data to download.
> 
>   Indeed, it does say that in the summary. I guess I must have read the spec more carefully when I wrote setNetworkState - good thing! 
> 
> > 
> > Confusion (in my mind at least :-) abounds.
> 
> So propose a fix.

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > 
> > I also see this in the spec for the suspend event:
> > 
> >       The user agent is intentionally not currently fetching media data, but does not have the entire
> >       media resource downloaded.
> > 
> > Which implies to me that you get a suspend event if download stops but there's more data to download.
> 
>   Indeed, it does say that in the summary. I guess I must have read the spec more carefully when I wrote setNetworkState - good thing! 
> 
> > 
> > Confusion (in my mind at least :-) abounds.
> 
> So propose a fix.

My suggestion would be to create a new layout test basically the same as video-play-suspend, but called video-play-progress and does the same thing but expects a progress event.

I'll start working on this.

Then mod video-play-suspend to cause a stall and expect a suspend event. I'm not sure how to do this or do it in a non-flakey manner as it would rely on the networking internals of the port.

[Steve Lacey]

Created attachment 84127 [details]
Patch

[Steve Lacey]

The attached patch:

- removes video-play-suspend as it was basically an incorrect test. I don't believe it's possible to test this deterministically as it depends on port player internals
- introduces video-play-progress that tests that at least one progress event is fired. Ideally we'd test that progress events are fired every 350ms (flaky...) and that a final progress event is fired after all media is loaded (but it's unclear how to determine this state).

[Eric Carlson]

Comment on attachment 84127 [details]
Patch

Will you also please add comments to bugs 37034, 37035, and 37036 to clarify what they should do since the description talks about the 'suspend' event?

[Steve Lacey]

Comment on attachment 84127 [details]
Patch

Will comment on those bugs when this goes in.

Could I get a cq+? Thanks!

[Eric Carlson]

Comment on attachment 84127 [details]
Patch

FYI - if you set cq? when requesting a review, the reviewer will generally cq+ when marking r+.

[WebKit Commit Bot]

Comment on attachment 84127 [details]
Patch

Clearing flags on attachment: 84127

Committed r80017: <http://trac.webkit.org/changeset/80017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.