Bug 55198 - LayoutTests/http/tests/media/video-play-suspend is now hanging on chromium
Summary: LayoutTests/http/tests/media/video-play-suspend is now hanging on chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-24 17:01 PST by Steve Lacey
Modified: 2011-03-01 11:53 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.04 KB, patch)
2011-02-28 14:49 PST, Steve Lacey
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Lacey 2011-02-24 17:01:24 PST
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?
Comment 1 Eric Carlson 2011-02-24 21:11:23 PST
(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.
Comment 2 Steve Lacey 2011-02-25 09:30:56 PST
(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.
Comment 3 Eric Carlson 2011-02-25 09:35:44 PST
(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.
Comment 4 Steve Lacey 2011-02-25 10:31:46 PST
(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.
Comment 5 Steve Lacey 2011-02-28 14:49:08 PST
Created attachment 84127 [details]
Patch
Comment 6 Steve Lacey 2011-02-28 14:52:03 PST
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).
Comment 7 Eric Carlson 2011-03-01 09:39:57 PST
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?
Comment 8 Steve Lacey 2011-03-01 09:45:17 PST
Comment on attachment 84127 [details]
Patch

Will comment on those bugs when this goes in.

Could I get a cq+? Thanks!
Comment 9 Eric Carlson 2011-03-01 09:56:22 PST
Comment on attachment 84127 [details]
Patch

FYI - if you set cq? when requesting a review, the reviewer will generally cq+ when marking r+.
Comment 10 WebKit Commit Bot 2011-03-01 11:53:45 PST
Comment on attachment 84127 [details]
Patch

Clearing flags on attachment: 84127

Committed r80017: <http://trac.webkit.org/changeset/80017>
Comment 11 WebKit Commit Bot 2011-03-01 11:53:50 PST
All reviewed patches have been landed.  Closing bug.