Closed
Bug 1096597
Opened 10 years ago
Closed 10 years ago
Race condition when playback resumes after adding missing data
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file, 1 obsolete file)
2.05 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
This was causing the tests in bug 1063323 to time out intermittently.
See bug 1091008 comment 32, which I decided to split out into a separate bug. Patch coming up.
Assignee | ||
Comment 1•10 years ago
|
||
from bug 1091008 comment 32:
> So the MSE stuff is racey here. I've compared TBPL NSPR logs between a good
> and bad run, and here's what's happening:
>
> (1) We append data to the source buffer. It gets piped down into the track
> buffer, which schedules the state machine thread. It also queues up decoder
> initialization to happen asynchronously on the decode thread.
> (2A) The state machine thread wakes up, and requests data from the reader.
> (2B) The decoder gets initialized and added to mInitializedDecoders.
> (3) The reader tries to process the request for video data. If (2A) happens
> faster than (2B), it doesn't find anything in mInitializedDecoders with the
> appropriate range, and fires OnNotDecoded(WAITING_FOR_DATA), which causes
> the state machine to hang.
>
> One easy fix here is to put the state machine into buffering mode when we
> receiving OnNotDecoded(WAITING_FOR_DATA) - this will ensure that it tries
> again, and generally feels like the correct thing to do.
>
> We could also have the reader do additional checks to see if there are any
> not-yet-initialized decoders, and wait to respond to the state machine until
> that's sorted out. That generally feels a lot less robust, though.
>
> I'll attach a patch.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8520209 -
Flags: review?(cpearce)
Assignee | ||
Comment 3•10 years ago
|
||
Comment 4•10 years ago
|
||
Comment on attachment 8520209 [details] [diff] [review]
Switch to buffering mode on WAITING_FOR_DATA. v1
Review of attachment 8520209 [details] [diff] [review]:
-----------------------------------------------------------------
Does it make sense to also have a callback on the RequestSampleCallback interface to say "I'm not longer waiting for data", to move us out of the BUFFERING state? Then rather than polling once per second in the BUFFERING case handler in MediaDecoderStateMachine to determine whether we should move out of buffering state, we could move out of buffering state as soon we're able to. This would improve the latency. If you think this makes sense, we should do it in another patch.
Attachment #8520209 -
Flags: review?(cpearce) → review+
Comment hidden (obsolete) |
Comment 6•10 years ago
|
||
Ideally update and updateend events and changes to buffered should not happen
until after ReadMetadata has run to initialize the new decoder.
Ignore comment 5.
Assignee | ||
Comment 7•10 years ago
|
||
Still tinkering with this:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=98c5bde79d39 (orange, made some fixes)
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6287c12a8999
(In reply to Karl Tomlinson (:karlt) from comment #6)
> Ideally update and updateend events and changes to buffered should not happen
> until after ReadMetadata has run to initialize the new decoder.
Yeah - do you think we should get a bug on file for that?
Comment 8•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #7)
> (In reply to Karl Tomlinson (:karlt) from comment #6)
> > Ideally update and updateend events and changes to buffered should not happen
> > until after ReadMetadata has run to initialize the new decoder.
>
> Yeah - do you think we should get a bug on file for that?
I don't know what priority we should give it, but I filed bug 1097252.
Assignee | ||
Comment 9•10 years ago
|
||
This wasn't quite right - we only want to switch to buffering mode when we're
out of samples.
Attachment #8520209 -
Attachment is obsolete: true
Attachment #8521019 -
Flags: review?(cpearce)
Updated•10 years ago
|
Attachment #8521019 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•