Closed Bug 930603 Opened 6 years ago Closed 6 years ago

Cherry-pick webrtc.org AEC tail increase to 128ms

Categories

(Core :: WebRTC: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jesup, Assigned: jesup)

Details

(Whiteboard: [webrtc])

Attachments

(2 files)

Cherry-pick http://review.webrtc.org/2151007/
and https://webrtc-codereview.appspot.com/2328005

These bump the AEC tail length to 128ms from 48, and also makes the delay calculations more conservative (though that won't affect our usage).

We basically never use the delay values (given where the AEC is in our chain), so output chain instability will cause the AEC to have to re-adapt without being told there was a delay change, and performance in that situation will be sub-optimal, as it will be if the output delay to the speakers is too long (beyond the 128ms tail).  We route the input to the peerconnection on a fast-path (bug 844354) and measurements show that's stable and fast at least on desktop platforms (it likely is stable and slow on Android).

I'm ignoring the kludge to re-use the DelayCorrection for turning this on and off, and instead have wired it on all the time.

Note I don't think this will affect AECM (mobile AEC), which should be used on Android/B2G.

I've run this in local calls, and a 50-minute remote call (Linux 27+this to FF 24 on Mac).  The other side heard no echo at any time (so my AEC must have been working reasonably well).
Comment on attachment 821765 [details] [diff] [review]
Increase WebRTC AEC tail from 48ms to 128ms (rev 4837 at webrtc.org)

The only change other than paths is hardwiring the 128ms mode on (and ifdefing out the SetExtraOptions() that uses a post 3.34 typedef).  Also, the NR_PART and PREF_BAND_SIZE had moved from aec_core.h to aec_core_internal.h between 3.34 and when this patch landed.

I'm not really looking for an "is this done right" signal-processing-wise (though that'd be interesting) - we're importing this patch from upstream, just a little ahead of when we'd normally get it.  Mostly this is a sanity-check for an import.  Upstream is running this in Beta IIRC, and has been getting good reports.

And it doesn't change the fact that the AEC is in the wrong place (peerconnection), and has no delay-change feedback from the MSG/backend; those will be addressed in the AEC move coming up.
Attachment #821765 - Flags: review?(jmvalin)
Comment on attachment 821766 [details] [diff] [review]
Ensure AEC known delay doesn't go negative (rev 4886 at webrtc.org)

No changes from upstream; a rubber-stamp for sanity is all I'm looking for here.  This just tries to make sure it doesn't use negative delays (see upstream landing comments)
Attachment #821766 - Flags: review?(jmvalin)
Whiteboard: [webrtc]
Attachment #821765 - Flags: review?(jmvalin)
Attachment #821766 - Flags: review?(jmvalin)
Attachment #821765 - Flags: review?(jib)
Attachment #821766 - Flags: review?(jib)
Comment on attachment 821765 [details] [diff] [review]
Increase WebRTC AEC tail from 48ms to 128ms (rev 4837 at webrtc.org)

Review of attachment 821765 [details] [diff] [review]:
-----------------------------------------------------------------

Update looks sane compared to upstream.
Attachment #821765 - Flags: review?(jib) → review+
Comment on attachment 821766 [details] [diff] [review]
Ensure AEC known delay doesn't go negative (rev 4886 at webrtc.org)

Review of attachment 821766 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.
Attachment #821766 - Flags: review?(jib) → review+
You need to log in before you can comment on or make changes to this bug.