Closed
Bug 1108838
Opened 10 years ago
Closed 10 years ago
dispatch "stalled" even when no bytes have been received
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: karlt, Assigned: karlt)
References
Details
Attachments
(3 files)
22.72 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
7.91 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
16.49 KB,
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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•10 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•10 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 7•10 years ago
|
||
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.
Flags: in-testsuite?
Assignee | ||
Comment 8•10 years ago
|
||
Flags: in-testsuite? → in-testsuite+
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1d18303ed2f7
https://hg.mozilla.org/mozilla-central/rev/6da2e1063f0e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 10•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•10 years ago
|
Attachment #8548719 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 11•10 years ago
|
||
Updated•10 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•