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

Issue 628084 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Merge fix for resetting BWE to zero when a connection switches

Project Member Reported by honghaiz@chromium.org, Jul 14 2016

Issue description

Bug details are copied from https://bugs.chromium.org/p/webrtc/issues/detail?id=6076

In https://codereview.webrtc.org/2094863003/, we reset the BWE bitrate when there is a connection switch. But it resets the BWE bitrate to zero unexpectedly.
 
This happens because BitrateControllerImpl::ResetBitrates() ignores sets -1 as start bitrate on a newly created SendSideBandwidthEstimation, which means it will keep its default value of zero.

Fix: copy the start bitrate only if it is positive. If it is -1, there is no point remembering it. 
Fixing CL: https://codereview.webrtc.org/2125523004

I would like to merge that CL into WebRTC's M53 branch. 
Justification for M53 merge: 
1. This is a regression compared to M52. 
2. Fixing this is important because it may make the video sending rate in WebRTC call lower than expected values. 

 
Components: Blink>WebRTC>Video
Labels: -Type-Bug Merge-Request-53 M-53 OS-All Type-Bug-Regression

Comment 2 by gov...@chromium.org, Jul 14 2016

Before we approve merge to M53, Could you please confirm whether this change is baked/verified in Canary OR on ToT and safe to merge?

Comment 3 by dimu@google.com, Jul 15 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Thanks!
The issue was detected in mobile apps and the fix was verified there and we think it will also affect chrome M53.
It is only triggered under certain conditions (max_bitrate is set and a network switch), and it is not straightforward to reproduce it. 
Cc: tnakamura@chromium.org
I have landed the merging CL here
https://chromium.googlesource.com/external/webrtc/+/0c54eb178003a7af9f275429205fea483c1d2578
and verified it is in webrtc branch 53 here: 
https://chromium.googlesource.com/external/webrtc/+log/branch-heads/53

Can someone verify it will be picked by Chrome 53?

Comment 6 by gov...@chromium.org, Jul 15 2016

Cc: tommi@chromium.org
+tommi@, could you ptal comment #5. Thank you.

Comment 7 by tommi@chromium.org, Jul 15 2016

Looks like everything is in place. Holmer can you double check? 

Comment 8 by gov...@chromium.org, Jul 15 2016

Please merge your change to M53 branch 2785 ASAP (latest by 4:00 PM PST on Monday, 07/18) in order to make it to M53 dev release next week before Beta promotion.
I have merged the CL to webrtc branch 53. 
Do I need to do anything extra to merge the change to M53 branch 2785?
If change is merged to  webrtc branch 53, please remove "Merge-Approved-53" label and apply "merge-merged-53" label. I don't see any update from  bugdroid1@ in this bug, so please do make sure that the change is merged. Thank you.
Project Member

Comment 11 by sheriffbot@chromium.org, Jul 18 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-53 Merge-Merged
Sorry the landed CL was not linked to this issue probably because I did not prefix  chromium when referring to the bug in the CL. 
The following CL refers to this bug:
https://chromium.googlesource.com/external/webrtc/+/0c54eb178003a7af9f275429205fea483c1d2578
commit	0c54eb178003a7af9f275429205fea483c1d2578	
author	Honghai Zhang <honghaiz@webrtc.org>	
Date Fri Jul 15 15:22:57 2016

Fix bug where a connection switch causes BWE to be set to zero.

BUG= webrtc:6076 ,  628084 
R=sprang@webrtc.org,stefan@webrtc.org
TBR=tommi@webrtc.org

patch from issue:
Review URL: https://codereview.webrtc.org/2125523004 .
(cherry picked from commit be40296ccef50dd79312c6fce533bfa36b7063dc)

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

Cr-Original-Commit-Position: refs/branch-heads/53@{#3}
Cr-Original-Branched-From: 65f47275cbb5ad3aa5146b9e75afa9894467daff-refs/heads/master@{#13317}
Cr-Commit-Position: refs/branch-heads/53@{#3}
Cr-Branched-From: 65f47275cbb5ad3aa5146b9e75afa9894467daff-refs/heads/master@{#13317}
Status: Fixed (was: Assigned)
Labels: -Merge-Merged Merge-Merged-53
holmer@, can you double check to make sure it will be included in Chrome-53?

Sign in to add a comment