751030 - Import PulseAudio cubeb backend

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

This patch imports the PulseAudio cubeb backend.  It's not wired into the build yet, since we don't have the necessary devel packages on the build machines (bug 662417), plus it's probably necessary to support dynamically selecting the ALSA or PulseAudio backend depending on what's available on the host system.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

There's a known deadlock in this code (found by mwu) caused by calling data_callback from PA's write_callback, which runs with the PA mainloop lock held.  This thread then has a lock ordering of mainloop->nsBufferedAudioStream::DataCallback (monitor).  When another thread calls a cubeb function that takes the PA mainloop lock, the lock ordering for that caller is nsBufferedAudioStream::DataCallback (monitor)->mainloop.  This manifests mostly on systems where data_callback runs slowly, but can obviously occur on any system.

It's probably unsafe (and may not be possible) to force PA to drop the mainloop lock temporarily for the duration of data_callback, so the likely fix will require queuing the write_callback requests and running them outside of PA's callback.

Comment 2

[Michael Wu [:mwu]]

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

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

Mostly makes sense to me. Reading the code hasn't revealed any new issues over what I've discovered while running it.

::: media/libcubeb/src/cubeb_pulse.c
@@ +69,5 @@
> +{
> +  /* XXX handle PA_STREAM_FAILED during creation */
> +  if (pa_stream_get_state(s) == PA_STREAM_TERMINATED) {
> +  } else {
> +    assert(PA_STREAM_IS_GOOD(pa_stream_get_state(s)));

IIRC I had issues with one of the state callbacks taking down gecko after a pulseaudio crash. We should figure out how to handle this better at some point.

@@ +190,5 @@
> +  cubeb * ctx;
> +
> +  *context = NULL;
> +
> +  ctx = calloc(1, sizeof(*ctx));

Should probably assert(ctx) for consistency with cubeb_stream_init.

@@ +281,5 @@
> +  stm = calloc(1, sizeof(*stm));
> +  assert(stm);
> +
> +  stm->context = context;
> +  assert(stm->context);

I think it would make more sense to assert(context) near the top of the function. stm->context = context; isn't what's going to fail.

@@ +290,5 @@
> +
> +  stm->sample_spec = ss;
> +
> +  battr.maxlength = -1;
> +  battr.tlength = pa_usec_to_bytes(latency * 1000, &stm->sample_spec);

PA_USEC_PER_MSEC

@@ +292,5 @@
> +
> +  battr.maxlength = -1;
> +  battr.tlength = pa_usec_to_bytes(latency * 1000, &stm->sample_spec);
> +  battr.prebuf = -1;
> +  battr.minreq = battr.tlength / 2;

Any particular reason for setting this instead of letting PA set it?

@@ +370,5 @@
> +  if (r != 0) {
> +    return CUBEB_ERROR;
> +  }
> +
> +  /* XXX might be more accurate to compute directly from get_timing_info */

It seems like pa_stream_get_time compensates for latency whereas data taken directly from get_timing_info doesn't.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Comment 4

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d221d99abef1

Comment 5

[Arun Raghavan]

Comment on attachment 621483 [details] [diff] [review]
patch v1

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

::: media/libcubeb/src/cubeb_pulse.c
@@ +35,5 @@
> +static void
> +context_state_callback(pa_context * c, void * m)
> +{
> +  if (pa_context_get_state(c) != PA_CONTEXT_TERMINATED) {
> +    assert(PA_CONTEXT_IS_GOOD(pa_context_get_state(c)));

As Michael pointed out, this will lead to a crash if the PA server crashes or a user forcibly disconnects you. You can avoid this by checking the context state at the end of your pa_threaded_mainloop_wait()s.

The GStreamer pulsesink element is good example of how to deal with this -- look for gst_pulsering_is_dead() and its callers in http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/ext/pulse/pulsesink.c

@@ +114,5 @@
> +    r = pa_stream_write(s, buffer, got * frame_size, NULL, 0, PA_SEEK_RELATIVE);
> +    assert(r == 0);
> +
> +    if ((size_t) got < size / frame_size) {
> +      size_t buffer_fill = pa_stream_get_buffer_attr(s)->maxlength - pa_stream_writable_size(s);

The maxlength might be a _lot_ longer than the amount of data you've actually filled. You'd be better off using (latency + got + some safety margin) instead.

@@ +115,5 @@
> +    assert(r == 0);
> +
> +    if ((size_t) got < size / frame_size) {
> +      size_t buffer_fill = pa_stream_get_buffer_attr(s)->maxlength - pa_stream_writable_size(s);
> +      double buffer_time = (double) buffer_fill / stm->sample_spec.rate;

There's a channel count missing in this division. Just use pa_bytes_to_usec() instead.

@@ +294,5 @@
> +
> +  battr.maxlength = -1;
> +  battr.tlength = pa_usec_to_bytes(latency * PA_USEC_PER_MSEC, &stm->sample_spec);
> +  battr.prebuf = -1;
> +  battr.minreq = battr.tlength / 2;

Your best bet is to do a (tlength / 4) here to deal smoothtly with very high or very low latencies. This is ugly voodoo, I realise, and I'm hoping to deal with this at some point in the future so that PA would be a lot more intelligent about picking a good minreq, but it's sort of the best we have at the moment.

Comment 6

[Matthew Gregan [:kinetik]]

Oops, there were a bunch of comments I wrote to go with the revised patch I checked in, but it looks like I forgot to submit the comment to the bug.  I only noticed it was missing when reading Arun's comment.  And thanks for the feedback Arun, I appreciate it.

Second attempt at comments:

(In reply to Michael Wu [:mwu] from comment #2)
> IIRC I had issues with one of the state callbacks taking down gecko after a
> pulseaudio crash. We should figure out how to handle this better at some
> point.

Yeah, there are a bunch of stream state/API failure situations that are currently caught with asserts that should be converted to use the CUBEB_STREAM_ERROR callback state I introduced to the other backends more recently.  We can do that in a follow up bug.

> > +  battr.maxlength = -1;
> > +  battr.tlength = pa_usec_to_bytes(latency * 1000, &stm->sample_spec);
> > +  battr.prebuf = -1;
> > +  battr.minreq = battr.tlength / 2;
> 
> Any particular reason for setting this instead of letting PA set it?

From testing it seemed that setting minreq was required to get close to the latency I was requesting via tlength.  I'm not sure how ADJUST_LATENCY interacts with this, either.  Anyway, let's change it to what Arun recommended in the followup and at a comment to that code block to that effect.

> It seems like pa_stream_get_time compensates for latency whereas data taken
> directly from get_timing_info doesn't.

I removed the comment to avoid confusion.

Everything else was fixed per Michael's comments.

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d221d99abef1