[GStreamer] Player doesn't return to NETWORK_IDLE when not using network

[Andrew Scherkus]

In order to implement the suspend event, MediaPlayerPrivate implementations should return to NETWORK_IDLE when not using the network.

[Philippe Normand]

I don't see NETWORK_LOADED in the spec anymore while we still use it.

http://www.w3.org/TR/html5/video.html#dom-media-networkstate

Should we replace Loaded with Idle?

[Philippe Normand]

BTW the test to fix/unskip is http/tests/media/video-play-suspend.html

[Philippe Normand]

LOADED is used when a media is fully buffered I think
and IDLE is when network is not used, so I guess it's kind of not the same ;)

I'm still confused though to still see we use LOADED and the spec not mentionning it anymore, or I'm reading the wrong one.

[Eric Carlson]

(In reply to comment #1)
> I don't see NETWORK_LOADED in the spec anymore while we still use it.
> 
> http://www.w3.org/TR/html5/video.html#dom-media-networkstate
> 
NETWORK_LOADING and the 'loaded' event went away in r63684 (and a few follow-on build fixes ;-).


(In reply to comment #3)
> LOADED is used when a media is fully buffered I think
> and IDLE is when network is not used, so I guess it's kind of not the same ;)
> 
> I'm still confused though to still see we use LOADED and the spec not mentionning it 
> anymore, or I'm reading the wrong one.
We still have the internal state MediaPlayer::Loaded, but that translates to networkState == NETWORK_IDLE. This internal state is necessary so we can tell if we need to fire the 'suspend' event or not. The spec says this about the 'suspend' event:

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

http://www.w3.org/TR/html5/video.html#mediaevents

The 'suspend' event is only fired when the media engine sets networkState to MediaPlayer::Idle itself.

[Philippe Normand]

(In reply to comment #4)
> (In reply to comment #3)
> > LOADED is used when a media is fully buffered I think
> > and IDLE is when network is not used, so I guess it's kind of not the same ;)
> > 
> > I'm still confused though to still see we use LOADED and the spec not mentionning it 
> > anymore, or I'm reading the wrong one.
> We still have the internal state MediaPlayer::Loaded, but that translates to networkState == NETWORK_IDLE. This internal state is necessary so we can tell if we need to fire the 'suspend' event or not. The spec says this about the 'suspend' event:
> 
> The user agent is intentionally not currently fetching media data, but does not have the entire media resource downloaded.
> 
> http://www.w3.org/TR/html5/video.html#mediaevents
> 
> The 'suspend' event is only fired when the media engine sets networkState to MediaPlayer::Idle itself.

Hum does that mean we should set networkState to Idle when the media is fully loaded?

Sorry but I don't see how to make that test pass without doing that, simply because the media is local, download is instantaneous, so I don't see why a suspend event should be fired in this case.

In progressive download mode GStreamer will keep the download running even if the playback is paused, until it fully completes, AFAIK.

[Steve Lacey]

FYI, http/tests/media/video-play-suspend.html has been removed as the test isn't actually valid according to spec. See https://bugs.webkit.org/show_bug.cgi?id=55198 for details.

[Philippe Normand]

Thanks for the heads up Steve! Seems like this new test is passing on GTK. Does this mean we can close this bug?

[Steve Lacey]

You need to make sure you set the network state to loaded before transitioning to the final idle so that a final progress event is fired (see HTMLMediaElement.cpp). If you're doing this, all should be good.

[Philippe Normand]

Created attachment 85687 [details]
proposed patch

[Martin Robinson]

Comment on attachment 85687 [details]
proposed patch

Are there any tests covering this?

[Philippe Normand]

(In reply to comment #10)
> (From update of attachment 85687 [details])
> Are there any tests covering this?

Hum it doesn't seem so. I'll try to write one :) Will clear the r+ flag for now.

[Martin Robinson]

If writing a test is a lot of work, it's probably okay just to land this as-is.

[Xabier Rodríguez Calvar]

There is a http/tests/media/video-load-suspend.html test and it seems to check out now. I think we can close this.