Closed Bug 1351378 Opened 5 years ago Closed 2 months ago

Allow building against sndio on more than just OpenBSD

Categories

(Firefox Build System :: General, enhancement)

Unspecified
FreeBSD
enhancement
Not set
normal

Tracking

(firefox55 wontfix, firefox100 fixed)

RESOLVED FIXED
100 Branch
Tracking Status
firefox55 --- wontfix
firefox100 --- fixed

People

(Reporter: jbeich, Assigned: NordinAbouzahra+BMO)

References

Details

Attachments

(3 files, 1 obsolete file)

sndio supports SunAudio, ALSA and OSS. Its libcubeb backend is 3x smaller than ALSA and PulseAudio.
Comment on attachment 8852071 [details]
Bug 1351378 - Expose sndio as an option for Linux and FreeBSD.

Landry, can you check for regressions?
Attachment #8852071 - Flags: feedback?(landry)
Attachment #8852071 - Flags: feedback?(landry)
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 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())
Attachment #8852071 - Flags: review?(mh+mozilla)
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...
Attachment #8852071 - Flags: feedback?(landry) → feedback+
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...
Flags: needinfo?(padenot)
Flags: needinfo?(mh+mozilla)
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
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.
Flags: needinfo?(mh+mozilla)
Product: Core → Firefox Build System
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 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.
Attachment #8852071 - Flags: review?(mh+mozilla)
Flags: needinfo?(padenot)

(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.

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.

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).

Flags: needinfo?(jbeich)

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).

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

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.

Assignee: nobody → NordinAbouzahra+BMO
Status: NEW → ASSIGNED

Depends on D141450

Attachment #9268388 - Attachment is obsolete: true
Attachment #9268387 - Attachment description: Bug 1351378 - Add an --enable-audio-backends option. r?mhentges,Sylvestre → Bug 1351378 - Add an --enable-audio-backends option. r?mhentges,glandium
Pushed by mhentges@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c25c591841cd
Add an --enable-audio-backends option. r=mhentges
Flags: needinfo?(mhentges)

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

Flags: needinfo?(mhentges) → needinfo?(NordinAbouzahra+BMO)

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.

Flags: needinfo?(NordinAbouzahra+BMO)
Pushed by mhentges@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7639234969b5
Add an --enable-audio-backends option. r=mhentges
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
Flags: needinfo?(jbeich)
You need to log in before you can comment on or make changes to this bug.