Closed
Bug 1074420
Opened 10 years ago
Closed 10 years ago
Repeated small Audio DataCallbacks to MSG will cause data to be lost and unneeded processing
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox32 | --- | unaffected |
firefox33 | --- | unaffected |
firefox34 | + | fixed |
firefox35 | --- | fixed |
b2g-v2.1 | --- | fixed |
b2g-v2.2 | --- | fixed |
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Keywords: dataloss, Whiteboard: [webrtc-uplift])
Attachments
(1 file, 1 obsolete file)
7.68 KB,
patch
|
roc
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
When you get an Audio driver DataCallback, and the left-over audio from rounding up to a chunk size (typically 128 frames) is stored in the mScratchBuffer, and the *next* DataCallback is smaller than the amount of data stored in the scratchbuffer, we (a) run an iteration even though we don't need to, (b) throw away any buffered data beyond what was requested, (c), throw a warning (Available() is 0 in the mixer callback) (d) maybe more bad things We should handle this case much more cleanly. This *might* be the cause of the false-OOM crasher (bug 1033066).
Assignee | ||
Comment 1•10 years ago
|
||
Most of the diff here is because I indented a block
Assignee | ||
Updated•10 years ago
|
Attachment #8497042 -
Flags: review?(roc)
Comment on attachment 8497042 [details] [diff] [review] handle repeated short audio DataCallback()s Review of attachment 8497042 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioBufferUtils.h @@ +122,5 @@ > > mPosition -= FramesToSamples(CHANNELS, framesToWrite); > + // If we didn't empty the spill buffer for some reason, shift the remaining data down > + if (mPosition > 0) { > + PodMove(mBuffer + FramesToSamples(CHANNELS, framesToWrite), mBuffer, are the src and dst in the correct order here? They look wrong to me
Attachment #8497042 -
Flags: review?(roc) → review-
Assignee | ||
Comment 3•10 years ago
|
||
Good catch PodMove(T* aDst, const T* aSrc, size_t aNElem) Careful listening would have shown it; the tones used were low frequency enough (and the "before" case was so badly distorted) I hadn't noticed the small (single-digit-ms-level) repetitions.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8497042 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8497405 -
Flags: review?(roc)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8497405 [details] [diff] [review] handle repeated short audio DataCallback()s Review of attachment 8497405 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/GraphDriver.cpp @@ +855,5 @@ > + mIterationEnd, > + mStateComputedTime, > + mNextStateComputedTime); > + } else { > + NS_WARNING("DataCallback buffer filled entirely from scratch buffer, skipping iteration."); We can probably downgrade this from a WARNING now (not sure logging defs are available in this file currently); I'd left it as one because the original "if !Available()" was a warning (and to avoid the "make logging available in a .h file"). It's rare (on linux at least) unless you overload the system, and then it's not too fast (with many thousands of streams I saw a few a second - though I can tell Windows was much more likely to produce it)
Attachment #8497405 -
Flags: review?(roc) → review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6c0c0e03b8
status-firefox33:
--- → unaffected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
Whiteboard: [webrtc-uplift]
https://hg.mozilla.org/mozilla-central/rev/2e6c0c0e03b8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8497405 [details] [diff] [review] handle repeated short audio DataCallback()s Approval Request Comment [Feature/regressing bug #]: bug 848954 [User impact if declined]: Annoying audio glitches after ending MediaStreams (webrtc/loop calls, getusermedia, webaudio) for a while. [Describe test coverage new/current, TBPL]: Manual test - requires listening. Generally very apparent. (10s-1m of slightly to very distorted audio after ending) [Risks and why]: very low risk - clearing a buffer [String/UUID change made/needed]: none
Attachment #8497405 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Comment 9•10 years ago
|
||
Comment on attachment 8497405 [details] [diff] [review] handle repeated short audio DataCallback()s Aurora+
Attachment #8497405 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•