Closed
Bug 793274
Opened 12 years ago
Closed 12 years ago
Underrun at the very beginning of the playback when using cubeb
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: padenot, Assigned: padenot)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
STR:
- Open http://gr.ema.lv/andris/_soundTest_/audio/1.ogg (a short sine)
- Click play on the built-in controls
Expected:
- The sound plays fine, from the beginning to the end.
Actual result:
- There is almost always an underrun a the beginning (~25% of the time, it plays fine).
- If I disable cubeb, it works fine.
Also, I put a printf in the data callback (|printf("underrun: %d\n", bytesWanted / mBytesPerFrame);| at [1]), and I see that it is indeed an underrun.
It works fine with a file with a greater duration.
[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/nsAudioStream.cpp#1223
Assignee | ||
Comment 1•12 years ago
|
||
Matthew, the problem here is that we are starting the cubeb thread too early, when not enough frames have been written to the nsBufferedAudioStream circular buffer.
This broken patch fixes the behavior reported in comment 0, but the fix will not work for super tiny files (that have a duration lesser than |INITIAL_BUFFER_FILL|). This patch is mainly to show the problem, really.
We could really fix this by adding an method to the nsAudioStream, to make the audio loop explicitly start the cubeb stream when a certain amount of frames have been written to the stream, or if the queue is finished. Or maybe some other thing.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Flags: needinfo?(kinetik)
Comment 2•12 years ago
|
||
Good catch. libcubeb tries to prefill buffers, but with the current design this happens at stream initialization time and since the BufferedAudioStream is always empty at this point, only serves to prefill with silence.
I think we should disable the prefill on stream init, and add a method to prefill at the appropriate time, and have that signal when it's safe to start.
Flags: needinfo?(kinetik)
Assignee | ||
Comment 3•12 years ago
|
||
Do you fill like doing it, or should I write a patch?
Comment 4•12 years ago
|
||
I'm happy to take care of it, but I won't get to it for a little while. Maybe the approach suggested in your last paragraph is the way to go.
Assignee | ||
Comment 5•12 years ago
|
||
I'll give the idea I proposed in my last paragraph a shot.
Assignee | ||
Comment 6•12 years ago
|
||
This implements comment 2 idea, and fixes the problem. I've chosen the prefill
value empirically, a lower value shows a tiny tiny underrun on my machine, and a
higher value induces latency, so I guess this is more or less OK.
Seems green on try: https://tbpl.mozilla.org/?tree=Try&rev=fa140696c827
Attachment #685132 -
Flags: review?(kinetik)
Assignee | ||
Comment 7•12 years ago
|
||
Ha, it seems to cause problem on Mac, but I don't have a Mac handy and can't reproduce on other platforms. I've ordered one but maybe the problem is obvious to you, since it's probably a subtle difference in cubeb between windows/linux and mac.
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 685132 [details] [diff] [review]
Make sure to have enough frames pushed to the AudioStream before starting it. r=
Alright, does not work too well on Windows as well. I'm cancelling the review for now.
Attachment #685132 -
Flags: review?(kinetik)
Assignee | ||
Comment 9•12 years ago
|
||
This is now green on try: https://tbpl.mozilla.org/?tree=Try&rev=4956e51b7340
Attachment #696035 -
Flags: review?(kinetik)
Assignee | ||
Updated•12 years ago
|
Attachment #685132 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #681236 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #696035 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Backed out on suspicion of causing permaorange win pgo-only mochitest-1 timeouts:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18841611&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18840712&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18846633&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18847209&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18850103&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18850099&tree=Mozilla-Inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/c970a1c329bb
Assignee | ||
Comment 13•12 years ago
|
||
After bisecting the queue on try, this patch has been proved to cause no problem [0], note the patch at the top of the queue that requests a PGO build. The problem remains in the other part of the queue [1].
This has been pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/4317e42ac807
[0]: https://tbpl.mozilla.org/?tree=Try&rev=516ddf4c963c
[1]: https://tbpl.mozilla.org/?tree=Try&rev=418c581b8023
Comment 14•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•