Closed Bug 882027 Opened 7 years ago Closed 7 years ago

Rapid oscillations between HAVE_CURRENT_DATA and HAVE_FUTURE_DATA due to threading mismatch


(Core :: Audio/Video, defect, P1)

Gonk (Firefox OS)



1.1 QE3 (26jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed


(Reporter: roc, Assigned: roc)




(2 files)

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.
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
Attached patch fixSplinter Review
Attachment #761286 - Flags: review?(cpearce)
Comment on attachment 761286 [details] [diff] [review]

Review of attachment 761286 [details] [diff] [review]:

Remember to patch on m-c too.
Attachment #761286 - Flags: review?(cpearce) → review+
From a user perspective, what impact does this bug have on watching media content?
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.
Comment on attachment 761286 [details] [diff] [review]

NOTE: Please see 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?
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?
blocking-b2g: leo? → leo+
Attachment #761286 - Flags: approval-mozilla-b2g18?
Backed out:
The patch needs to be rebased on top of 822952 on trunk, which is nontrivial.
Attachment #763308 - Flags: review?(cpearce) → review+
Priority: -- → P1
Target Milestone: --- → 1.1 QE3 (24jun)
Closed: 7 years ago
Resolution: --- → FIXED
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.