Closed Bug 1265641 Opened 8 years ago Closed 8 years ago

Delay-agnostic pref applied to wrong VoiceEngine

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
firefox-esr38 --- unaffected
firefox-esr45 --- affected

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(1 file)

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)
Rank: 8
Attachment #8742674 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/bafad7114902
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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+
You need to log in before you can comment on or make changes to this bug.