Delay-agnostic pref applied to wrong VoiceEngine

RESOLVED FIXED in Firefox 47

Status

()

Core
WebRTC
P1
critical
Rank:
8
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47 fixed, firefox48 fixed, firefox-esr38 unaffected, firefox-esr45 affected)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8742674 [details] [diff] [review]
Move AEC tail length and delay-agnostic settings to getUserMedia

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

2 years ago
Rank: 8

Updated

2 years ago
Attachment #8742674 - Flags: review?(padenot) → review+

Comment 1

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bafad7114902

Comment 2

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bafad7114902
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
status-firefox45: --- → wontfix
status-firefox46: --- → wontfix
status-firefox47: --- → affected
status-firefox-esr38: --- → unaffected
status-firefox-esr45: --- → affected
(Assignee)

Comment 3

2 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 4

2 years ago
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

2 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0be14550a2b
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.