757034 - import OpenBSD's sndio cubeb backend

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Landry Breuil (:gaston)]

Placeholder bug for OpenBSD's sndio backend for cubeb. Patch in a few when i've done some real testing.

Comment 1

[Landry Breuil (:gaston)]

Attachment: patch v0

v0, doesnt seem to work yet.

Comment 2

[Matthew Gregan [:kinetik]]

A few notes:

cubeb upstream is at https://github.com/kinetiknz/cubeb.  It looks like this code is under the same license (ISC-style), so that's good.  It'd be slightly nicer to use the short header the other files use (with the copyright changed to Alexandre rather than Mozilla, of course), but it'd be polite to check with him before making that change.  Same with the coding style--the rest of the files use two space indents, etc.  This is all minor, though.

The general design for cubeb is to make each stream as lightweight as possible, so I've tried to avoid using a thread per stream.  The current code will work for now, but longer term we'll want to move this to something where single (or pair) of worker threads handle all of the active streams.  Since sio gives hands out waitable fd, so this'll be easy enough to deal with in the future--the code will probably be similar (but less complex, since sio is a *much* nicer API) to the ALSA code.

stream_init needs support for CUBEB_SAMPLE_FLOAT32LE as that's what most of the media code will output.  Looks like sio_setpar will accept 32-bit samples, but I can't see how to specify that they're floats rather than integers.  CUBEB_SAMPLE_U8 is gone, so that chunk can be removed.

stream_destroy shouldn't need to call stream_stop; the API user is expected to do that.

The mainloop shouldn't call the state callback with DRAINED every time it stops, otherwise every call to stream_stop results in a drain callback.  Drain should only be signaled when data_cb has returned >= 0 && < nfr.  Ideally there should also be a sufficient wait involved that all submitted audio has completed playback, since the stream is usually destroyed immediately after drain fires.

Calling data_cb with the mutex held may result in a deadlock, as the callback implementation also holds a lock, and that lock ordering is reversed when stream_get_position is called from another thread.

I ended up removing stream_set_volume from the API for now, since it wasn't available on all backends and it was quicker and easier to do software volume in the nsBufferedAudioStream code that uses cubeb.  It looks like sio_setvol does exactly what we want (per-stream volume control), so when bug 753600 is fixed this code can be used.

Comment 3

[Matthew Gregan [:kinetik]]

Landy, Alexandre, and I discussed this a bit over email too.  I've imported the updated version into github here: https://github.com/kinetiknz/cubeb/blob/master/src/cubeb_sndio.c

Comment 4

[Landry Breuil (:gaston)]

Attachment: patch v1

Corresponding version of the backend as imported on github, works in really basic testing with a bunch of <audio> testcases.

Comment 5

[Matthew Gregan [:kinetik]]

+  case CUBEB_SAMPLE_S16LE:
+    wpar.le = 1;
+    break;
+  case CUBEB_SAMPLE_S16BE:
+    wpar.le = 1;
+    break;

The second one should be 0, right?

I think the patches going back and forth in email had a fix for the latency/buffer size calculation, but I'm not sure I quite understand how sndio wants this to work.  It looks like this code is submiting a single buffer at a time that's the same size as sndio has been requested to buffer internally, so it seems like it is likely to xrun regularly because there's no refill until the buffer drains entirely... With the other backends, I've split the buffering up so that for a latency of n there are n/4 (I chose 4 arbitrarily, but you probably want at least 3) buffer refill cycles.

Comment 6

[Landry Breuil (:gaston)]

Attachment: patch v2

New version with lantecy fixed. Asking for review so that we get a chance to snuck it in for fx 15 before the next aurora uplift. What are the plans re enabling cubeb by default ?

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 629159 [details] [diff] [review]
patch v2

I haven't reviewed this thoroughly since last time I went over it, but I'm fine with landing it.

Per my earlier comment, please fix this before landing:

+  case CUBEB_SAMPLE_S16LE:
+    wpar.le = 1;
+    break;
+  case CUBEB_SAMPLE_S16BE:
+    wpar.le = 1;
+    break;

(In reply to Landry Breuil (:gaston) from comment #6)
> What are the plans re enabling cubeb by default ?

It's already enabled by default.  There's a chance that it'll be disabled for the next Aurora if I don't get bug 759677 fixed.

Comment 8

[Landry Breuil (:gaston)]

Attachment: patch v3

Right, forgot about it. New patch fixes it, let's carry the r+.

Comment 9

[Matthew Gregan [:kinetik]]

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

Comment 10

[Matthew Gregan [:kinetik]]

And I added cubeb_sndio.c to cubeb's update script here: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ef00347029

And pushed this version of cubeb_sndio.c to Github here: https://github.com/kinetiknz/cubeb/commit/21d9678eb9755761f7a9d26253189b926258de7c

So from now on, any updates made to the Github version will be imported each time the in-tree cubeb is updated.

Comment 11

[Matthew Gregan [:kinetik]]

And apparently Bugzilla clears the milestone without giving you a clobber warning on form submit. :-/

Comment 12

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b646d0607a32
https://hg.mozilla.org/mozilla-central/rev/d7ef00347029