Closed Bug 1074420 Opened 5 years ago Closed 5 years ago

Repeated small Audio DataCallbacks to MSG will cause data to be lost and unneeded processing


(Core :: Audio/Video, defect)

Other Branch
Not set



Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed


(Reporter: jesup, Assigned: jesup)



(Keywords: dataloss, Whiteboard: [webrtc-uplift])


(1 file, 1 obsolete file)

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).
Most of the diff here is because I indented a block
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-
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.
Attachment #8497042 - Attachment is obsolete: true
Attachment #8497405 - Flags: review?(roc)
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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?
Comment on attachment 8497405 [details] [diff] [review]
handle repeated short audio DataCallback()s

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.