1351378 - Allow building against sndio on more than just OpenBSD

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Jan Beich]

sndio supports SunAudio, ALSA and OSS. Its libcubeb backend is 3x smaller than ALSA and PulseAudio.

Comment 1

[Jan Beich]

Attachment: Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

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

Comment 2

[Jan Beich]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

Landry, can you check for regressions?

Comment 3

[Jan Beich]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

https://reviewboard.mozilla.org/r/124310/#review126856

::: media/webrtc/trunk/webrtc/build/common.gypi:372
(Diff revision 1)
>          'defines': [
>            # doesn't support pthread_condattr_setclock
>            'WEBRTC_CLOCK_TYPE_REALTIME',
>          ],
>        }],
> -      ['OS=="openbsd"', {
> +      ['include_pulse_audio==1', {

Oops, this busted --enable-pulseaudio and silently breaking sndio in WebRTC.

Comment 4

[Jan Beich]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

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

Comment 5

[Landry Breuil (:gaston)]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

Diff reads good to me, will have to test next week (hopefully) - dont wait for me !

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

https://reviewboard.mozilla.org/r/124310/#review127108

::: toolkit/moz.configure:86
(Diff revision 2)
>  set_config('MOZ_JACK', jack)
>  set_define('MOZ_JACK', jack)
>  
> +# SNDIO cubeb and audio_device backend
> +# ==============================================================
> +option('--enable-sndio', env='MOZ_SNDIO',

I'd rather add a --enable-audio-backends option that can take one or more of jack,alsa,pulseaudio,sndio (check out --enable-default-toolkit)

--enable-audio-backends=jack would imply --enable-jack, =alsa would imply --enable-alsa, etc.
(using imply_option())

Comment 7

[Landry Breuil (:gaston)]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

The proposed patch doesnt cause regressions on OpenBSD:

$grep MOZ_SNDIO /usr/obj/m-c/config.*                                                                  
/usr/obj/m-c/config.status:    'MOZ_SNDIO': '1',

- but the direction glandium suggested is probably better...

Comment 8

[Landry Breuil (:gaston)]

Fwiw, alex and i redid more or less the same patch since he wants to test sndio/cubeb on linux - mike, it's been 7 months, can we at least move forward on this ? paul, any opinion ? Since we have runtime cubeb switching now...

Comment 9

[Jan Beich]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

https://reviewboard.mozilla.org/r/124310/#review199338

::: toolkit/moz.configure:86
(Diff revision 2)
>  set_config('MOZ_JACK', jack)
>  set_define('MOZ_JACK', jack)
>  
> +# SNDIO cubeb and audio_device backend
> +# ==============================================================
> +option('--enable-sndio', env='MOZ_SNDIO',

Creating --enable-audio-backends is not so simple. The current state is a mess:
- alsa[1] and sndio lack lazy bindings (e.g., to enable by default downstream)
- sndio doesn't play well with other backends under media/webrtc/
- alsa, jack, pulseaudio haven't been converted to moz.configure yet
- alsa and jack have broken full_duplex in libcubeb
- jack lacks a backend under media/webrtc/
- jack and pulseaudio code hasn't been tested on Android, OS X or Windows
- Android, OS X or Windows backends are unusable on (free)desktop Unix

[1] bug 1021761 has a patch

Comment 10

[Mike Hommey [:glandium]]

If multiple backends are not supported properly, then it's even more a reason for having *one* option, and not n, where you need to make all of them conflict with each other.

Comment 11

[Jan Beich]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

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

Comment 12

[Jan Beich]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

Rebased but still stalled due to scope creep requested by :glandium. moz.configure reminds me of Waf except for complete lack of documentation.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

https://reviewboard.mozilla.org/r/124310/#review240816

Comment 10 still applies.

Comment 14

[Jan Beich]

(In reply to Jan Beich from comment #9)

• sndio doesn't play well with other backends under media/webrtc/

Obsolete after bug 1397793.

• alsa, jack, pulseaudio haven't been converted to moz.configure yet

Obsolete after bug 1452509.

Comment 15

[mforney]

What's the current status of this patch? I'd also like to build firefox on Linux with sndio.

It sounds to me like --enable-audio-backends is a separate cleanup that should be tracked in its own bug.

Comment 16

[Paul Adenot (:padenot)]

Jan, maybe it's time to merge this or a variation of this? As you say, things should work, I have reports of downstream using this patch (or a variation) and folks say it works well. Quoting and breaking down a message of yours above:

Creating --enable-audio-backends is not so simple. The current state is a mess:

• alsa[1] and sndio lack lazy bindings (e.g., to enable by default downstream)

Those are not enabled by default by us, and it seems to work for Alsa for a few downstream users at least. Not sure if sndio is different here?

• sndio doesn't play well with other backends under media/webrtc/

We're not using this anymore in any circumstances.

• alsa, jack, pulseaudio haven't been converted to moz.configure yet

Done

• alsa and jack have broken full_duplex in libcubeb

Folks using this tell me it works currently (we've received patches since then).

• jack lacks a backend under media/webrtc/

Not needed.

• jack and pulseaudio code hasn't been tested on Android, OS X or Windows

This is not working today, is it? I reckon it would be rather cool to have jack working on Windows though so it could be used as with ASIO later in the chain to have amazing performance. This doesn't need to be done here, but I can help someone who would like to have it working probably. We can make configure fail in those cases.

• Android, OS X or Windows backends are unusable on (free)desktop Unix

This is fine, we can make configure fail in those cases as well. It's improbable that we'd ever implement a compatible API for those.

So really what's needed is a bit of code that splits the list of backeds requested for inclusion, and then have another bit of code that checks and decides based on (1) the presence of a header file for backends that are not usable lazily (2) a few hard-coded rules ("OpenSL on Windows -> failure", etc.). I can probably do something like that on the side (based on your patch), we're in the process of doing quite a few cleanups on cubeb anyways (with the switch to rust for tier-1 backends, we have two backends for the same audio stack, for example).

Comment 17

[Nordin Abouzahra [:sna]]

Attachment: add enable audio backends with sndio support

I took a shot at this.

I added --enable-audio-backends and the logic which should trigger a failure when selecting incompatible backends for certain targets. For example, if wasapi is selected on anything other than WINNT it errors out.

I also added the logic for allowing sndio to be built on more platforms than just OpenBSD.

I have not tested the other configurations due to lack of means (no OS X or Android).

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Nordin, please use phabricator to propose patches.
please see:
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Comment 19

[Nordin Abouzahra [:sna]]

Attachment: Bug 1351378 - Add an --enable-audio-backends option. r?mhentges,glandium

This addresses the original intent of the bug report which asks for allowing
sndio to be built on more than just OpenBSD. In addition of modifying the
existing --enable-sndio to support this request, the option
--enable-audio-backends was added which takes a list of possible backends to
support per discussion in the bug report.

For example specifying --enable-audio-backends=alsa,jack,pulseaudio,sndio
allows for runtime selection of those four cubeb backends. If all four backends
are available the user can specify media.cubeb.backend in about:config to
force a specific backend.

Comment 20

[Nordin Abouzahra [:sna]]

Attachment: Bug 1351378 - Address linter error. r?mhentges,Sylvestre

Depends on D141450

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c25c591841cd
Add an --enable-audio-backends option. r=mhentges

Comment 22

[Iulian Moraru]

Backed out for causing multiple failures.

Push with failures

Failure log for build bustages
Failure log py3 failures

Backout link

Comment 23

[Mitchell Hentges [:mhentges] 🦀]

Going to pass this to Nordin, looks like we need to tweak Windows defaults :)

Comment 24

[Nordin Abouzahra [:sna]]

I updated the commit to fix what the logs indicated.

I addressed cubeb_resampler.cpp being added redundantly when two different backends which depend on it were enabled in media/libcubeb/src/moz.build which caused failures when building for Android. For the py3 failures it seems that was caused by having the help= text missing {Enable|Disable} due to a non-constant default. I added that and it should be good now. Finally, the logic which checks if the platform supports the backend selected was fixed. Hopefully this addresses everything.

I cannot build or test for Windows but I suspect it should be good. I am waiting for the Linux build to finish compiling currently but it passed the configure phase fine so far. Thank you for reviewing.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7639234969b5
Add an --enable-audio-backends option. r=mhentges

Comment 26

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7639234969b5