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?
(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.
(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.
(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. (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.
Created attachment 84127 [details] Patch
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 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 on attachment 84127 [details] Patch Will comment on those bugs when this goes in. Could I get a cq+? Thanks!
Comment on attachment 84127 [details] Patch FYI - if you set cq? when requesting a review, the reviewer will generally cq+ when marking r+.
Comment on attachment 84127 [details] Patch Clearing flags on attachment: 84127 Committed r80017: <http://trac.webkit.org/changeset/80017>
All reviewed patches have been landed. Closing bug.