Closed
Bug 882027
Opened 11 years ago
Closed 11 years ago
Rapid oscillations between HAVE_CURRENT_DATA and HAVE_FUTURE_DATA due to threading mismatch
Categories
(Core :: Audio/Video, defect, P1)
Tracking
()
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(2 files)
4.30 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
4.68 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
nsBuiltinDecoderStateMachine::UpdateReadyState gets called frequently from nsBuiltinDecoderStateMachine off the main thread. It calls nsBuiltinDecoderStateMachine::GetNextFrameStatus and based on the results, dispatches one of three nsBuiltinDecoder::NextFrame* runnables to the main thread, which call mElement->UpdateReadyStateForData with the right status.
Meanwhile, on the main thread, nsBuiltinDecoder::UpdateReadyStateForData gets called quite often, which calls nsBuiltinDecoderStateMachine::GetNextFrameStatus and then forwards to mElement->UpdateReadyStateForData.
The problem is, values of nsBuiltinDecoderStateMachine::GetNextFrameStatus are computed off the main thread and then effectively stored in the event queue. Meanwhile, while those events are queued, the status can change. Then, for a while, we can have calls to nsBuiltinDecoder::UpdateReadyStateForData interleaved with processing of NextFrame* runnables, which disagree about the the next-frame-status is, so we end up flip-flopping between two different states.
The fix is easy and simplifies code: nsBuiltinDecoderStateMachine::UpdateReadyState should just fire off a runnable to run nsBuiltinDecoder::UpdateReadyStateForData on the main thread, which will get the latest GetNextFrameStatus when it runs. No more flip-flopping.
Assignee | ||
Comment 1•11 years ago
|
||
I'm not sure how much this actually hurts Youtube, but it definitely doesn't help, because it can trigger the firing of a lot of DOM events unnecessarily.
Blocks: 877024
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #761286 -
Flags: review?(cpearce)
Comment 3•11 years ago
|
||
Comment on attachment 761286 [details] [diff] [review]
fix
Review of attachment 761286 [details] [diff] [review]:
-----------------------------------------------------------------
Remember to patch on m-c too.
Attachment #761286 -
Flags: review?(cpearce) → review+
Comment 4•11 years ago
|
||
From a user perspective, what impact does this bug have on watching media content?
Assignee | ||
Comment 5•11 years ago
|
||
It'll be slowing down the Web site a bit, at least, and may in some cases cause the spinny "waiting" controls UI to appear and disappear unnecessarily. It's hard to say though, because it's hard to do a controlled experiment while there are other bugs.
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 761286 [details] [diff] [review]
fix
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: slightly slower Web sites playing video, possibly unnecessary flipping of video control UI between waiting/not-waiting states
Testing completed: local testing
Risk to taking this patch (and alternatives if risky): pretty low risk. Just makes readyState handling more uniform instead of handling it differently depending on what triggered the change.
String or UUID changes made by this patch: none
The main reason I want this on b2g18 is to simplify testing. I'm trying to fix Youtube bugs on b2g18 and I want to be sure that bugs we see are not due to this (known) issue.
Attachment #761286 -
Flags: approval-mozilla-b2g18?
Comment 7•11 years ago
|
||
Per Alex we're apparently not doing approvals anymore, so I'll do the leo? nom instead to see if we want this on b2g18.
blocking-b2g: --- → leo?
Updated•11 years ago
|
blocking-b2g: leo? → leo+
Updated•11 years ago
|
Attachment #761286 -
Flags: approval-mozilla-b2g18?
Assignee | ||
Comment 8•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/775c4219d32a
The patch needs to be rebased on top of 822952 on trunk, which is nontrivial.
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #763308 -
Flags: review?(cpearce)
Updated•11 years ago
|
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
Updated•11 years ago
|
Attachment #763308 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
Push backed out for reftest failures:
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff486a6eb5c9
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad2d8b5e7c9
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f8048b030fe6
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f2d55d17a0d8
Comment 14•11 years ago
|
||
Updated•11 years ago
|
Priority: -- → P1
Target Milestone: --- → 1.1 QE3 (24jun)
Assignee | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Flags: in-moztrap-
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•