1221580 - Write a full-duplex sndio cubeb backend

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P5)

Comment 1

[Jan Beich]

If bug 1385258 drops media.navigator.audio.full_duplex=false support getUserMedia may stop working on OpenBSD.

Comment 2

[Paul Adenot (:padenot)]

There has been a lot of thinking done over the last few days. We're going to take a completely different approach, and I've closed the bug you mention explaining the rationale.

We need to keep the code working for media.navigator.audio.full_duplex=false, because that's what we're using by default on android.

We'll open various bugs about the new effort we're planning, I'll CC you on the relevant ones.

Comment 3

[Landry Breuil (:gaston)]

Work is being done by alex in https://github.com/kinetiknz/cubeb/compare/master...ratchov:sndio_rec - and its to the point where it seems to work - there will be a PR on github probably for integration before it goes to mozilla-central ?

For proper integration i guess we'll have to patch https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#552 so that when we're on OpenBSD media.navigator.audio.full_duplex defaults to true, but that goes against the goal of making sndio backend available on other platforms.. (ie freebsd/linux)

I think there were conflicts on linux where pulseaudio was taking priority over sndio even if media.cubeb.backend was set to sndio - is the latter pref supposed to work (ie in https://dxr.mozilla.org/mozilla-central/source/dom/media/CubebUtils.cpp#244) ? i havent tested alex's work yet, its in the pipe :)

Alex also has a patch to allow building sndio backend on linux for bug #1351378, which im going to polish for inclusion.

Comment 4

[Landry Breuil (:gaston)]

https://github.com/kinetiknz/cubeb/pull/376 has the full-duplex implem.

As for the pref, is it better to add a section #elif defined(MOZ_SNDIO) or add it to  #elif defined(XP_LINUX) in modules/libpref/init/all.js ?

ie #elif defined(XP_LINUX) || defined(MOZ_SNDIO) (if that's a valid preprocessor macro)

Comment 5

[Landry Breuil (:gaston)]

paul, i suppose your review queue is quite busy, but do you think this could be reviewed so that it lands in 58, including the github merge and then in m-c ?

Comment 6

[Paul Adenot (:padenot)]

(In reply to Landry Breuil (:gaston) from comment #5)
> paul, i suppose your review queue is quite busy, but do you think this could
> be reviewed so that it lands in 58, including the github merge and then in
> m-c ?

Yes. The review for the actual audio code will happen on github.

Comment 7

[Landry Breuil (:gaston)]

I see that you merged the PR in https://github.com/kinetiknz/cubeb/commit/7d00ea6705cf94b46a20b7f3ea6ff6ad8dbeecde (thanks!), should we use this bug to handle the merging to m-c, and the libpref commit switching the full-duplex pref for SNDIO ?

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1221580 - Update libcubeb to revision 7d00ea6705.

This includes a changes so that the sndio backend supports duplex audio, and
documentations fix on the resampler.

Review commit: https://reviewboard.mozilla.org/r/196278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196278/

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1221580 - Use duplex when sndio is enabled.

Review commit: https://reviewboard.mozilla.org/r/196280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196280/

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8925038 [details]
Bug 1221580 - Update libcubeb to revision 7d00ea6705.

This touches only comments and non-tier-1 stuff.

Comment 11

[Landry Breuil (:gaston)]

(In reply to Paul Adenot (:padenot) from comment #9)
> Created attachment 8925039 [details]
> Bug 1221580 - Use duplex on BSDs.
> 
> Review commit: https://reviewboard.mozilla.org/r/196280/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/196280/

Im not sure this is what you want. This inconditionally enables full-duplex for everyone, not only sndio…

As i said in comment #4, id rather have it conditional to MOZ_SNDIO, be it a new section, or changing the 
#elif defined(XP_LINUX)
for
#elif defined(XP_LINUX) || defined(MOZ_SNDIO)
...

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8925039 [details]
Bug 1221580 - Use duplex when sndio is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196280/diff/1-2/

Comment 13

[Landry Breuil (:gaston)]

Comment on attachment 8925039 [details]
Bug 1221580 - Use duplex when sndio is enabled.

I dont know how to do it in mozreview, but definitely r+ for that part :)

Comment 14

[Landry Breuil (:gaston)]

Except it should be built-tested first :)

mozbuild.preprocessor.Error: ('/usr/obj/ports/firefox-57.0beta14/firefox-57.0b14/modules/libpref/init/all.js', 542, 'SYNTAX_ERR', 'defined(XP_LINUX || defined(MOZ_SNDIO))')

Comment 15

[Landry Breuil (:gaston)]

Comment on attachment 8925039 [details]
Bug 1221580 - Use duplex when sndio is enabled.

oh dammit copypaste.

Comment 16

[Matthew Gregan [:kinetik]]

Comment on attachment 8925038 [details]
Bug 1221580 - Update libcubeb to revision 7d00ea6705.

https://reviewboard.mozilla.org/r/196278/#review201716

Comment 17

[Landry Breuil (:gaston)]

Comment on attachment 8925039 [details]
Bug 1221580 - Use duplex when sndio is enabled.

https://reviewboard.mozilla.org/r/196280/#review203706

Damn you mozreview, land this already :)

Comment 18

[Paul Adenot (:padenot)]

Sorry got side tracked quite hard. Will land this in a few.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab8967c4e3c6
Update libcubeb to revision 7d00ea6705. r=kinetik,achronop
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc48926ed9b
Use duplex when sndio is enabled. r=gaston

Comment 20

[Landry Breuil (:gaston)]

Woot ! So given the branches status right now, i guess it's targeted at 59 now, or it will be in 58 ? Either way, i'll probably ship it to our users in 57...

Comment 21

[Landry Breuil (:gaston)]

Hmm and i think that what landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc48926ed9b isnt what you wanted, it seems a mixture of the two versions of the patch that was in mozreview.

Comment 22

[Jan Beich]

MOZ_SNDIO version appears to imply sndio capture delay is as bad as PulseAudio. Is that true?

Comment 23

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #22)
> MOZ_SNDIO version appears to imply sndio capture delay is as bad as
> PulseAudio. Is that true?

I have no idea what those numbers mean :)

Comment 24

[Alexandre Ratchov]

(In reply to Jan Beich from comment #22)
> MOZ_SNDIO version appears to imply sndio capture delay is as bad as
> PulseAudio. Is that true?

I don't know PulseAudio capture latency, cubeb_sndio
capture latency is a single block, which is 20ms by
default on OpenBSD. People may configure their machines
to use lower values. On modern (and not too busy) machines
2.5ms is known to work.

Comment 25

[Paul Adenot (:padenot)]

Those number aren't used. We should remove them.

I'll fix my mistake about the patch now.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9dda4555fa5
Followup, don't enable full-duplex on unknown platforms.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab8967c4e3c6
https://hg.mozilla.org/mozilla-central/rev/5cc48926ed9b
https://hg.mozilla.org/mozilla-central/rev/b9dda4555fa5