Closed Bug 1384655 Opened 7 years ago Closed 7 years ago

Build with --enable-alsa --enable-tests is broken by webrtc::AudioDeviceLinuxALSA

Categories

(Core :: WebRTC, defect)

Unspecified
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: jld, Assigned: padenot)

References

Details

Attachments

(1 file)

I ran into this while trying to get an --enable-alsa build for bug 1384439. STR is to build with --enable-alsa and without --disable-tests; result is the linker error mentioned in bug 1352812 comment #8: > .../audio_device_alsa_linux.cc:2152: undefined reference to `LogTime(AsyncLatencyLogger::LatencyLogIndex, unsigned long, long)' > .../audio_device_alsa_linux.cc:2155: undefined reference to `LogLatency(AsyncLatencyLogger::LatencyLogIndex, unsigned long, long)' That seems to be caused by these macros in audio_device_alsa_linux.cc: > #define LOG_FIRST_CAPTURE(x) LogTime(AsyncLatencyLogger::AudioCaptureBase, \ > reinterpret_cast<uint64_t>(x), 0) > #define LOG_CAPTURE_FRAMES(x, frames) LogLatency(AsyncLatencyLogger::AudioCapture, \ > reinterpret_cast<uint64_t>(x), frames) I don't know where those came from — apparently not the upstream webrtc revision mentioned in bug 1341285 comment #0 — but they're calling methods on AsyncLatencyLogger (documented to be a singleton, so it's not as if AudioDeviceLinuxALSA should be inheriting from it?) as if they were global functions. Which, as the linker points out, they're not. If nobody has any better idea, I can post the trivial patch to delete the macro bodies, because they're clearly not doing anything useful as it stands. Also, given that --disable-tests is an effective workaround… is this all trivially dead code in non-test usage and being GCed by the linker? Is that expected?
We no longer use the webrtc.org audio backends so I don't think there's any harm in just not building the ALSA backend. I'd suggested updating [1] to set: 'include_alsa_audio': 0 That's easier to maintain than a patch against upstream webrtc.org code. [1] http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/build/gyp.mozbuild#23
Assignee: nobody → padenot
Comment on attachment 8890790 [details] Bug 1384655 - Remove obsolete latency measuring macros in downstream code we don't use anymore. https://reviewboard.mozilla.org/r/161988/#review167274 lgtm
Attachment #8890790 - Flags: review?(dminor) → review+
Pushed by paul@paul.cx: https://hg.mozilla.org/integration/autoland/rev/7829ceaaec31 Don't compile Alsa support in WebRTC on Linux. r=dminor
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to Dan Minor [:dminor] from comment #1) > We no longer use the webrtc.org audio backends After the change here getUserMedia fails in Firefox built with --disable-pulseaudio --enable-alsa. Am I missing something?
Jan, I'm very sorry we did that too quickly. I'm working on a fix right now, that will bring speed improvement, build time improvements, and solves the issue we had originally as well. For some weird reason, even if, these days, we use cubeb's input and output device code, webrtc still was initializing a bunch of platform specific code (whether it be pulse or alsa or window's or osx' platform API). This is quite expensive and wasteful, so I'm doing a patch that works around this.
The proper fix for this (explained in comment 7) is going to land in bug 1385258.
Depends on: 1385258
Given bug 1385258 is stalled maybe this one should be backed out.
Yes sorry, doing that now.
Pushed by paul@paul.cx: https://hg.mozilla.org/integration/mozilla-inbound/rev/49af461825cb Backed out changeset 7829ceaaec31 for busting gUM on some configurations.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
jesup, I want to land this to unbreak jed's thing. I'm doing the real work in bug 1385258.
Comment on attachment 8890790 [details] Bug 1384655 - Remove obsolete latency measuring macros in downstream code we don't use anymore. https://reviewboard.mozilla.org/r/161988/#review171088 ::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_alsa_linux.cc (Diff revision 2) > -#include "Latency.h" > - > -#define LOG_FIRST_CAPTURE(x) LogTime(AsyncLatencyLogger::AudioCaptureBase, \ > - reinterpret_cast<uint64_t>(x), 0) > -#define LOG_CAPTURE_FRAMES(x, frames) LogLatency(AsyncLatencyLogger::AudioCapture, \ > - reinterpret_cast<uint64_t>(x), frames) Can you clean up sndio/audio_device_sndio.cc as well?
Yes, updated.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Comment on attachment 8890790 [details] Bug 1384655 - Remove obsolete latency measuring macros in downstream code we don't use anymore. https://reviewboard.mozilla.org/r/161988/#review171150
Attachment #8890790 - Flags: review?(rjesup) → review+
Pushed by paul@paul.cx: https://hg.mozilla.org/integration/mozilla-inbound/rev/313e352e2440 Remove obsolete latency measuring macros in downstream code we don't use anymore. r=jesup
Can you request approval for Beta 56? At least AlpineLinux and DragonFly build with ALSA-only; FreeBSD keeps both ALSA and PulseAudio as opt-out; Fedora keeps ALSA as opt-in; Ubuntu disabled ALSA in FF55 due to unrelated bustage. ALSA-only (NPOTB) Try on FF56: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4205f870f791
Flags: needinfo?(padenot)
Comment on attachment 8890790 [details] Bug 1384655 - Remove obsolete latency measuring macros in downstream code we don't use anymore. Approval Request Comment [Feature/Bug causing the regression]: A patch in this very bug that landed just before the cutoff. [User impact if declined]: Downstream packager will have trouble compiling firefox [Is this code covered by automated tests?]: it's a built-time issue, and Firefox does not use this code in tier-1 configuration [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: none [List of other uplifts needed for the feature/fix]: just the two patches in this bug [Is the change risky?]: no [Why is the change risky/not risky?]: this is a backout and code deletion of things that we don't run [String changes made/needed]:
Flags: needinfo?(padenot)
Attachment #8890790 - Flags: approval-mozilla-beta?
Comment on attachment 8890790 [details] Bug 1384655 - Remove obsolete latency measuring macros in downstream code we don't use anymore. Fix for WebRTC test builds, let's uplift this for beta 2.
Attachment #8890790 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Can you backport mozilla-central changeset 49af461825cb as well?
Flags: needinfo?(padenot)
huh, not sure what mozreview or I have done here, there is only one patch showing up. I'll take care of the other one, if was mentioned in the uplift request.
Flags: needinfo?(padenot)
Pushed the other one to beta with Sylvestre blessing, on irc. This is also non-tier1 and NPOTB. https://hg.mozilla.org/releases/mozilla-beta/rev/04478e41dbf2d4143ab9d437c24a2f3783531af4 Jan, I believe that after all this mess, everything is in order, right? Sorry again about that.
Flags: needinfo?(jbeich)
Yes. getUserMedia (audio) now works fine with ALSA-only builds on FF56.
Flags: needinfo?(jbeich)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: