Let's try to not diverge from Linux defaults too much. PulseAudio is required on BSDs for HTML5 audio/video since bug 852401 and for WebRTC since bug 807492. OpenBSD may opt out when bug 911450 lands. While sndio works on top of SunAudio it doesn't seem NetBSD or Solaris are supported. GStreamer is likely as portable as PulseAudio but lighter on Gnome dependencies. In case someone writes a libcubeb backend this may bring audio support to more platforms.

I, for one am definitely not fond of pulseaudio. Enabling Gstreamer by default is fine for media decoding, but for audio playing OpenBSD will never use pulseaudio, since we have a libcubeb sndio backend. To my knowledge, sndio is not tied to SunAudio, and it works on linux (there's a package in arch linux), so it should also work on other systems. As of now, pulse is needed for webrtc audio yes, but i disable webrtc for my users anyway because it's not working well so far, and it will only be enabled once we have the sndio backend for the sound.

Per request OpenBSD is left broken until bug 911450. (In reply to Landry Breuil (:gaston) from comment #1) > To my knowledge, sndio is not tied to SunAudio, and it works on linux > (there's a package in arch linux), so it should also work on other systems. What about existing backends, not writing new? Neither sio_sun.c nor sio_alsa.c work on platforms they weren't developed for. Let me demonstrate $ aucat -i foo.wav ALSA lib pcm_hw.c:1667:(_snd_pcm_hw_open) Invalid value for card couldn't open play stream: No such file or directory snd0: default: failed to open audio device $ make ... sio_sun.c:93:17: error: no member named 'msb' in 'struct audio_prinfo' par->msb = ai->msb; ~~ ^ sio_sun.c:95:17: error: no member named 'bps' in 'struct audio_prinfo' par->bps = ai->bps; ~~ ^ sio_sun.c:273:27: error: no member named 'bps' in 'struct audio_encoding' cap->enc[nenc].bps = ae.bps; ~~ ^ sio_sun.c:274:27: error: no member named 'msb' in 'struct audio_encoding' cap->enc[nenc].msb = ae.msb; ~~ ^ sio_sun.c:639:39: error: no member named 'bps' in 'struct audio_prinfo' aui.record.channels * aui.record.bps : 1; ~~~~~~~~~~ ^ sio_sun.c:641:35: error: no member named 'bps' in 'struct audio_prinfo' aui.play.channels * aui.play.bps : 1; ~~~~~~~~ ^ sio_sun.c:654:15: error: no member named 'block_size' in 'struct audio_prinfo' aui.record.block_size = round * ibpf; ~~~~~~~~~~ ^ sio_sun.c:656:13: error: no member named 'block_size' in 'struct audio_prinfo' aui.play.block_size = round * obpf; ~~~~~~~~ ^ sio_sun.c:667:21: error: no member named 'block_size' in 'struct audio_prinfo' infr = aui.record.block_size / ibpf; ~~~~~~~~~~ ^ sio_sun.c:668:19: error: no member named 'block_size' in 'struct audio_prinfo' onfr = aui.play.block_size / obpf; ~~~~~~~~ ^ sio_sun.c:721:17: error: no member named 'block_size' in 'struct audio_prinfo' aui.record.block_size / (par->bps * par->rchan) : ~~~~~~~~~~ ^ sio_sun.c:722:15: error: no member named 'block_size' in 'struct audio_prinfo' aui.play.block_size / (par->bps * par->pchan); ~~~~~~~~ ^ 12 errors generated. $ make ... sio_sun.c:90:51: warning: declaration of 'struct audio_encoding' will not be visible outside of this function [-Wvisibility] sio_sun_infotoenc(struct sio_sun_hdl *hdl, struct audio_encoding *ai, ^ sio_sun.c:93:15: error: incomplete definition of type 'struct audio_encoding' par->msb = ai->msb; ~~^ sio_sun.c:90:51: note: forward declaration of 'struct audio_encoding' sio_sun_infotoenc(struct sio_sun_hdl *hdl, struct audio_encoding *ai, ^ sio_sun.c:94:16: error: incomplete definition of type 'struct audio_encoding' par->bits = ai->precision; ~~^ sio_sun.c:90:51: note: forward declaration of 'struct audio_encoding' sio_sun_infotoenc(struct sio_sun_hdl *hdl, struct audio_encoding *ai, ^ sio_sun.c:95:15: error: incomplete definition of type 'struct audio_encoding' par->bps = ai->bps; ~~^ sio_sun.c:90:51: note: forward declaration of 'struct audio_encoding' sio_sun_infotoenc(struct sio_sun_hdl *hdl, struct audio_encoding *ai, ^ sio_sun.c:96:12: error: incomplete definition of type 'struct audio_encoding' switch (ai->encoding) { ~~^ sio_sun.c:90:51: note: forward declaration of 'struct audio_encoding' sio_sun_infotoenc(struct sio_sun_hdl *hdl, struct audio_encoding *ai, ^ sio_sun.c:139:11: error: use of undeclared identifier 'AUDIO_ENCODING_SLINEAR' *renc = AUDIO_ENCODING_SLINEAR; ^ sio_sun.c:141:11: error: use of undeclared identifier 'AUDIO_ENCODING_SLINEAR_LE' *renc = AUDIO_ENCODING_SLINEAR_LE; ^ sio_sun.c:143:11: error: use of undeclared identifier 'AUDIO_ENCODING_SLINEAR_BE' *renc = AUDIO_ENCODING_SLINEAR_BE; ^ sio_sun.c:145:11: error: use of undeclared identifier 'AUDIO_ENCODING_ULINEAR_LE' *renc = AUDIO_ENCODING_ULINEAR_LE; ^ sio_sun.c:147:11: error: use of undeclared identifier 'AUDIO_ENCODING_ULINEAR_BE' *renc = AUDIO_ENCODING_ULINEAR_BE; ^ sio_sun.c:167:19: error: use of undeclared identifier 'AUDIO_ENCODING_SLINEAR_LE' pr->encoding = AUDIO_ENCODING_SLINEAR_LE; ^ sio_sun.c:169:19: error: use of undeclared identifier 'AUDIO_ENCODING_SLINEAR_BE' pr->encoding = AUDIO_ENCODING_SLINEAR_BE; ^ sio_sun.c:171:19: error: use of undeclared identifier 'AUDIO_ENCODING_ULINEAR_LE' pr->encoding = AUDIO_ENCODING_ULINEAR_LE; ^ sio_sun.c:173:19: error: use of undeclared identifier 'AUDIO_ENCODING_ULINEAR_BE' pr->encoding = AUDIO_ENCODING_ULINEAR_BE; ^ sio_sun.c:229:24: error: variable has incomplete type 'struct audio_encoding' struct audio_encoding ae; ^ sio_sun.c:229:9: note: forward declaration of 'struct audio_encoding' struct audio_encoding ae; ^ sio_sun.c:241:22: error: use of undeclared identifier 'AUDIO_GETENC' if (ioctl(hdl->fd, AUDIO_GETENC, &ae) < 0) { ^ sio_sun.c:248:18: error: use of undeclared identifier 'AUDIO_ENCODINGFLAG_EMULATED' if (ae.flags & AUDIO_ENCODINGFLAG_EMULATED) ^ sio_sun.c:250:22: error: use of undeclared identifier 'AUDIO_ENCODING_SLINEAR_LE' if (ae.encoding == AUDIO_ENCODING_SLINEAR_LE) { ^ sio_sun.c:253:29: error: use of undeclared identifier 'AUDIO_ENCODING_SLINEAR_BE' } else if (ae.encoding == AUDIO_ENCODING_SLINEAR_BE) { ^ sio_sun.c:256:29: error: use of undeclared identifier 'AUDIO_ENCODING_ULINEAR_LE' } else if (ae.encoding == AUDIO_ENCODING_ULINEAR_LE) { ^ fatal error: too many errors emitted, stopping now [-ferror-limit=] 1 warning and 20 errors generated. > As of now, pulse is needed for webrtc audio yes, but i disable webrtc for my > users anyway because it's not working well so far, and it will only be > enabled once we have the sndio backend for the sound. The build currently fails if neither ALSA or PulseAudio is enabled on BSDs while include_internal_audio_device != 0. OpenBSD is among the platforms that build WebRTC by default. How do you spot regressions in WebRTC code if it's disabled even on your buildbot?

(In reply to Jan Beich from comment #2) > $ aucat -i foo.wav > ALSA lib pcm_hw.c:1667:(_snd_pcm_hw_open) Invalid value for card > couldn't open play stream: No such file or directory > snd0: default: failed to open audio device Bugzilla ate the following header # FreeBSD with alsa-plugins-(oss|pulseaudio) > $ make > ... > sio_sun.c:93:17: error: no member named 'msb' in 'struct audio_prinfo' > par->msb = ai->msb; > ~~ ^ Ditto # NetBSD > $ make > ... > sio_sun.c:90:51: warning: declaration of 'struct audio_encoding' will not be > visible > outside of this function [-Wvisibility] Oops, ignore errors about 'struct audio_encoding' in sio_sun_infotoenc(). They're from junk I've left after trying to fix NetBSD build. Other errors are specific to Solaris.

Comment on attachment 8351085 [details] [diff] [review] convert to XP_UNIX-like conditional, v2 (without openbsd) Review of attachment 8351085 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +5568,5 @@ > dnl ======================================================== > > dnl If using Linux, ensure that the PA library is available > +case "$OS_TARGET" in > +WINNT|Darwin|Android|OpenBSD) I don't see a reason to exclude openbsd here. AIUI, the cubeb backends are not mutually exclusive, and the pulseaudio backend doesn't make us link against libpulse (it's dlopened). For instance, you don't need pulse at runtime on linux, and cubeb will use alsa instead.

libpulse and libgst* configure checks are unlike other checks for dlopen libs (libgconf, libgio) as they don't auto-disable feature based on pkg-config. While mozboot/openbsd.py still has pulseaudio, comment 1 gives the impression it won't be for long. So, configure may fail due to missing pulseaudio package without --disable-pulseaudio. v2 patch tries to anticipate that without having to rework the checks to "auto" format. And this patch is an addition to v1 or v2 to relax the check for dlopen platforms. (In reply to Mike Hommey [:glandium] from comment #4) > Flags: review?(mh+mozilla@glandium.org) -> feedback+ Am I to interpret above as an advice to ask another build peer for review?

A small optimization. No need to install gstreamer libs on build boxes if we're not building for OS X.

(In reply to Jan Beich from comment #5) > (In reply to Mike Hommey [:glandium] from comment #4) > > Flags: review?(mh+mozilla@glandium.org) -> feedback+ > > Am I to interpret above as an advice to ask another build peer for review? It just means the patch looks generally good, but i want to see an updated patch.

Comment on attachment 8356507 [details] [diff] [review] avoid -lfoo check for dlopen case Review of attachment 8356507 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather keep those tests so that developer builds can have gstreamer working out of the box.

Comment on attachment 8356505 [details] [diff] [review] auto-disable feature like with other dlopen'd libs Review of attachment 8356505 [details] [diff] [review]: ----------------------------------------------------------------- Auto disabling features is not really something we want to do more of. Even for runtime-optional features. If you *want* those runtime-optional features off, you can turn them off manually, but a default build with no option should not lead to surprises.

Comment on attachment 8351007 [details] [diff] [review] convert to XP_UNIX-like conditional, v1 Landry, does libcubeb built against both pulseaudio and sndio work for you as described in comment 4? (In reply to Mike Hommey [:glandium] from comment #7) > (In reply to Jan Beich from comment #5) > > (In reply to Mike Hommey [:glandium] from comment #4) > > > Flags: review?(mh+mozilla@glandium.org) -> feedback+ > > > > Am I to interpret above as an advice to ask another build peer for review? > > It just means the patch looks generally good, but i want to see an updated > patch. As in v1 ? Only bug 806917 landing before may need updating the patch to fix conflict on GST_API_VERSION=0.10.

(In reply to Jan Beich from comment #10) > Comment on attachment 8351007 [details] [diff] [review] > convert to XP_UNIX-like conditional, v1 > > Landry, does libcubeb built against both pulseaudio and sndio work for you > as described in comment 4? No thanks for me. cubeb only needs a single backend.. Whatever ends up commited out of this, i know if needed i'll use --disable-pulseaudio. OpenBSD has a working slim native audio api, there's no need to spend time on a monster.

Comment on attachment 8351007 [details] [diff] [review] convert to XP_UNIX-like conditional, v1 I have nothing against gstreamer (i actually enable it by default since ffx 14) but please dont force-enable pulse on OpenBSD. We have a native sndio backend for cubeb, and we dont plan to use pulse for webrtc - the sndio backend is also in progress here.

Again, enabling libpulse makes the code *runtime-optional*. It's *not* force-enabled, it's force-built.

(In reply to Mike Hommey [:glandium] from comment #13) > Again, enabling libpulse makes the code *runtime-optional*. It's *not* > force-enabled, it's force-built. I got that - but just for the principle of least surprise, i'm against it - i dont want users reporting strange audio bugs because firefox automagically detected that pulseaudio was here. The port forces the presence of gstreamer at runtime because we want to use it, i dont want the same for pulse, and there's no way to ensure pulse is _not_ installed.

Only the first patch with v2 in name needs to land. Other patches (different approach + fixup) shouldn't be obsoleted and are left for reference.