New issue
Advanced search Search tips

Issue 804263 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Echo saturations makes the echo alignment less accurate in AEC3

Project Member Reported by peah@chromium.org, Jan 22 2018

Issue description

When the echo is saturated the echo alignment sometimes has issues as the handling of saturation is not done correctly.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 22 2018

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

commit 700ef33edcb5c257e560e6f9153a3d154d80e75b
Author: Per Åhgren <peah@webrtc.org>
Date: Mon Jan 22 16:37:43 2018

Corrected the handling of saturation in the AEC3 alignment

Bug:  webrtc:8782 ,  chromium:804263 
Change-Id: I58660364f66959cc5bea3b081a626e743acedb1b
Reviewed-on: https://webrtc-review.googlesource.com/42581
Reviewed-by: Gustaf Ullberg <gustaf@webrtc.org>
Commit-Queue: Per Åhgren <peah@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21725}
[modify] https://crrev.com/700ef33edcb5c257e560e6f9153a3d154d80e75b/modules/audio_processing/aec3/matched_filter.cc

Comment 2 by pkl@chromium.org, Jan 22 2018

Status: Assigned (was: Untriaged)

Comment 3 by peah@chromium.org, Feb 8 2018

Labels: Merge-Request-65
Project Member

Comment 4 by sheriffbot@chromium.org, Feb 8 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: Less than 22 days to go before AppStore submit on M65
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Before we approve merge to M65, could you pls confirm followings?

Is this M65 regression and critical to merge?
Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge?
Any other imp details to justify the merge.

Please note M65 is already promoted to Beta so merge bar is very high. Thank you.

Comment 6 by peah@chromium.org, Feb 8 2018

This change has been very well tested in Canary and in manual testing.

The main motivation behind asking for a merge is that all the manual testing for the other AEC3 CLs that were merged/has a pending merge request to M65 for the purpose of reducing an increased sensitivity to audio pipeline issues was done including this change.  
For that reason we deem it would be safer to also merge this CL as the performance of the other CLs is better known if used together with this CL.
Labels: -Merge-Review-65 Merge-Approved-65
Approving merge to M65 branch 3325 based on comment #6.
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 9 2018

Labels: merge-merged-65
The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/e14cf5524a814dff00e40b450ae41f77c4378e4c

commit e14cf5524a814dff00e40b450ae41f77c4378e4c
Author: Per Åhgren <peah@webrtc.org>
Date: Fri Feb 09 07:06:15 2018

Merge to M65: Corrected the handling of saturation in the AEC3 alignment

TBR=henrik.lundin@webrtc.org,gustaf@webrtc.org
(cherry picked from commit 700ef33edcb5c257e560e6f9153a3d154d80e75b)

Bug:  webrtc:8782 ,  chromium:804263 
Change-Id: I58660364f66959cc5bea3b081a626e743acedb1b
Reviewed-on: https://webrtc-review.googlesource.com/42581
Reviewed-by: Gustaf Ullberg <gustaf@webrtc.org>
Commit-Queue: Per Åhgren <peah@webrtc.org>
Cr-Original-Commit-Position: refs/heads/master@{#21725}
Reviewed-on: https://webrtc-review.googlesource.com/50182
Reviewed-by: Per Åhgren <peah@webrtc.org>
Cr-Commit-Position: refs/branch-heads/65@{#15}
Cr-Branched-From: 3ac67a736bb200ecf7c116a88b2f8d5c542973c8-refs/heads/master@{#21637}
[modify] https://crrev.com/e14cf5524a814dff00e40b450ae41f77c4378e4c/modules/audio_processing/aec3/matched_filter.cc

Labels: -Merge-Approved-65
Per comment #8, this is already merged to M65.

Comment 10 by peah@chromium.org, Feb 19 2018

Status: Fixed (was: Assigned)

Sign in to add a comment