844818 - Support PulseAudio in WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Comment 1

[Jan Beich]

Attachment: Make WebRTC aware of --enable-alsa/--enable-pulseaudio.

Push to Try first.

Comment 2

[Landry Breuil (:gaston)]

Heh awesome, you totally beat me to it - will try it out asap.

https://tbpl.mozilla.org/?tree=Try&rev=05fabb4b36d0

Comment 3

[Landry Breuil (:gaston)]

All green on try - i've tested it here on OpenBSD and smth still needs to be done to pass the include path for pulse headers MOZ_PULSEAUDIO_CFLAGS/LIBS to either webrtc or the system_wrappers :

../../../../../../dist/system_wrappers/pulse/pulseaudio.h:2:15: fatal error: 'pulse/pulseaudio.h' file not found

the header is in usr/local/include, and that include dir is not on the build command for audio_device_utility_linux.cc.

config.status:    (''' MOZ_PULSEAUDIO_CFLAGS ''', r''' -I/usr/local/include -D_REENTRANT '''),
config.status:    (''' MOZ_PULSEAUDIO_LIBS ''', r''' -L/usr/local/lib -Wl,-rpath,/usr/local/lib/pulseaudio -lpulse '''),
config.status:    (''' MOZ_PULSEAUDIO ''', r''' 1 '''),


But on the intent of the diff itself, we're on the right track since it correctly selected the pulseaudio backend.

Comment 4

[Jan Beich]

Attachment: v2, append cflags

Comment 5

[Landry Breuil (:gaston)]

Comment on attachment 718508 [details] [diff] [review]
v2, append cflags

https://tbpl.mozilla.org/?tree=Try&rev=9e4ee528ceaf

I dont think we use checkin flag for that :)

Comment 6

[Landry Breuil (:gaston)]

Comment on attachment 718508 [details] [diff] [review]
v2, append cflags

Seems to work fine here, building under audio_device properly gets ' -I/usr/local/include -D_REENTRANT' from MOZ_PULSEAUDIO_CFLAGS

Comment 7

[Landry Breuil (:gaston)]

jesup ? review ping ?

Comment 8

[Landry Breuil (:gaston)]

Attachment: Allow to separate alsa from pulse, v3

Unbitrot patch, still seems to do the right thing with pulse here. would be nice to get this in altogether with  844430, so that we can concentrate on bug 807492..

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 756899 [details] [diff] [review]
Allow to separate alsa from pulse, v3

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

r+; configure.in changes require a build peer

Comment 10

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0acfbc3b4941

Comment 11

[Landry Breuil (:gaston)]

my push with 844430 broke b2g builds: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=82fab5c8a6d5

And i'm pretty sure that cset is the guilty one..

Comment 12

[Landry Breuil (:gaston)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e43466d95794

Comment 13

[Landry Breuil (:gaston)]

If you compare the log of a broken build:
http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-ics_armv7a_gecko/1370688647/mozilla-inbound-ics_armv7a_gecko-bm62-build1-build311.txt.gz

with the log of the previous working build:
http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-ics_armv7a_gecko/1370684387/mozilla-inbound-ics_armv7a_gecko-bm57-build1-build290.txt.gz

trunk/webrtc/modules/audio_device/audio_device_impl.cc:36:44: error: audio_device_utility_linux.h: No such file or directory

So something in the cset makes b2g use linux codepaths, maybe the WEBRTC_LINUX forced define for everyone ?

Comment 14

[Landry Breuil (:gaston)]

Yeah, very likely to be WEBRTC_LINUX set in audio_device.gypi, which in turns includes audio_device_utility_linux.h in audio_device/audio_device_impl.cc

Comment 15

[Landry Breuil (:gaston)]

Pushed to try without the defines += WEBRTC_LINUX chunk in https://tbpl.mozilla.org/?tree=Try&rev=848613146528.
Since in bug 807492 part 5 we add a WEBRTC_BSD flag, we might aswell reuse it there instead of breaking b2g/linux.... and it paves the way for an sndio audio_device backend in the works.

Comment 16

[Landry Breuil (:gaston)]

Nope, thats not it, b2g still burning without WEBRTC_LINUX define. Somehow, passing MOZ_ALSA | MOZ_PULSEAUDIO from configure.in to audio_device.gypi enables more stuff on b2g. Jan ?

Comment 17

[Landry Breuil (:gaston)]

Ok, found the reason (i think). A broken b2g build doesnt have media/webrtc/trunk/webrtc/modules/audio_device/linux in the include path -I list - while a working one has it. Probably because of :

    2.12 -        ['OS=="linux"', {
    2.13 +        ['include_alsa_audio==1 or include_pulse_audio==1', {
    2.14            'include_dirs': [
    2.15              'linux',
    2.16            ],

So when on b2g/gonk, OS is also linux.. so gonk should also be in that condition. And maybe android ? though it didnt break on my push, and seems to have its own OS value.. looking in the tree, moz_widget_toolkit_gonk==1 seems the only way to detect gonk in gyp, so let's go for this.

Pushed to try (well, once the push actually goes...) with 'alsa|pulse|gonk' in the test.

I'm still unsure about the defines += WEBRTC_LINUX... we use WEBRTC_BSD in bug 807492 part 5 which is all the audio_device #ifdef changes, so i'd rather go the way of the least surprise here... ted, any insight ?

Comment 18

[Landry Breuil (:gaston)]

https://tbpl.mozilla.org/?tree=Try&rev=dc19a93fb7ce

Comment 19

[Landry Breuil (:gaston)]

All green on try... but i'm not sure try shows the correct cset on https://hg.mozilla.org/try/rev/fab791796087 since this one doesnt show moz_widget_toolkit_gonk==1 in the test, although last time i pushed this b2g was broken with it..

Comment 20

[Landry Breuil (:gaston)]

Oh, the actual change ended up in the try cset : https://hg.mozilla.org/try/rev/dc19a93fb7ce - so adding moz_widget_toolkit_gonk=1 indeed fixes b2g builds. I will make a full build on bsd with that cset + removing the defines+=WEBRTC_LINUX in audio_device.gypi + all the patches from 807492 to ensure the WEBRTC_LINUX thing is not needed for the BSDs.

Comment 21

[Landry Breuil (:gaston)]

Attachment: Allow to separate alsa from pulse, v4

So all green on try + built on OpenBSD fine without the defines += [ WEBRTC_LINUX ], so lets keep this out. Carrying jesup's r+, just reasking confirmation from ted for the .gyp part..

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 760965 [details] [diff] [review]
Allow to separate alsa from pulse, v4

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

::: media/webrtc/trunk/webrtc/modules/audio_device/audio_device.gypi
@@ +44,5 @@
>          'dummy/audio_device_dummy.h',
>          'dummy/audio_device_utility_dummy.h',
>        ],
>        'conditions': [
> +        ['moz_widget_toolkit_gonk==1 or include_alsa_audio==1 or include_pulse_audio==1', {

This just feels wrong. What are you actually trying to express here? "Things that are Linux or things that are using ALSA or Pulse"? If so, shouldn't you keep the original OS==linux and just add on the (or alsa or pulse) conditional?

Comment 23

[Landry Breuil (:gaston)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> Comment on attachment 760965 [details] [diff] [review]
> Allow to separate alsa from pulse, v4
> 
> Review of attachment 760965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/trunk/webrtc/modules/audio_device/audio_device.gypi
> @@ +44,5 @@
> >          'dummy/audio_device_dummy.h',
> >          'dummy/audio_device_utility_dummy.h',
> >        ],
> >        'conditions': [
> > +        ['moz_widget_toolkit_gonk==1 or include_alsa_audio==1 or include_pulse_audio==1', {
> 
> This just feels wrong. What are you actually trying to express here? "Things
> that are Linux or things that are using ALSA or Pulse"? If so, shouldn't you
> keep the original OS==linux and just add on the (or alsa or pulse)
> conditional?

Agreed. Since include_alsa_audio==1 so far means OS==linux, can we make that OS==linux or include_pulse_audio==1 ? Or be really clear and have include_alsa_audio==1 in the test anyway ?

Comment 24

[Landry Breuil (:gaston)]

Attachment: Allow to separate alsa from pulse, v5

Lets be conservative & use OS=="linux" or include_alsa_audio==1 or include_pulse_audio==1 to ensure all cases are covered, 5th time should be the good one. Marked the dep on 864513 to ensure the audio_device.gypi patch is upstreamed..

Comment 25

[Jan Beich]

I need for --disable-alsa to actually work on linux and --enable-alsa everywhere else. There should be no difference with --enable/--disable-pulseaudio. Using OS=="linux" or WEBRTC_BSD means the next porter has to do the work again e.g., for solaris.

Comment 26

[Jan Beich]

Attachment: Allow to separate alsa from pulse, v6

WEBRTC_FOO that affects pulse should be here, not in bug 807492.

(In reply to Landry Breuil (:gaston) from comment #13)
> trunk/webrtc/modules/audio_device/audio_device_impl.cc:36:44: error:
> audio_device_utility_linux.h: No such file or directory

This seems like an upstream bug. B2G uses "dummy" backend so
it shouldn't include any "linux" headers.

Comment 27

[Jan Beich]

Oops, openbsd-specific quirks should stay in bug 807492. Push to Try in the meantime.

Comment 28

[Landry Breuil (:gaston)]

Comment on attachment 761588 [details] [diff] [review]
Allow to separate alsa from pulse, v5

Please don't interfer with what i'm trying to do here, this is already complicated enough. Not including linux/ on b2g breaks, and i dont want to break it.

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 761588 [details] [diff] [review]
Allow to separate alsa from pulse, v5

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

Thanks, that seems a lot more straightforward.

Comment 30

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb2eb06412d

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbb2eb06412d