1108838 - dispatch "stalled" even when no bytes have been received

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Karl Tomlinson (:karlt)]

This is important for MediaSource, where there is no initial HTTP request start to set up the stall counter by sending an initial progress event.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: move stalled/progress timing from MediaDecoder to HTMLMediaElement

This provides that mNetworkState is available for determining whether progress
notification from the resource/MediaCache after stalled should resume progress
events.  The timer will be stopped while stalled in a subsequent patch, and
should not be restarted unless NETWORK_LOADING.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: dispatch "stalled" even when no bytes have been received

Also reset stalled timer on transitions to NETWORK_LOADING,
and don't run the progress timer while stalled.

For sources using ChannelMediaResource, this means that "stalled" can now fire
before an HTTP response is received.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8533414 [details] [diff] [review]
move stalled/progress timing from MediaDecoder to HTMLMediaElement

Review of attachment 8533414 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +2643,5 @@
>    mDecoder->SetAudioChannel(mAudioChannel);
>    mDecoder->SetAudioCaptured(mAudioCaptured);
>    mDecoder->SetVolume(mMuted ? 0.0 : mVolume);
>    mDecoder->SetPreservesPitch(mPreservesPitch);
>    mDecoder->SetPlaybackRate(mPlaybackRate);

You have a behaviour change here; you removed the StartProgress() call. I guess that's required for your networkState assertions to pass?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8533416 [details] [diff] [review]
dispatch "stalled" even when no bytes have been received

Review of attachment 8533416 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +3079,5 @@
>    if (aHaveNewProgress) {
>      mDataTime = now;
>    }
>  
>    // If this is the first progress, or PROGRESS_MS has passed since the last

I feel like you should assert that mDataTime is non null before the logic below...

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 8533414 [details] [diff] [review]
> move stalled/progress timing from MediaDecoder to HTMLMediaElement

> ::: dom/html/HTMLMediaElement.cpp
> @@ +2643,5 @@
> >    mDecoder->SetAudioChannel(mAudioChannel);
> >    mDecoder->SetAudioCaptured(mAudioCaptured);
> >    mDecoder->SetVolume(mMuted ? 0.0 : mVolume);
> >    mDecoder->SetPreservesPitch(mPreservesPitch);
> >    mDecoder->SetPlaybackRate(mPlaybackRate);
> 
> You have a behaviour change here; you removed the StartProgress() call. I
> guess that's required for your networkState assertions to pass?

HTMLMediaElement::FinishDecoderSetup() calls ChangeNetworkState() before
setting mDecoder above these lines.

Previously StartProgress() was not called from ChangeNetworkState() because
mDecoder was not set, and so an additional call was required after mDecoder
was set.  Now, ChangeNetworkState() will StartProgress() even when mDecoder is
not set and so the additional call is no longer required.

I'm not intending to change behavior here, merely removing code that is no
longer required (because it has no additional effect).

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Chris Pearce (:cpearce) from comment #4)
> I feel like you should assert that mDataTime is non null before the logic
> below...

It may be null if no progress events have been dispatched, so
I've added

  NS_ASSERTION((mProgressTime.IsNull() && !aHaveNewProgress) ||
               !mDataTime.IsNull(),
               "null TimeStamp mDataTime should not be used in comparison");

but could do this all differently with a new bool dispatchProgress and replacing the ?: with an if/else that uses different logic to set dispatchProgress depending on mProgressTime.IsNull(), if you prefer.

Comment 7

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d18303ed2f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6da2e1063f0e

The changes in bug 975782 will test this.

Comment 8

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78bde999fddd

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d18303ed2f7
https://hg.mozilla.org/mozilla-central/rev/6da2e1063f0e

Comment 10

[Ralph Giles (:rillian)]

Attachment: Backport part 1 to 36 beta "move stalled progress timing"

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, with more video playback stalls.
[Describe test coverage new/current, TBPL]: Landed on m-c some time ago.
[Risks and why]: This affects non-MSE playback but has had quite some time to settle on the 37 branch. The main risk is an interaction with other fixes we haven't uplifted.
[String/UUID change made/needed]: None.

This request applies to both patches on this bug, but the first one needed manual rebase onto the beta tree.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/15e3be526862
https://hg.mozilla.org/releases/mozilla-beta/rev/b07f9144d190