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

Issue 622647 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Last visit 15 days ago
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression



Sign in to add a comment

WebRTC: Don't drop encoder config state

Project Member Reported by kwiberg@chromium.org, Jun 23 2016

Issue description

Since March 8th, WebRTC has erroneously been recreating AudioEncoder instances instead of reusing them. Besides being slightly (but not promlematically) wasteful, this causes the loss of encoder state; this was initially discovered because AudioEncoderOpus failed to remember its DTX setting.

This bug was merged into Chrome some days later, and was released in M51.

Here's the WebRTC bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=6020. Ideally, we'd like to backport the fix to M52 in addition to landing it in master.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 23 2016

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

commit 3f81fcd2e845505c2873c7ec2edade910213596b
Author: kwiberg <kwiberg@webrtc.org>
Date: Thu Jun 23 10:58:36 2016

Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG= webrtc:6020 ,  chromium:622647 

Review-Url: https://codereview.webrtc.org/2089183002
Cr-Commit-Position: refs/heads/master@{#13273}

[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/base/array_view.h
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/acm2/codec_manager.cc
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/acm2/codec_manager.h
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/codecs/audio_encoder.cc
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/codecs/audio_encoder.h
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc
[modify] https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h

Components: Blink>WebRTC>Audio
Labels: -Pri-3 M-51 M-53 M-52 OS-All Pri-1
Status: Fixed (was: Started)
The WebRTC fix (r13273) was imported into Chrome in https://crrev.com/432518763cb20a68128d4af3c162bafae1849291.
The fix was first picked up by Chrome branch 53.0.2778.0.

It has been in Win Canary since 2016-06-24; still waiting for other platforms to pick it up.
The fix landed in Mac Canary on 2016-06-27.
Cc: minyue@chromium.org
Labels: Merge-Request-52
The fix has been in Canary for 2 days or more. We have verified that it works as intended on Canary builds. General sanity metrics, such as average call duration, are unaffected.

We consider this fix verified and safe, and would like to have it merged to M52.

Comment 6 by dimu@google.com, Jun 29 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 29 2016

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

commit 15a5d9f9e3e4e8591c7a598c3818f42987cff522
Author: Henrik Lundin <henrik.lundin@webrtc.org>
Date: Wed Jun 29 08:27:14 2016

[MERGE TO M52] Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG= webrtc:6020 ,  chromium:622647 

Review-Url: https://codereview.webrtc.org/2089183002
Cr-Commit-Position: refs/heads/master@{#13273}
(cherry picked from commit 3f81fcd2e845505c2873c7ec2edade910213596b)

Review URL: https://codereview.webrtc.org/2105163003 .

Cr-Commit-Position: refs/branch-heads/52@{#8}
Cr-Branched-From: a376e70cf9d0df3c35d53533b454da542661775b-refs/heads/master@{#12798}

[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/base/array_view.h
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/acm2/codec_manager.cc
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/acm2/codec_manager.h
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/codecs/audio_encoder.cc
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/codecs/audio_encoder.h
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc
[modify] https://crrev.com/15a5d9f9e3e4e8591c7a598c3818f42987cff522/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h

Labels: -Merge-Approved-52
Labels: -M-53

Sign in to add a comment