add --enable-pulseaudio configure option

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jan Beich (away from May 25 to June 11), Assigned: Jan Beich (away from May 25 to June 11))

Tracking

Trunk
mozilla17
All
FreeBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Comment hidden (empty)
Created attachment 649057 [details] [diff] [review]
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
Attachment #649057 - Flags: review?(chris.double)
Created attachment 649060 [details] [diff] [review]
add --enable-pulseaudio (hg version)
Attachment #649057 - Attachment is obsolete: true
Attachment #649057 - Flags: review?(chris.double)
Attachment #649060 - Flags: review?(chris.double)

Comment 3

5 years ago
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".
Attachment #649060 - Flags: review?(chris.double)
Created attachment 649073 [details] [diff] [review]
media
Attachment #649060 - Attachment is obsolete: true
Attachment #649073 - Flags: review?(kinetik)
Created attachment 649074 [details] [diff] [review]
rest
Attachment #649074 - Flags: review?(khuey)
Attachment #649073 - Attachment description: Bug 780432 - Add --enable-pulseaudio configure option. → media
Attachment #649073 - Attachment filename: Bug-780432---Add---enable-pulseaudio-configure-opt.patch → pulse_media.patch
Attachment #649074 - Attachment description: Bug 780432 - Add --enable-pulseaudio configure option. → rest
Attachment #649074 - Attachment filename: Bug-780432---Add---enable-pulseaudio-configure-opt.patch → pulse_rest.patch
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.
Attachment #649073 - Flags: review?(kinetik) → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 649114 [details] [diff] [review]
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.
Attachment #649073 - Attachment is obsolete: true
Attachment #649074 - Attachment is obsolete: true
Attachment #649074 - Flags: review?(khuey)
Attachment #649114 - Flags: review?(kinetik)
Attachment #649114 - Flags: review?(khuey)
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.
Attachment #649114 - Flags: review?(kinetik) → review+
Created attachment 649174 [details] [diff] [review]
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.
Attachment #649174 - Flags: review?(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.
Attachment #649174 - Flags: review?(kinetik) → review-
Created attachment 649182 [details] [diff] [review]
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.
Attachment #649174 - Attachment is obsolete: true
Attachment #649182 - Flags: review?(kinetik)
Attachment #649182 - Flags: review?(kinetik) → review+
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 :-/
Attachment #649114 - Flags: review?(khuey) → review-
Created attachment 649826 [details] [diff] [review]
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] (khuey@mozilla.com) 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 ...
Attachment #649114 - Attachment is obsolete: true
Attachment #649826 - Flags: review?(khuey)
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.
Attachment #649826 - Flags: review?(khuey) → review+
Created attachment 649829 [details] [diff] [review]
Bug 780432 - Explicitly pass CFLAGS for ALSA and PulseAudio. r=kinetik
Attachment #649182 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee816acf901
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c486bb219f
Assignee: nobody → jbeich
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/5ee816acf901
https://hg.mozilla.org/mozilla-central/rev/a5c486bb219f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 844818
You need to log in before you can comment on or make changes to this bug.