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)
Tracking
()
VERIFIED
FIXED
mozilla56
People
(Reporter: jld, Assigned: padenot)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
jesup
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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?
Comment 1•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → padenot
Comment 3•7 years ago
|
||
mozreview-review |
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
Comment 5•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
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?
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
The proper fix for this (explained in comment 7) is going to land in bug 1385258.
Given bug 1385258 is stalled maybe this one should be backed out.
Assignee | ||
Comment 10•7 years ago
|
||
Yes sorry, doing that now.
Comment 11•7 years ago
|
||
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49af461825cb
Backed out changeset 7829ceaaec31 for busting gUM on some configurations.
Assignee | ||
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
jesup, I want to land this to unbreak jed's thing. I'm doing the real work in bug 1385258.
Comment 14•7 years ago
|
||
mozreview-review |
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?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
Yes, updated.
Comment 17•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Comment 18•7 years ago
|
||
mozreview-review |
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+
Comment 19•7 years ago
|
||
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
Comment 20•7 years ago
|
||
bugherder |
Comment 21•7 years ago
|
||
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)
Assignee | ||
Comment 22•7 years ago
|
||
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+
Comment 24•7 years ago
|
||
bugherder uplift |
Comment 25•7 years ago
|
||
Can you backport mozilla-central changeset 49af461825cb as well?
Flags: needinfo?(padenot)
Assignee | ||
Comment 26•7 years ago
|
||
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)
Assignee | ||
Comment 27•7 years ago
|
||
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)
Comment 28•7 years ago
|
||
Yes. getUserMedia (audio) now works fine with ALSA-only builds on FF56.
Flags: needinfo?(jbeich)
You need to log in
before you can comment on or make changes to this bug.
Description
•