Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912523003:
Fix issue with send-side bandwidth estimation over TURN TCP connections. (Closed)
|
Created:
3 years, 6 months ago by Taylor Brandstetter Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix issue with send-side bandwidth estimation over TURN TCP connections.
AsyncStunTCPSocket wasn't firing SignalSentPacket, which the bandwidth
estimator requires for every packet in order to look up send times when
feedback arrives. If the signal isn't fired, it always assumes feedback
is arriving extremely late, and decreases the bandwidth by a factor of
2 until it reaches the minimum of 10kbps.
BUG=webrtc:7717
TBR=pthatcher@webrtc.org
Review-Url: https://codereview.webrtc.org/2912523003
Cr-Commit-Position: refs/heads/master@{#18279}
Committed: https://chromium.googlesource.com/external/webrtc/+/b56671e0517d4de773faeaa645af40840e863e79
Patch Set 1 #
Total comments: 3
Messages
Total messages: 16 (8 generated)
deadbeef@webrtc.org changed reviewers: + pthatcher@webrtc.org
PTAL
PTAL
Description was changed from ========== Fix issue with send-side bandwidth estimation over TURN TCP connections. AsyncStunTCPSocket wasn't firing SignalSentPacket, which the bandwidth estimator requires for every packet in order to look up send times when feedback arrives. If the signal isn't fired, it always assumes feedback is arriving extremely late, and decreases the bandwidth by a factor of 2 until it reaches the minimum of 10kbps. BUG=webrtc:7717 ========== to ========== Fix issue with send-side bandwidth estimation over TURN TCP connections. AsyncStunTCPSocket wasn't firing SignalSentPacket, which the bandwidth estimator requires for every packet in order to look up send times when feedback arrives. If the signal isn't fired, it always assumes feedback is arriving extremely late, and decreases the bandwidth by a factor of 2 until it reaches the minimum of 10kbps. BUG=webrtc:7717 TBR=pthatcher@webrtc.org ==========
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1495848140993210, "parent_rev":
"1f3fa0843bdb69be57afefa08b59e7ea2a74b42b", "commit_rev":
"b56671e0517d4de773faeaa645af40840e863e79"}
Message was sent while issue was closed.
Description was changed from ========== Fix issue with send-side bandwidth estimation over TURN TCP connections. AsyncStunTCPSocket wasn't firing SignalSentPacket, which the bandwidth estimator requires for every packet in order to look up send times when feedback arrives. If the signal isn't fired, it always assumes feedback is arriving extremely late, and decreases the bandwidth by a factor of 2 until it reaches the minimum of 10kbps. BUG=webrtc:7717 TBR=pthatcher@webrtc.org ========== to ========== Fix issue with send-side bandwidth estimation over TURN TCP connections. AsyncStunTCPSocket wasn't firing SignalSentPacket, which the bandwidth estimator requires for every packet in order to look up send times when feedback arrives. If the signal isn't fired, it always assumes feedback is arriving extremely late, and decreases the bandwidth by a factor of 2 until it reaches the minimum of 10kbps. BUG=webrtc:7717 TBR=pthatcher@webrtc.org Review-Url: https://codereview.webrtc.org/2912523003 Cr-Commit-Position: refs/heads/master@{#18279} Committed: https://chromium.googlesource.com/external/webrtc/+/b56671e0517d4de773faeaa64... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/b56671e0517d4de773faeaa64...
Message was sent while issue was closed.
juberti@webrtc.org changed reviewers: + juberti@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... File webrtc/p2p/base/asyncstuntcpsocket.cc (right): https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... webrtc/p2p/base/asyncstuntcpsocket.cc:82: rtc::SentPacket sent_packet(options.packet_id, rtc::TimeMillis()); This signal should probably only be fired when the packet has been fully serialized. That said, this change will still be an improvement.
Message was sent while issue was closed.
Description was changed from ========== Fix issue with send-side bandwidth estimation over TURN TCP connections. AsyncStunTCPSocket wasn't firing SignalSentPacket, which the bandwidth estimator requires for every packet in order to look up send times when feedback arrives. If the signal isn't fired, it always assumes feedback is arriving extremely late, and decreases the bandwidth by a factor of 2 until it reaches the minimum of 10kbps. BUG=webrtc:7717 TBR=pthatcher@webrtc.org Review-Url: https://codereview.webrtc.org/2912523003 Cr-Commit-Position: refs/heads/master@{#18279} Committed: https://chromium.googlesource.com/external/webrtc/+/b56671e0517d4de773faeaa64... ========== to ========== Fix issue with send-side bandwidth estimation over TURN TCP connections. AsyncStunTCPSocket wasn't firing SignalSentPacket, which the bandwidth estimator requires for every packet in order to look up send times when feedback arrives. If the signal isn't fired, it always assumes feedback is arriving extremely late, and decreases the bandwidth by a factor of 2 until it reaches the minimum of 10kbps. BUG=webrtc:7717 TBR=pthatcher@webrtc.org Review-Url: https://codereview.webrtc.org/2912523003 Cr-Commit-Position: refs/heads/master@{#18279} Committed: https://chromium.googlesource.com/external/webrtc/+/b56671e0517d4de773faeaa64... ==========
Message was sent while issue was closed.
On 2017/05/27 23:17:21, juberti1 wrote: > https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... > File webrtc/p2p/base/asyncstuntcpsocket.cc (right): > > https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... > webrtc/p2p/base/asyncstuntcpsocket.cc:82: rtc::SentPacket > sent_packet(options.packet_id, rtc::TimeMillis()); > This signal should probably only be fired when the packet has been fully > serialized. > > That said, this change will still be an improvement. Stefan, any input on when we should report the packet-sent signal when sending over TCP?
Message was sent while issue was closed.
holmer@google.com changed reviewers: + holmer@google.com
Message was sent while issue was closed.
https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... File webrtc/p2p/base/asyncstuntcpsocket.cc (right): https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... webrtc/p2p/base/asyncstuntcpsocket.cc:82: rtc::SentPacket sent_packet(options.packet_id, rtc::TimeMillis()); On 2017/05/27 23:17:21, juberti1 wrote: > This signal should probably only be fired when the packet has been fully > serialized. > > That said, this change will still be an improvement. Yes, I agree. If I correctly understand how the TCP socket classes interact, I'd say the signal should be fired in the implementation of socket_: https://cs.chromium.org/chromium/src/third_party/webrtc/base/asynctcpsocket.c... which should be where we send the packet to the real socket. When we run in Chrome, I assume that socket may be the IpcPacketSocket? In that case we really want the signal fired from the browser process, which maybe this signal now overrides? If I've understood correctly, I think we may want to connect to this signal instead: https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory....
Message was sent while issue was closed.
https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... File webrtc/p2p/base/asyncstuntcpsocket.cc (right): https://codereview.webrtc.org/2912523003/diff/1/webrtc/p2p/base/asyncstuntcps... webrtc/p2p/base/asyncstuntcpsocket.cc:82: rtc::SentPacket sent_packet(options.packet_id, rtc::TimeMillis()); On 2017/06/02 07:08:00, holmer wrote: > Yes, I agree. If I correctly understand how the TCP socket classes interact, I'd > say the signal should be fired in the implementation of socket_: > https://cs.chromium.org/chromium/src/third_party/webrtc/base/asynctcpsocket.c... > > which should be where we send the packet to the real socket. I guess I don't see how filling a buffer in userspace differs fundamentally from filling a buffer in kernel space. I thought the main purpose of SignalSentPacket was to eliminate jitter due to IPC between the browser/renderer processes. But anyway: this is copying the existing behavior of AsyncTcpSocket, so if this is an issue it's one that already existed. > When we run in Chrome, I assume that socket may be the IpcPacketSocket? In that > case we really want the signal fired from the browser process, which maybe this > signal now overrides? > > If I've understood correctly, I think we may want to connect to this signal > instead: > https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory.... In chromium, we connect to that signal; in native apps we connect to this one. This class is only created by BasicPacketSocketFactory, but chromium uses IpcPacketSocketFactory as you mentioned. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
