Closed Bug 844818 Opened 7 years ago Closed 6 years ago

Support PulseAudio in WebRTC

Categories

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

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jbeich, Assigned: gaston)

References

Details

(Whiteboard: [WebRTC] [blocking-webrtc-] [qa-])

Attachments

(2 files, 4 obsolete files)

No description provided.
Push to Try first.
Attachment #717857 - Flags: review?(rjesup)
Heh awesome, you totally beat me to it - will try it out asap.

https://tbpl.mozilla.org/?tree=Try&rev=05fabb4b36d0
Whiteboard: [WebRTC] [blocking-webrtc-]
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.
Attached patch v2, append cflags (obsolete) — Splinter Review
Attachment #717857 - Attachment is obsolete: true
Attachment #717857 - Flags: review?(rjesup)
Attachment #718508 - Flags: review?(rjesup)
Attachment #718508 - Flags: checkin?(landry)
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 :)
Attachment #718508 - Flags: checkin?(landry)
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
Attachment #718508 - Flags: feedback+
jesup ? review ping ?
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..
Attachment #718508 - Attachment is obsolete: true
Attachment #718508 - Flags: review?(rjesup)
Attachment #756899 - Flags: review?(rjesup)
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
Attachment #756899 - Flags: review?(ted)
Attachment #756899 - Flags: review?(rjesup)
Attachment #756899 - Flags: review+
Attachment #756899 - Flags: review?(ted) → review+
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..
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 ?
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
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.
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 ?
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 ?
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..
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.
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..
Attachment #756899 - Attachment is obsolete: true
Attachment #760965 - Flags: review?(ted)
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?
Attachment #760965 - Flags: review?(ted) → review-
(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 ?
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..
Assignee: nobody → landry
Attachment #760965 - Attachment is obsolete: true
Attachment #761588 - Flags: review?(ted)
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.
Assignee: landry → nobody
Assignee: nobody → landry
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.
Attachment #761588 - Attachment is obsolete: true
Attachment #761588 - Flags: review?(ted)
Attachment #761661 - Flags: feedback?(landry)
Oops, openbsd-specific quirks should stay in bug 807492. Push to Try in the meantime.
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.
Attachment #761588 - Attachment is obsolete: false
Attachment #761588 - Flags: review?(ted)
Attachment #761661 - Flags: feedback?(landry)
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.
Attachment #761588 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/bbb2eb06412d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
Depends on: 910875
Blocks: 911450
No longer blocks: webrtc_upstream_bugs
You need to log in before you can comment on or make changes to this bug.