Issue metadata
Sign in to add a comment
|
Send-side bandwidth estimation not working over TCP connections |
||||||||||||||||||||||
Issue descriptionChrome Version: (copy from chrome://version) OS: (e.g. Win7, OSX 10.9.5, etc...) What steps will reproduce the problem? (1) Force TCP TURN. For example, by going to https://appr.tc/?tt=tcp&it=relay (2) Connect to an endpoint (such as another instance of Chrome) that supports the transport-cc RTCP extension, needed for send-side bandwidth estimation. What is the expected result? Decent video quality. What happens instead? Terrible video quality. Bandwidth estimator ramps down to 10kbps. Creating this bug to request a merge to M60. This is a high-impact regression, since TURN over TCP will typically be used whenever UDP is blocked (for example, by a corporate firewall). Parallel webrtc bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=7717
,
May 30 2017
This will need to be applied well before release of 60 or lots of very good work in enterprise and institutional sectors will be ruined. Do you really propose that we ask all of these users to make UDP egress network change requests, that require change board approval, just because of this regression. I repeat, this is a serious degradation of performance for a huge user population outside of home and the smallest of businesses. It effectively makes all but very low res/rate video unusable.
,
May 30 2017
I agree about the severity of the issue. I'll try requesting a merge to 58 (which is where the issue really got bad, going from 300kbps to 10kbps). In summary: about 1% of WebRTC connections (mostly enterprise users, I assume) are made in network environments that require the use of TCP. Out of these connections, every one that's made to another instance of Chrome, or to a different WebRTC endpoint that supports send-side bandwidth estimation, will result in an unusable level of video quality. I'd estimate this means around 0.5% of WebRTC connections are affected, or at least some number in that ballpark. Given the severity of the issue, and the fact that it's a straightforward two-line fix, merging as far back as M58 (or even M57) doesn't seem unreasonable to me.
,
May 31 2017
Can this be added in 58 as a hotfix? I have developed a webrtc test for multiple enterprise clients and this bug is producing all sorts of troubles for me.
,
May 31 2017
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
,
May 31 2017
Correction: the originally linked webrtc issue (7717) actually doesn't impact chromium. But these issues do: https://bugs.chromium.org/p/webrtc/issues/detail?id=7717 (affects direct TCP connections) https://bugs.chromium.org/p/chromium/issues/detail?id=715099 (affects all TCP connections) Fixed by these CLs: https://codereview.webrtc.org/2834083002 https://codereview.chromium.org/2841803003
,
Jun 1 2017
We are not planning any further M58 stable releases. Hence, rejecting merge to M58. Please request a merge to M59 and M60 if needed. Also pls apply applicable OSs label. Thank you.
,
Jun 1 2017
Ok. Requesting merge to M59 in that case.
,
Jun 1 2017
This bug requires manual review: We are only 4 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 1 2017
For M59, we are targeting a stable release next week and bar for merges is very high. Have you tested and verified the fix in Canary or Dev? Is this an overall safe merge, with well automated test coverage? What are the actual changes that you are requesting a merge for?
,
Jun 1 2017
https://codereview.chromium.org/2841803003 looks pretty big to be taking this late in the game (assuming that's what is required for the merge). That said, 25 stars on an issue filed 5 days ago is huge :\ When did this regress? M58, or previously?
,
Jun 1 2017
The CLs that need to be merged are: https://codereview.webrtc.org/2834083002 https://codereview.chromium.org/2841803003 It actually originally regressed in M57. Though the behavior got significantly worse in M58, with the bitrate going from 300kbps to 10kbps. Which is the difference between the video just being slightly fuzzy, and it having a noticeably low framerate/high latency.
,
Jun 1 2017
Seems M58, I got some data in hindsight which show the impact: https://cdn-images-1.medium.com/max/2000/1*2BI3VfKNITSp8_NBXx2KUg.png (the send bitrate was significantly lower in M58 for connections that use TURN/TCP; full story in https://medium.com/the-making-of-appear-in/working-around-turn-bugs-495a058c2436) I would estimate the impact a bit higher than Taylor even, 1-2% of calls on appear.in use TURN/TCP (or TURN/TLS) and have therefore been affected (before the workaround was deployed). That ratio varies with the usage scenario and will be higher with users from enterprise environments.
,
Jun 1 2017
deadbeef@ - have you confirmed and verified your fix in Canary/Dev?
,
Jun 1 2017
,
Jun 1 2017
,
Jun 2 2017
Yes, verified in Canary/Dev.
,
Jun 2 2017
thanks - looking at https://codereview.webrtc.org/2834083002 https://codereview.chromium.org/2841803003, these are quite medium to large size patches. Are you fairly confident that these will be safe merges?
,
Jun 2 2017
The WebRTC change is pretty trivial; it connects a callback that was already connected for incoming connections, but not outgoing. And the chromium change in essence just adds an ID field to a structure being passed around, and adds this ID to a callback. In short, both of these changes are relatively small, and add functionality that other subclasses of the same base class are already doing. So I'm pretty confident they'll be safe merges.
,
Jun 2 2017
Ok great thanks. Based on comment 12, 17,19, approving merge for M59.
,
Jun 2 2017
M59 branch is 3071.
,
Jun 2 2017
I just realized I mixed up which bugs affect chromium and still need to be merged. Let me try to re-summarize: There are three places where a "packet sent" callback wasn't hooked up, causing bandwidth estimation to not work starting in M57 (and getting worse in M58): 1. Issue that only affects chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=715099 2. Issue that only affects non-TURN TCP connections: https://bugs.chromium.org/p/webrtc/issues/detail?id=7509 3. Issue that only affects TURN TCP connections, and only for native apps (not chromium): https://bugs.chromium.org/p/webrtc/issues/detail?id=7717 #1 is fixed in M59; #2 and #3 are fixed in M60. So, the only thing that still needs to be merged to M59 is the webrtc CL, https://codereview.webrtc.org/2834083002. And this only affects direct TCP connections; TCP connections over TURN work ok in M59. So the severity is lower than I initially thought (for M59), though I still believe it's worth fixing and safe to fix. So unless I hear something new, I'll go ahead with merging it.
,
Jun 2 2017
We have lots of external folks with this issue starred, so maybe we should ask them - has anyone tried M59 beta, and is performance acceptable there? If the answer is yes, then we should ask ourselves if this is absolutely necessary (and if it's no then we can still consider the merge, it'd just be good to have this data).
,
Jun 3 2017
,
Jun 4 2017
Looks like change (#2834083002) has already been merged to WebRTC for M59 branch on Friday. I am fine with taking the change; however based on new insights in #22, can someone test this in M59 Beta to confirm if the quality is substantially better?
,
Jun 5 2017
The latest 59 beta doesn't have the fix as it was cut before the fix landed in 59. I built Chrome branch 3071, manually going into third_party/webrtc to move it to the tip of WebRTC's 59 branch (currently Chrome's 3071 DEPS file points to an earlier WebRTC commit). With this I was able to verify the fix, and as expected the quality improvement is substantial. abdulsyed, is there anything else we need to do to get DEPS for m59 (3071) updated to point to the latest webrtc 59 commit (https://chromium.googlesource.com/external/webrtc/+log/branch-heads/59), or will that happen automatically?
,
Jun 5 2017
Since it was merged on Friday, this was missed for Beta. However, the fix should be in Friday night's build, which is our release candidate for today. Thanks for confirming and verifying it manually.
,
Jun 5 2017
Marking this as fixed since it's been merged to all applicable platforms. Please reopen if further work is required.
,
Jun 15 2017
Here's a brief timeline of this issue, for anyone who needs to know which version(s) affect their application: M57 - Issue introduced. Affects all TCP connections and results in bandwidth of 300kbps. M58 - Issue got worse; bandwidth now may go down to 10kbps. M59 - Issue fixed for chromium. Still affects TCP TURN connections in native applications. M60 - Issue fixed for everyone (as far as we know). |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by sheriffbot@chromium.org
, May 28 2017