Last Comment Bug 780432 - add --enable-pulseaudio configure option
: add --enable-pulseaudio configure option
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All FreeBSD
: -- normal (vote)
: mozilla17
Assigned To: Jan Beich
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: 844818
  Show dependency treegraph
 
Reported: 2012-08-04 20:29 PDT by Jan Beich
Modified: 2013-02-25 06:16 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add --enable-pulseaudio (5.21 KB, patch)
2012-08-04 20:37 PDT, Jan Beich
no flags Details | Diff | Splinter Review
add --enable-pulseaudio (hg version) (5.12 KB, patch)
2012-08-04 20:51 PDT, Jan Beich
no flags Details | Diff | Splinter Review
media (1.09 KB, patch)
2012-08-04 23:36 PDT, Jan Beich
kinetik: review-
Details | Diff | Splinter Review
rest (3.76 KB, patch)
2012-08-04 23:38 PDT, Jan Beich
no flags Details | Diff | Splinter Review
Bug 780432 - Add --enable-pulseaudio configure option. (4.25 KB, patch)
2012-08-05 11:19 PDT, Jan Beich
kinetik: review+
khuey: review-
Details | Diff | Splinter Review
Bug 780432 - explicitly pass CFLAGS for ALSA and PulseAudio (519 bytes, patch)
2012-08-05 21:09 PDT, Jan Beich
kinetik: review-
Details | Diff | Splinter Review
Bug 780432 - explicitly pass CFLAGS for ALSA and PulseAudio (517 bytes, patch)
2012-08-05 22:09 PDT, Jan Beich
kinetik: review+
Details | Diff | Splinter Review
Bug 780432 - Add --enable-pulseaudio configure option. r=kinetik,khuey (3.49 KB, patch)
2012-08-07 15:00 PDT, Jan Beich
khuey: review+
Details | Diff | Splinter Review
Bug 780432 - Explicitly pass CFLAGS for ALSA and PulseAudio. r=kinetik (528 bytes, patch)
2012-08-07 15:07 PDT, Jan Beich
no flags Details | Diff | Splinter Review

Description Jan Beich 2012-08-04 20:29:14 PDT

    
Comment 1 Jan Beich 2012-08-04 20:37:54 PDT
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
Comment 2 Jan Beich 2012-08-04 20:51:10 PDT
Created attachment 649060 [details] [diff] [review]
add --enable-pulseaudio (hg version)
Comment 3 cajbir (:cajbir) 2012-08-04 22:42:29 PDT
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 2012-08-04 23:36:39 PDT
Created attachment 649073 [details] [diff] [review]
media
Comment 5 Jan Beich 2012-08-04 23:38:28 PDT
Created attachment 649074 [details] [diff] [review]
rest
Comment 6 Matthew Gregan [:kinetik] 2012-08-05 03:30:09 PDT
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 2012-08-05 11:19:50 PDT
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.
Comment 8 Matthew Gregan [:kinetik] 2012-08-05 19:44:08 PDT
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 2012-08-05 21:09:21 PDT
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.
Comment 10 Matthew Gregan [:kinetik] 2012-08-05 21:20:48 PDT
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 2012-08-05 22:09:24 PDT
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.
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-07 11:51:22 PDT
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 2012-08-07 15:00:29 PDT
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 ...
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-07 15:01:35 PDT
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 2012-08-07 15:07:01 PDT
Created attachment 649829 [details] [diff] [review]
Bug 780432 - Explicitly pass CFLAGS for ALSA and PulseAudio. r=kinetik

Note You need to log in before you can comment on or make changes to this bug.