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

Issue 738852 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.8% regression in webrtc_perf_tests at 18542:18542

Project Member Reported by olka@chromium.org, Jul 3 2017

Issue description

neteq_performance/10_pl_10_drift
 

Comment 1 by olka@chromium.org, Jul 3 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=738852

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgtpDh_ggM


Bot(s) for this bug's original alert(s):

webrtc-win-large-tests

Comment 2 by olka@chromium.org, Jul 3 2017

Owner: kwiberg@chromium.org
The range points to https://chromium.googlesource.com/external/webrtc/+/0703856b53e80159d666e289762406989f08ccfa

Not sure if it makes sense though.

kwiberg@ PTAL

Comment 3 by olka@chromium.org, Jul 3 2017

Cc: yujo@chromium.org
The next build is worse, associated with
https://chromium.googlesource.com/external/webrtc/+/36b1a5fcec6270ec4a5bea87b33a49b418a3cb29

Comment 4 by yujo@chromium.org, Jul 6 2017

Investigating. This is probably caused by frames being muted and then unmuted, causing a memset to 0. Prior to my change, the caller may have skipped the zeroing if it knew it was going to overwrite the buffer anyway.

Given the age of the change, is it reasonable to fix forward rather than rolling back?

Comment 5 by olka@chromium.org, Jul 6 2017

Depending on how easy is a fix and how risky is a rollback.
If nothing is built on top of this CL and if it does not contain important fixes itself, then it is reasonable to roll back. Regression in  Issue 738839  is quite big.
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/3ffa72d0f03b75812c0ce367c1958ff9a80a6aeb

commit 3ffa72d0f03b75812c0ce367c1958ff9a80a6aeb
Author: Jonathan Yu <yujo@chromium.org>
Date: Sat Jul 08 23:36:26 2017

Add AudioFrame::ResetWithoutMuting() to address performance regression.

Prior to https://codereview.webrtc.org/2750783004/ Reset() intentionally
did not zero out the buffer. After that change, callers calling Reset()
and then mutable_data() were performing a wasteful zeroing.

This change adds ResetWithoutMuting() to match the old behavior and
switches the sole non-test caller of Reset() to use ResetWithoutMuting()
instead.

Prior to this change (optimized, Linux):
$ out/Default/webrtc_perf_tests --gtest_filter=NetEqPerformanceTest.Run* \
  --gtest_repeat=10 | grep neteq_performance
*RESULT neteq_performance: 10_pl_10_drift= 4051 ms
*RESULT neteq_performance: 0_pl_0_drift= 1768 ms
*RESULT neteq_performance: 10_pl_10_drift= 3666 ms
*RESULT neteq_performance: 0_pl_0_drift= 1690 ms
*RESULT neteq_performance: 10_pl_10_drift= 3685 ms
*RESULT neteq_performance: 0_pl_0_drift= 1693 ms
*RESULT neteq_performance: 10_pl_10_drift= 3720 ms
*RESULT neteq_performance: 0_pl_0_drift= 1690 ms
*RESULT neteq_performance: 10_pl_10_drift= 3780 ms
*RESULT neteq_performance: 0_pl_0_drift= 1728 ms
*RESULT neteq_performance: 10_pl_10_drift= 3733 ms
*RESULT neteq_performance: 0_pl_0_drift= 1737 ms
*RESULT neteq_performance: 10_pl_10_drift= 3781 ms
*RESULT neteq_performance: 0_pl_0_drift= 1744 ms
*RESULT neteq_performance: 10_pl_10_drift= 3712 ms
*RESULT neteq_performance: 0_pl_0_drift= 1731 ms
*RESULT neteq_performance: 10_pl_10_drift= 3681 ms
*RESULT neteq_performance: 0_pl_0_drift= 1691 ms
*RESULT neteq_performance: 10_pl_10_drift= 3681 ms
*RESULT neteq_performance: 0_pl_0_drift= 1690 ms

With this change:
$ out/Default/webrtc_perf_tests --gtest_filter=NetEqPerformanceTest.Run* \
  --gtest_repeat=10 | grep neteq_performance
*RESULT neteq_performance: 10_pl_10_drift= 3824 ms
*RESULT neteq_performance: 0_pl_0_drift= 1632 ms
*RESULT neteq_performance: 10_pl_10_drift= 3502 ms
*RESULT neteq_performance: 0_pl_0_drift= 1521 ms
*RESULT neteq_performance: 10_pl_10_drift= 3520 ms
*RESULT neteq_performance: 0_pl_0_drift= 1534 ms
*RESULT neteq_performance: 10_pl_10_drift= 3517 ms
*RESULT neteq_performance: 0_pl_0_drift= 1530 ms
*RESULT neteq_performance: 10_pl_10_drift= 3521 ms
*RESULT neteq_performance: 0_pl_0_drift= 1527 ms
*RESULT neteq_performance: 10_pl_10_drift= 3511 ms
*RESULT neteq_performance: 0_pl_0_drift= 1533 ms
*RESULT neteq_performance: 10_pl_10_drift= 3518 ms
*RESULT neteq_performance: 0_pl_0_drift= 1523 ms
*RESULT neteq_performance: 10_pl_10_drift= 3503 ms
*RESULT neteq_performance: 0_pl_0_drift= 1524 ms
*RESULT neteq_performance: 10_pl_10_drift= 3514 ms
*RESULT neteq_performance: 0_pl_0_drift= 1534 ms
*RESULT neteq_performance: 10_pl_10_drift= 3501 ms
*RESULT neteq_performance: 0_pl_0_drift= 1530 ms

BUG= webrtc:7343 , chromium:738852 , chromium:738839 

Change-Id: Idcbb276ca0ed27fff95164a73f1c1fa310175ee5
Reviewed-on: https://chromium-review.googlesource.com/563021
Reviewed-by: Henrik Lundin <henrik.lundin@webrtc.org>
Reviewed-by: Olga Sharonova <olka@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#18939}
[modify] https://crrev.com/3ffa72d0f03b75812c0ce367c1958ff9a80a6aeb/webrtc/modules/audio_coding/neteq/sync_buffer.cc
[modify] https://crrev.com/3ffa72d0f03b75812c0ce367c1958ff9a80a6aeb/webrtc/modules/include/module_common_types.h

Comment 7 by yujo@chromium.org, Jul 11 2017

Cc: kwiberg@chromium.org
Owner: yujo@chromium.org
Status: Fixed (was: Untriaged)
Graphs are back down to normal, marking fixed.

Sign in to add a comment