New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 837563 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

The leakage behavior in some AEC3 FFT estimates do not match

Project Member Reported by peah@chromium.org, Apr 27 2018

Issue description

For legacy reasons, not all of the FFT estimates in AEC3 are computed the same. Recent changes in the code have removed the previous benefits of this, and even made that approach more complex. Apart from the complexity, there is a likely performance gain to be achived by unifying the way some of the FFT estimates are computed.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 27 2018

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/169c7fd521da7530ea55f9c4d4d045ccfd952e18

commit 169c7fd521da7530ea55f9c4d4d045ccfd952e18
Author: Per Åhgren <peah@webrtc.org>
Date: Fri Apr 27 14:47:56 2018

Use windowed, data padded, FFTs when computing the AEC3 suppressor gain

This CL changes the way the suppressor gain is computed in AEC3 in that
the FFTs used are padded with data and windowed with a Hanning-style
window.
This gives better FFT accuracy, an behavior matching the suppressor
gain application, and also results in one less FFT operation.

Bug:  webrtc:9204 , chromium:837563 
Change-Id: I612676c389cb76a3130966a9b596ff3f44d21863
Reviewed-on: https://webrtc-review.googlesource.com/73141
Reviewed-by: Gustaf Ullberg <gustaf@webrtc.org>
Commit-Queue: Per Åhgren <peah@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23057}
[modify] https://crrev.com/169c7fd521da7530ea55f9c4d4d045ccfd952e18/modules/audio_processing/aec3/echo_remover.cc
[modify] https://crrev.com/169c7fd521da7530ea55f9c4d4d045ccfd952e18/modules/audio_processing/aec3/subtractor.cc
[modify] https://crrev.com/169c7fd521da7530ea55f9c4d4d045ccfd952e18/modules/audio_processing/aec3/subtractor_output.h

Project Member

Comment 2 by bugdroid1@chromium.org, Apr 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4c679428c1b5e7cb7db8ec93626818e4f173cc1

commit a4c679428c1b5e7cb7db8ec93626818e4f173cc1
Author: webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Apr 27 19:21:00 2018

Roll src/third_party/webrtc/ 95141d91d..169c7fd52 (4 commits)

https://webrtc.googlesource.com/src.git/+log/95141d91d8ab..169c7fd521da

$ git log 95141d91d..169c7fd52 --date=short --no-merges --format='%ad %ae %s'

Created with:
  roll-dep src/third_party/webrtc
BUG= chromium:837563 


The AutoRoll server is located here: https://webrtc-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.mac:mac_chromium_archive_rel_ng
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: Iaa07e0d7ffe3f545cd5d2200e45ad5e3de4b3f43
Reviewed-on: https://chromium-review.googlesource.com/1033178
Commit-Queue: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#554443}
[modify] https://crrev.com/a4c679428c1b5e7cb7db8ec93626818e4f173cc1/DEPS

Comment 3 by peah@chromium.org, May 11 2018

Cc: gustaf@chromium.org huib@chromium.org hlundin@chromium.org
Labels: Merge-Request-67
We would like to merge this into M67. 

The reason for that is that the Canary testing of the four other merge requests (issues  841193 ,  841187 ,  840347  and  839860 ) (that address issues found during testing of the last M67 Beta) were done together with the CL for this issue.

The CL we would like to merge is:
-https://webrtc-review.googlesource.com/c/src/+/73141 (Use windowed, data padded, FFTs when computing the AEC3 suppressor gain)


The CL has been tested both in Canary and in offline testing and works well.

The merge is safe in the sense that the code affected is beneath an experimental flag. Furthermore, a field_trial switch was added that can be used to turn back on the effect of the headroom. 
Project Member

Comment 4 by sheriffbot@chromium.org, May 11 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 Deleted

Comment 6 by gov...@chromium.org, May 11 2018

Labels: -Merge-Review-67 Merge-Approved-67 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Approving mege to M67 branch 3396 based on https://bugs.chromium.org/p/chromium/issues/detail?id=841193#c6

Comment 7 by peah@chromium.org, May 12 2018

The CL was merged.
The merging CL was
-https://webrtc-review.googlesource.com/c/src/+/76320

Comment 8 by gov...@chromium.org, May 12 2018

Pls remove "Merge-Approved-67" label and apply "Merge-Merged-67" label if nothing else is pending for M67. Thank you.

Comment 9 by peah@chromium.org, May 12 2018

Labels: -Merge-Approved-67 merge-merged-67

Comment 10 by peah@chromium.org, May 12 2018

Labels: M-67

Comment 11 by peah@chromium.org, May 12 2018

Status: Fixed (was: Assigned)

Sign in to add a comment