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

Issue 754159 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression

Blocked on:
issue webrtc:7885



Sign in to add a comment

Muting send audio may cause loud distortion

Project Member Reported by hlundin@chromium.org, Aug 10 2017

Issue description

Chrome Version: 61.0.3136.4
OS: All

What steps will reproduce the problem?
This was reported in detail in https://bugs.chromium.org/p/webrtc/issues/detail?id=7885.

This was a bug that was introduced in WebRTC, and first appeared in Chrome 61.0.3136.4.

A fix has already been committed to WebRTC (https://chromium-review.googlesource.com/c/593092), and was picked up in Chrome at 62.0.3176.0. The problem was that memset was used to zero out N bytes instead of N 2-byte samples.

I'd like to merge this fix to M61. I consider the fix safe, and it has been verified by the original reporter.

 
Project Member

Comment 1 by sheriffbot@chromium.org, Aug 10 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

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

Comment 3 by gov...@chromium.org, Aug 10 2017

Labels: -Type-Bug -Merge-Review-61 Merge-Approved-61 M-61 Type-Bug-Regression
Approving merge to M61 branch 3163 as this is M61 regression, change is baked/verified in Canary and safe to merge to M61 per original description of the bug.

Comment 4 by yujo@chromium.org, Aug 10 2017

Thanks for catching this, and sorry for overlooking it!
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 11 2017

Labels: merge-merged-61
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/a5903dc69eb7091f7a7b5cb76c827dbaed4a0b63

commit a5903dc69eb7091f7a7b5cb76c827dbaed4a0b63
Author: henrik.lundin <henrik.lundin@webrtc.org>
Date: Fri Aug 11 09:05:44 2017

[MERGE TO 61] Fix incorrect memset on muted frames.

Broken by https://codereview.webrtc.org/2750783004/. Since samples are
two bytes each, only half of the buffer was being zeroed, leading to
garbage noise.

BUG= webrtc:7885 , webrtc:7343 , chromium:754159 
NOTRY=true
NOPRESUBMIT=true

Change-Id: I46ecf90258b681ccdebbcfadd2e84ac6abadc9fe
Reviewed-on: https://chromium-review.googlesource.com/593092
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Jonathan Yu <yujo@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#19194}
Review-Url: https://codereview.webrtc.org/2997763002
Cr-Commit-Position: refs/branch-heads/61@{#3}
Cr-Branched-From: 83dc6b6f5380d7c03f5ebfe2553bf4a3147e2d1c-refs/heads/master@{#19063}

[modify] https://crrev.com/a5903dc69eb7091f7a7b5cb76c827dbaed4a0b63/webrtc/modules/audio_coding/acm2/audio_coding_module.cc

Labels: -Merge-Approved-61
Labels: MissingTests
Would it have been possible to catch this class of bugs with automated tests?
Ping
Yeah, it does look the fix doesn't have any tests in it. hlundin@, isn't it possible to add a unit test that covers this?

Comment 10 by yujo@chromium.org, Nov 6 2017

I can add the unit tests. I think the problem is, more generally, that non-DTX behaviors are never exercised anywhere, so bugs like this can go unnoticed.
Could you create a tracking bug for the tests or link the CL if it's already done here?
Ping :)
Ping
Labels: -MissingTests
Ok, we're giving up here.

Sign in to add a comment