779392 - Improve the ALSA PulseAudio bug workaround used to fix bug 761274

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

+++ This bug was initially created as a clone of Bug #761274 +++

See bug 761274 comments 37, 43, 44, 46, 47, 52, 53, 54, 55, 56.

Short summary: the new audio backend (landed in bug 623444 and bug 756944) tries to use a 100ms buffer by default, rather than the 500ms buffer we used in the old backend.  It's also pull based (waiting on the OS stream to be ready for more data) rather than push based (blocking in a write() call to the OS stream).

This revealed an issue in the PulseAudio ALSA plugin where the requested buffer size can't be honoured by PA, so it returns a stream using a larger buffer.  This larger buffer size isn't reflected via the ALSA API (or in the ALSA logic for its internal ring buffer sizing logic).  When attempting to fill ALSA's buffer and wait for more space, this situation results in a string of immediate buffer underruns and recoveries.

This problem will show up as badly stuttering (or inaudible) audio and video playing back too quickly.  You can also observe the Firefox ALSA stream quickly disappearing and reappearing in the PA volume controls as the stream is repeatedly recreated during underrun recovery.

This problem tends to appear with long running PA servers, as the sink's minimum latency increases over time and never decreases (fixed in PA 2.0; the minimum latency is recomputed when resuming sinks in 2.0).  Restarting PA via "pulseaudio -k" provides a way to reset the minimum latency with PA < 2.0.  You can query the PA server's minimum latency state by executing "pacmd list-sinks" and examining the latency range listed under "configured latency" or "fixed latency".  If this value is higher than 100ms, this bug will be triggered.

An ugly workaround was landed in bug 761274 that forces the minimum latency up to 200ms when the PA ALSA plugin is detected.  This bug is to continue working on an alternative workaround that involves disabling the PA ALSA plugin's problematic underrun reporting.

Comment 1

[Matthew Gregan [:kinetik]]

So, in my original attempt (https://github.com/kinetiknz/cubeb/commit/1aa0058d0729eb85505df104cd1ac072432c6d24), searching for the "pcm.default" configuration node fails on some distributions (e.g. Ubuntu 12.04) because the default configuration is only set up after the configuration is evaluated at runtime, because it relies on a test to load the PA ALSA plugin before setting it as the default.

See /usr/share/alsa/alsa.conf.d/pulse.conf on Ubuntu and /usr/share/alsa/alsa.conf.d/*-pulseaudio*.conf on Fedora to compare the different ways the PA ALSA plugin is configured and set as default.

Comment 2

[Matthew Gregan [:kinetik]]

It looks like I need a way to evaluate the hooks in the configuration.  Something like snd_config_search_alias_hooks or snd_config_search_hooks, but neither of those are part of the public API.

But I realized that there's an easier way to get the config space initialized: open a PCM as normal, which causes the config to be loaded and evaluated, then immediately close it.  We can't use that PCM, since we couldn't pass our modified config to it.  Now the config space is initialized, init_local_config_with_workaround can assert that snd_config is already non-NULL and then find the node it needs to configure.

Comment 3

[russianneuromancer]

Hello!
I not able to reproduce Bug #761274 for a long time but today it finally get it reproduced with 16.0~a2~hg20120816r102420 build. 
As you ask me in comment 46 https://bugzilla.mozilla.org/show_bug.cgi?id=761274#c46 I check your build from comment 56 https://bugzilla.mozilla.org/show_bug.cgi?id=761274#c56
Result: doesn't help. Right now I have this bug reproducible in 16.0~a2~hg20120816r102420 build and in your build too.

Comment 4

[russianneuromancer]

Attachment: pacmd list-sinks output

I guess that may be helpful.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to runetmember from comment #4)
> Created attachment 655402 [details]
> pacmd list-sinks output

Thanks, is that from when the problem is occuring?  It doesn't look like there are any streams playing (configured latency is 0ms, suggesting the server's idle).

It may be that the earlier workaround has merely reduced the frequency of the problem.  If your server's latency is growing to over ~200ms, the workaround that's current in place won't be sufficient (but the one planned in this bug should be).

Comment 6

[russianneuromancer]

Attachment: pacmd list-sinks output while playback

> Thanks, is that from when the problem is occuring?  It doesn't look like there are any streams playing (configured latency is 0ms, suggesting the server's idle).
pacmd list-sinks output while playback video in Firefox is attached. 

With Firefox that playback HTML5 video from YouTube:
> configured latency: 1820,44 ms; range is 116,00 .. 1820,44 ms

With VLC that playback movie:
> configured latency: 116,00 ms; range is 116,00 .. 1820,44 ms

Comment 7

[Matthew Gregan [:kinetik]]

(In reply to runetmember from comment #6)
> With Firefox that playback HTML5 video from YouTube:
> > configured latency: 1820,44 ms; range is 116,00 .. 1820,44 ms

Are you sure you don't have the Firefox pref media.cubeb_latency_ms set?  We should be requesting a 200ms latency with the workaround in place, so I'm not sure why the configured latency would end up so high...

Comment 8

[russianneuromancer]

> Are you sure you don't have the Firefox pref media.cubeb_latency_ms set?
Yes. All variables from "media" group is in default state. media.cubeb_latency_ms variable is not defined at all. I especially check this right now in about:config.

Comment 9

[Matthew Gregan [:kinetik]]

I've just pushed my proposed improvement to this workaround to Try: https://tbpl.mozilla.org/?tree=Try&rev=500fa169388a

Can you please test that once the builds appear here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-500fa169388a/

Comment 10

[russianneuromancer]

Last time it take few weeks before issue appear again, so I can't promise to check test build with improved workaround soon.

Comment 12

[Matthew Gregan [:kinetik]]

This patch causes test_a4_tone to fail consistently on try, but not locally.  Not sure why yet.  Logs here: https://tbpl.mozilla.org/php/getParsedLog.php?id=14934218&tree=Try

Also, it'd be nice if this workaround worked even when the PulseAudio PCM wasn't at the top of the chain, e.g.:

% cat ~/.asoundrc
pcm.foo {
  type lfloat
  slave {
    pcm "pulse"
    format S16_LE
  }
}

pcm.!default foo

--8<--

The above asoundrc breaks the workaround, because init_local_config_with_workaround ends up examining "pcm.foo", whose type is "lfloat" rather than "pulse".  init_local_config_with_workaround would need to walk the PCM config chain to handle this.  It may not be worth the effort of dealing with this edge case.

Comment 13

[Matthew Gregan [:kinetik]]

This version of the fix walks the slave PCMs in the config tree (assuming "pulse" is the last PCM), so it handles the case above.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=faad7a66c077

Comment 14

[Matthew Gregan [:kinetik]]

Ah, so test_a4_tone is failing because the audio stream is failing to initialize (which is bug 745748); why it's failing to initialize with this new code, I don't know... debugging now.

Comment 15

[Matthew Gregan [:kinetik]]

That's cause by snd_pcm_open_lconf failing with the the workaround local config because the PulseAudio plugin is too old to support the handle_underrun setting, resulting in snd_pcm_open_lconf failing with -EINVAL.  Let's see if this works: https://tbpl.mozilla.org/?tree=Try&rev=1360b705c9e2

Comment 16

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Disable PA's async underrun handling in the ALSA PulseAudio plugin, when present and new enough to support this workaround.  Older versions of the ALSA PA plugin will fall back to the old workaround.  Removes the ugly PA detection via strcmp, and now relies on detecting a "pulse" PCM in the ALSA config chain.

Comment 17

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/875aceabf236

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/875aceabf236