1226347 - Cherry-pick Delay-Agnostic AEC from later upstream versions of webrtc

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

Webrtc.org 43 has some support for the delay-agnostic AEC, but not all.  45 or 46 and later should have all the changes; this is to evaluate and possibly land a cherry-pick of post-43 changes so we can use the Delay-agnostic AEC

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Import cherry-pick of AEC changes from 43->48 (delay-agnostic AEC) - WIP

WIP, but builds and seems to work to a VERY coarse first test.  Hard-wired on for now (see XXX's).  Will need a pref to flip it on/off.  Also stubbed out the FFT log, because I don't think we care right now (mostly for the guys modifying the algorithm

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Import cherry-pick of AEC changes from 43->48 (delay-agnostic AEC) - WIP

now includes mobile (aecm) changes; largely just API changes to match

Comment 3

[Paul Kerr [:pkerr]]

Attachment: Part 2: WIP

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Import cherry-pick of AEC changes from 43->48 (delay-agnostic AEC)

will be merged with the pref update in patch 2 for landing.  Added support for the ftp dump file; we may need to revise the aec log upload code to include that.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Part 2: hook up prefs for aec options

r+, with some minor whitespace cleanup, changing the default to extended=true (which is what we have today), and adding them to all.js so they show in about:config

Comment 6

[Paul Kerr [:pkerr]]

Attachment: Part 2: Allow control of AEC via prefs

udpate to work in progress

Comment 7

[Paul Kerr [:pkerr]]

Attachment: Part 2: Allow control of AEC via prefs

Comment 8

[Paul Kerr [:pkerr]]

Comment on attachment 8690858 [details] [diff] [review]
Import cherry-pick of AEC changes from 43->48 (delay-agnostic AEC)

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

::: media/webrtc/trunk/webrtc/modules/audio_processing/aec/aec_core.c
@@ +1624,5 @@
>      rtc_WavClose(aec->farFile);
>      rtc_WavClose(aec->nearFile);
>      rtc_WavClose(aec->outFile);
>      rtc_WavClose(aec->outLinearFile);
> +    rtc_WavClose(aec->e_fft_file);

Is this a new file that will be generated when aec debug is enabled via the about:webrtc page?

Comment 9

[Paul Kerr [:pkerr]]

Attachment: Part 2: Allow control of AEC via prefs

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8691038 [details] [diff] [review]
Part 2: Allow control of AEC via prefs

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

Per IRC, we don't need to touch all these files.  ALso we need to modify all.js so they're visible in about:config.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +233,5 @@
>        return kMediaConduitSessionNotInited;
>      }
>  #endif
>  
> +  bool aec_extended_filter = false;

We've been defaulting to 'true' for the extended filter for a long time (they did rename the var in the delay-agnostic changes; see patch 1).

@@ +234,5 @@
>      }
>  #endif
>  
> +  bool aec_extended_filter = false;
> +  bool aec_delay_agnostic = false;

Google defaults this to true for mobile I believe (though for Chrome, I think they're using true for all platforms now)

::: media/webrtc/trunk/webrtc/modules/audio_processing/include/audio_processing.h
@@ +41,5 @@
> +  ExtendedFilter() : enabled(false) {}
> +  explicit ExtendedFilter(bool enabled) : enabled(enabled) {}
> +  bool enabled;
> +};
> + 

trailing spaces

Comment 11

[Paul Kerr [:pkerr]]

Attachment: Part 2: Allow control of AEC via prefs

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63d13b6b43ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/da34ad5d6957

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=17756755&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/897bd0c1180b

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d383dde4c3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e212333b735a

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d383dde4c3d
https://hg.mozilla.org/mozilla-central/rev/e212333b735a

Comment 16

[Paul Kerr [:pkerr]]

Attachment: Part 3: AEC agnostic delay pref true by default

Comment 17

[Paul Kerr [:pkerr]]

Comment on attachment 8693651 [details] [diff] [review]
Part 3: AEC agnostic delay pref true by default

r+ from rjesup via #media.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5af2a9aec9d

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5af2a9aec9d