747793 - Audio crackles since 20120417 Nightly

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[ferongr]

Attachment: DxDiag report

All audio originating from the browser (audio and video elements) is crackling, the same way it does during buffer underruns. 

STR:
Go to any page using an HTML audio or video element
Play the content

last good: 20120416 first bad: 20120417

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd06332733e5&tochange=c61e7c3a232a

Comment 1

[Matthew Gregan [:kinetik]]

Thanks for the report.  Just to confirm, this happens for *any* media element playing audio, and it happens all the time?  Does it happen with a simple audio-only WebM file such as http://flim.org/~kinetik/sync_a.webm?

This is certainly bug 742154, which enabled the new audio code on Windows.  If you'd like to verify, go to about:config and create a new boolean pref named "media.use_cubeb" and set it to false, then reload the page containing the audio/video element.  If could you do a before/after test of that pref with the file linked above and let me know how much CPU Firefox is using according to Task Manager with and without disabling the new code via the media.use_cubeb pref.

Comment 2

[Matthew Gregan [:kinetik]]

I can reproduce this somewhat in my XP VM if I set it to 1 CPU, force the host CPU to run at the lowest clock rate (~1.2GHz), and move the mouse in and out of the playing audio element to force the controls to repaint.  This experiment also induces underruns with the old code enabled, but they're much less severe.

It's not too surprising that it's easier to induce underruns with the new code, since it buffers 100ms of audio rather than 1000ms, but I think the new behaviour can be improved without drastically increasing the buffer size.

Comment 3

[ferongr]

(In reply to Matthew Gregan [:kinetik] from comment #1)
> Thanks for the report.  Just to confirm, this happens for *any* media
> element playing audio, and it happens all the time?  Does it happen with a
> simple audio-only WebM file such as http://flim.org/~kinetik/sync_a.webm?

The linked file does not crackle with the tab in the foreground and the mouse immobile but starts doing it the moment I do something else like bringing up the Start menu. A 320x240 Webm video file on Youtube is a lot worse, despite the fact that CPU usage hovers around 30%.

Comment 4

[ferongr]

And for some reason it varies, since right now (after I made the above comment) even the file you linked crackles.

Comment 5

[Matthew Gregan [:kinetik]]

In that situation, is it only crackling while you're continuing to browse the Start menu, or does it continue crackling continuously after that point, even if you let the machine go idle?

I pushed a couple of minor tweaks to try, they'll turn up here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4103d66accc4/ and http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c1dc886dfe76/ in a couple of hours.

Comment 6

[ferongr]

The crackling stops after the machine goes idle. Having another program (in this case, my music player) sending null audio data at the same time (playback paused) makes the crackling constant most of the time. I won't be here when the builds finish, but later in the day to confirm the above.

Comment 7

[ferongr]

I tested both builds (4103d66accc4 has just the thread priority bump, c1dc886dfe76 also raises buffer to 150ms). Both are non-ideal. While with one Youtube tab both play audio correctly as soon as a background tab starts doing something (e.g. Gmail doing a request to check for mail, tab loading in the background) then the problem shows up again.

The audio sample you linked me to clips at every beep and is counter-productive to finding such issues. I checked with a 440Hz sine tone at 90% amplitude [1] that makes any audio disruptions or clipping easily audible.

[1] http://ompldr.org/vZGhjYg/440Hz.ogg

Comment 8

[Matthew Gregan [:kinetik]]

Clearing the tracking-firefox14 flag because this code will be disabled on Aurora, see bug 746456.

Comment 9

[Matthew Gregan [:kinetik]]

Thanks for the feedback.  I've pushed another set of changes: https://tbpl.mozilla.org/?tree=Try&rev=30217ba3a20c which will show up at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-30217ba3a20c/ once the builds complete.

This is the thread priority bump, plus reworking the buffering by using a ring buffer (avoiding a bunch of memmove traffic (and malloc/free traffic in the worst case)) and the sample conversion (by using a fast path for the case where the volume is unity, and by using the more efficient existing code from nsNativeAudioStream::Write when adjusting sample volume while copying out).

This also adds a hidden pref, "media.cubeb_latency_ms" to allow tweaking the stream latency.  If the above changes aren't sufficient, please create an integer pref named "media.cubeb_latency_ms" and experiment with values between 100 and 1000 and let me know what works.  For reference, the old code (before bug 623444) landed used 1000ms.

It's possible there are other issues or bugs remaining in this new code, but until I can test this on a slow enough machine to reproduce the problem I'm making educated guesses.  I've got the hardware, it's just a matter of finding time to set it up and debug with it.

Comment 10

[Matthew Gregan [:kinetik]]

And I forgot to mention: there will be additional changes in bug 750596 that'll remove the ns*AudioStream layer and the additional data copies it introduces, which may improve the situation further.  I'm reluctant to wind the default stream latency back up too high until that work is done, but it's possible we can turn the latency up a bit and then experiment with winding it down again after that has landed.

Comment 11

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #9)
> I've pushed another set of changes:
> https://tbpl.mozilla.org/?tree=Try&rev=30217ba3a20c which will show up at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mgregan@mozilla.
> com-30217ba3a20c/ once the builds complete.

That had a bug.  Retry at https://tbpl.mozilla.org/?tree=Try&rev=d9c302d2bc68 and http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d9c302d2bc68/

Comment 12

[ferongr]

d9c302d2bc68 works perfectly. It's even better than before.

I tried watching a Youtube webm video while reloading all my other tabs in the background, playing Z-Type (uses <audio> for very short, repeated sound effects) and also doing the above with the test tone in the background. It didn't crackle once.

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: patch v0

This is the same patch I pushed to try server, except:
- fixed reversed use of min/max in the latency pref code that would cause the latency to be 1000ms any time the pref was set.
- lightly documented the circular buffer class, and made a few minor style changes.

I've left the hidden latency pref in for now.  I'm not opposed to removing it if someone disagrees with it, but it may be useful for testing to have it available until the current/ongoing cubeb and other audio changes settle down.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 620175 [details] [diff] [review]
patch v0

Review of attachment 620175 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsAudioStream.cpp
@@ +824,5 @@
> +{
> +public:
> +  nsCircularByteBuffer()
> +    : mBuffer(nsnull), mCapacity(0), mStart(0), mCount(0)
> +  {}

Use MOZ_COUNT_CTOR() and MOZ_COUNT_DTOR() to verify there's no leaks here please.

@@ +828,5 @@
> +  {}
> +
> +  ~nsCircularByteBuffer()
> +  {
> +    delete [] mBuffer;

It seems like you should be using nsAutoArrayPtr instead of manually managing memory here.

@@ +866,5 @@
> +    mCount += aLength;
> +  }
> +
> +  // Remove aSize bytes from the buffer.  Caller must check returned size in
> +  // aSize[12] before using pointer returned in aData[12].  Caller must

I'd say *aSize{1,2} rather than aSize[12], so that it's clearer that you don't mean *(aSize+12).

@@ +874,5 @@
> +    NS_ABORT_IF_FALSE(mBuffer && mCapacity, "Buffer not initialized.");
> +    NS_ABORT_IF_FALSE(aSize <= Length(), "Request too large.");
> +
> +    *aData1 = &mBuffer[mStart];
> +    *aSize1 = (mStart + mCount) - mStart;

Why not *aSize1 = mCount?

Or how about *aSize1 = NS_MIN((mCapacity - mStart), aSize)? Then you don't need the two following if statements?

@@ +1236,5 @@
> +      } else {
> +        // Adjust volume as each sample is copied out.
> +        switch (mFormat) {
> +        case FORMAT_S16_LE: {
> +          short volume = short(1 << 16) * scaled_volume;

short(1<<16) is 0, so I don't think this can possibly work?

The original nsNativeAudioStream::Write code uses PRInt32((1<<16) * scaled_volume, I think you should stick to that.

We appear to be getting away with using generic int/short sized variables here in nsNativeAudioStream::Write, but I think we should use explicitly sized PRInt32/PRInt16 types here to be safe.

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: patch v1

(In reply to Chris Pearce (:cpearce) from comment #14)
> Use MOZ_COUNT_CTOR() and MOZ_COUNT_DTOR() to verify there's no leaks here
> please.

It's never allocated on the heap, so it can't possibly leak directly.

> It seems like you should be using nsAutoArrayPtr instead of manually
> managing memory here.

Done.

> I'd say *aSize{1,2} rather than aSize[12], so that it's clearer that you
> don't mean *(aSize+12).

Changed and clarified the rest of the comment slightly.

> @@ +874,5 @@
> Or how about *aSize1 = NS_MIN((mCapacity - mStart), aSize)? Then you don't
> need the two following if statements?

Done.

> short(1<<16) is 0, so I don't think this can possibly work?

Transcription error, fixed.

> We appear to be getting away with using generic int/short sized variables
> here in nsNativeAudioStream::Write, but I think we should use explicitly
> sized PRInt32/PRInt16 types here to be safe.

Use of shorts is deliberate as that's the type the API being interfaced with uses.  I've changed the ints to PRInts as that argument doesn't apply to them.

Comment 16

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/823bdc42155c

Comment 17

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/823bdc42155c