dispatch "stalled" even when no bytes have been received

RESOLVED FIXED in Firefox 36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 1108787
(Assignee)

Comment 1

4 years ago
Created attachment 8533414 [details] [diff] [review]
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.
Attachment #8533414 - Flags: review?(cpearce)
(Assignee)

Comment 2

4 years ago
Created attachment 8533416 [details] [diff] [review]
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.
Attachment #8533416 - Flags: review?(cpearce)
(Assignee)

Updated

4 years ago
Blocks: 1108846
(Assignee)

Updated

4 years ago
No longer blocks: 1108846
See Also: → bug 1108846
(Assignee)

Updated

4 years ago
Blocks: 975782
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?
Attachment #8533414 - Flags: review?(cpearce) → review+
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...
Attachment #8533416 - Flags: review?(cpearce) → review+
(Assignee)

Comment 5

4 years ago
(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).
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bde999fddd
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/1d18303ed2f7
https://hg.mozilla.org/mozilla-central/rev/6da2e1063f0e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Created attachment 8548719 [details] [diff] [review]
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.
Attachment #8548719 - Flags: approval-mozilla-beta?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8548719 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.