780432 - add --enable-pulseaudio configure option

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Jan Beich]

Attachment: add --enable-pulseaudio

PulseAudio is a opt-in option in PkgSrc (since 2011/04/26) and FreeBSD ports (since 2012/07/26). This is an attempt to upstream the support.

I've also used it to diagnose a sound issue, see
http://lists.freebsd.org/pipermail/freebsd-gecko/2012-June/002408.html

Comment 2

[Jan Beich]

Attachment: add --enable-pulseaudio (hg version)

Comment 3

[cajbir (:cajbir)]

Comment on attachment 649060 [details] [diff] [review]
add --enable-pulseaudio (hg version)

Please split the patch up into the "media" subdirectory portions, and the rest. The media stuff should be reviewed by ":kinetik" and the other stuff needs to be reviewed by a build peer. Maybe ":khuey".

Comment 4

[Jan Beich]

Attachment: media

Comment 5

[Jan Beich]

Attachment: rest

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 649073 [details] [diff] [review]
media

libcubeb's PulseAudio backend hasn't been widely tested, but I'd like to get it into better shape.  Please change the configure.in description for the option to make it clear that it's experimental.  The eventually plan is to have it enabled by default and dynamically select PA or ALSA based on the system.

I'd rather not enable the sydneyaudio code at all, I have no idea what state it's in and I don't want to support it.  The plan is to remove sydneyaudio from the tree in the near future. r- based on this.

If there are playback issues on FreeBSD with libcubeb, please file bugs so that they can be addressed.

Comment 7

[Jan Beich]

Attachment: Bug 780432 - Add --enable-pulseaudio configure option.

Hmm, splitting an atomic commit doesn't seem to be useful in this case. I'll just request review from both maintainers unless "splinter review" button is just for show.

(In reply to Matthew Gregan [:kinetik] from comment #6)
> libcubeb's PulseAudio backend hasn't been widely tested, but I'd like to get
> it into better shape.

The lack of exposure for PA backend in libsydneyaudio is what led to its current state. Hopefully, PA backend in libcubeb won't share similar fate.

> Please change the configure.in description for the option to make it
> clear that it's experimental.

Done.

> The eventually plan is to have it enabled by default and dynamically
> select PA or ALSA based on the system.

MOZ_ARG_DISABLE_BOOL on any Unix except OS X? I'd much more like runtime backend switching, at least with GStreamer as it already has pulse, oss4, sunaudio, etc.

> I'd rather not enable the sydneyaudio code at all,

As a user I expect --enable-pulseaudio to pass all audio through it, not only when media.use_cubeb is true (default). This also allows me to easily determine if the issue is in libcubeb or libsydneyaudio which is harder when they use different backends.

Ah, whatever... I've removed libsydneyaudio ifdef. It now uses ALSA (or OSS here) even if --enable-pulseaudio is specified.

> I have no idea what state it's in and I don't want to support it.

It works here if I apply bug 685258. But pause latency with PA is as bad as with OSS backend.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 649114 [details] [diff] [review]
Bug 780432 - Add --enable-pulseaudio configure option.

Thanks for the patch.  MOZ_PULSEAUDIO_CFLAGS doesn't seem to be used anywhere, so perhaps that can be removed.

(In reply to Jan Beich from comment #7)
> MOZ_ARG_DISABLE_BOOL on any Unix except OS X? I'd much more like runtime
> backend switching, at least with GStreamer as it already has pulse, oss4,
> sunaudio, etc.

It'll be runtime switching.

Comment 9

[Jan Beich]

Attachment: Bug 780432 - explicitly pass CFLAGS for ALSA and PulseAudio

(In reply to Matthew Gregan [:kinetik] from comment #8)
> Comment on attachment 649114 [details] [diff] [review]
> Bug 780432 - Add --enable-pulseaudio configure option.
> 
> Thanks for the patch.  MOZ_PULSEAUDIO_CFLAGS doesn't seem to be used
> anywhere, so perhaps that can be removed.

No, it's a bug which also affects ALSA.

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 649174 [details] [diff] [review]
Bug 780432 - explicitly pass CFLAGS for ALSA and PulseAudio

Review of attachment 649174 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libcubeb/src/Makefile.in
@@ +51,5 @@
>  endif
>  
>  include $(topsrcdir)/config/rules.mk
> +
> +INCLUDES += \

This needs to be CFLAGS, or the MOZ_*_CFLAGS things need to be changed to be the result of pkg-config --cflags-only-I.  The former seems preferable.

Comment 11

[Jan Beich]

Attachment: Bug 780432 - explicitly pass CFLAGS for ALSA and PulseAudio

(In reply to Matthew Gregan [:kinetik] from comment #10)
> pkg-config --cflags-only-I

With PKG_CHECK_MODULES? It's easier to write $(MOZ_FOO_INCLUDES) as

  $(filter -I%, $(MOZ_FOO_CFLAGS))

But I like CFLAGS approach more, too.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 649114 [details] [diff] [review]
Bug 780432 - Add --enable-pulseaudio configure option.

This patch needs to be updated.  autoconf.mk substitution was totally reworked last week.  Sorry about that :-/

Comment 13

[Jan Beich]

Attachment: Bug 780432 - Add --enable-pulseaudio configure option. r=kinetik,khuey

I've added r=khuey as you don't seem to have other objections.

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> This patch needs to be updated.  autoconf.mk substitution was totally
> reworked last week.  Sorry about that :-/

It seems to work fine if I remove config/autoconf.mk.in hunk now,
after bug 742795 landed.

$ fgrep -i pulse config/autoconf.mk
MOZ_PULSEAUDIO = 1
MOZ_PULSEAUDIO_CFLAGS = -I/usr/local/include -D_REENTRANT
MOZ_PULSEAUDIO_LIBS = -L/usr/local/lib -lpulse -pthread
ac_configure_args =  ... --enable-pulseaudio ...

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 649826 [details] [diff] [review]
Bug 780432 - Add --enable-pulseaudio configure option. r=kinetik,khuey

Review of attachment 649826 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah it looks reasonable enough.

Comment 15

[Jan Beich]

Attachment: Bug 780432 - Explicitly pass CFLAGS for ALSA and PulseAudio. r=kinetik

Comment 16

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee816acf901
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c486bb219f

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ee816acf901
https://hg.mozilla.org/mozilla-central/rev/a5c486bb219f