Closed
Bug 1265641
Opened 8 years ago
Closed 8 years ago
Delay-agnostic pref applied to wrong VoiceEngine
Categories
(Core :: WebRTC, defect, P1)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: jesup, Assigned: jesup)
References
Details
Attachments
(1 file)
9.19 KB,
patch
|
padenot
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The Delay-Agnostic AEC pref added in patch 2 of bug 1226347 (overriding a hard-coding of the mode to enabled) was applied to the wrong VoiceEngine. The VoiceEngine in AudioConduit.cpp isn't used for audio capture and processing; it's the VoiceEngine in dom/media/webrtc/MediaEngineWebRTC* (which is also where the AEC/NS/etc prefs are handled). Net result was that landing bug 1226347 didn't enable the Delay-Agnostic AEC, and probably disabled the extended filter tail that was previously force-enabled. A very fast first quick test talking to a mac in an empty room shows much better AEC performance and stability especially on Mac. More testing TBD, and we should verify if the extended tail is worth the extra CPU use it implies, when combined with the Delay-Agnostic AEC. (Note: webrtc::Config must be scoped with similar lifetime to the VoiceEngine, since it's accessed after the engine is created; the original AudioConduit code is likely racy. Thus I made it a member of the MediaEngine.)
Attachment #8742674 -
Flags: review?(padenot)
Assignee | ||
Updated•8 years ago
|
Rank: 8
Updated•8 years ago
|
Attachment #8742674 -
Flags: review?(padenot) → review+
Comment 2•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bafad7114902
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
status-firefox45:
--- → wontfix
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8742674 [details] [diff] [review] Move AEC tail length and delay-agnostic settings to getUserMedia Approval Request Comment [Feature/regressing bug #]: bug 1226347 [User impact if declined]: Much worse AEC performance, especially on Mac [Describe test coverage new/current, TreeHerder]: AEC changes like this require manual testing on real hardware. We are working to get basic working/not-working AEC tests in, but that wouldn't cover this sort of thing well anyways. [Risks and why]: Low risk. Very simple change; moves the config parameter from one VoiceEngine to a different one. Required manual testing; I've been doing open-mic calls on different hardware with other devs with definitely improved quality, especially on Mac. [String/UUID change made/needed]: none
Attachment #8742674 -
Flags: approval-mozilla-aurora?
Comment on attachment 8742674 [details] [diff] [review] Move AEC tail length and delay-agnostic settings to getUserMedia Improves AEC experience on MAC, extensive manual testing, Aurora47+
Attachment #8742674 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 5•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0be14550a2b
You need to log in
before you can comment on or make changes to this bug.
Description
•