Closed Bug 1226347 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
Blocking Flags:

People

(Reporter: jesup, Assigned: pkerr)

References

Details

Attachments

(3 files, 7 obsolete files)

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
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
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
now includes mobile (aecm) changes; largely just API changes to match
Attachment #8689719 - Attachment is obsolete: true
Attached patch Part 2: WIP (obsolete) — Splinter Review
Assignee: rjesup → pkerr
backlog: --- → webrtc/webaudio+
Rank: 10
Priority: -- → P1
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.
Attachment #8690858 - Flags: review?(pkerr)
Attachment #8689744 - Attachment is obsolete: true
Assignee: pkerr → rjesup
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
Attachment #8690862 - Flags: review?(rjesup)
Attachment #8689866 - Attachment is obsolete: true
Attachment #8690862 - Flags: review?(rjesup) → review+
udpate to work in progress
Attachment #8690862 - Attachment is obsolete: true
Assignee: rjesup → pkerr
Attachment #8690901 - Attachment is obsolete: true
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?
Attachment #8690858 - Flags: review?(pkerr) → review+
Attachment #8690989 - Attachment is obsolete: true
Attachment #8691038 - Flags: review?(rjesup)
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
Attachment #8691038 - Flags: review?(rjesup) → review-
Attachment #8691038 - Attachment is obsolete: true
Attachment #8691232 - Flags: review?(rjesup)
Attachment #8691232 - Flags: review?(rjesup) → review+
Flags: needinfo?(pkerr)
https://hg.mozilla.org/mozilla-central/rev/2d383dde4c3d
https://hg.mozilla.org/mozilla-central/rev/e212333b735a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8693651 [details] [diff] [review]
Part 3: AEC agnostic delay pref true by default

r+ from rjesup via #media.
Attachment #8693651 - Flags: review+
Depends on: 1265641
You need to log in before you can comment on or make changes to this bug.