Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1345583004:
Wire up transport sequence number and send time. (Closed)
|
Created:
5 years, 3 months ago by Stefan Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWire up transport sequence number and send time.
This is used by WebRTC congestion control to know which packet was sent at what time.
BUG=webrtc:4173
Committed: https://crrev.com/0fbb02dc39e4da03f5901f5b6ef7f3d15c1cbd22
Cr-Commit-Position: refs/heads/master@{#355256}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comments addressed. #
Total comments: 13
Patch Set 3 : Comments addressed. #Patch Set 4 : Matching changes in .h file. #
Total comments: 4
Patch Set 5 : Addressed final comments. #Patch Set 6 : Addressed bug found during testing. #Patch Set 7 : Address change in libjingle #
Total comments: 3
Messages
Total messages: 42 (14 generated)
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/1345583004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/p2p/socket_host_udp.h (right): https://codereview.chromium.org/1345583004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/p2p/socket_host_udp.h:73: void OnSend(uint64 packet_id, please change this to uint64_t for consistency. https://codereview.chromium.org/1345583004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/p2p/socket_host_udp.h:75: uint64 tick_received, Use base::TimeTicks here instead of uint64? https://codereview.chromium.org/1345583004/diff/1/content/common/p2p_socket_t... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/1345583004/diff/1/content/common/p2p_socket_t... content/common/p2p_socket_type.h:56: : packet_id(0), transport_sequence_number(-1), send_time_ms(-1) {} add default initializers where the members are defined. https://codereview.chromium.org/1345583004/diff/1/content/common/p2p_socket_t... content/common/p2p_socket_type.h:59: int64_t send_time_ms) use base::TimeTicks?
https://codereview.chromium.org/1345583004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/p2p/socket_host_udp.h (right): https://codereview.chromium.org/1345583004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/p2p/socket_host_udp.h:73: void OnSend(uint64 packet_id, On 2015/09/15 23:38:40, Sergey Ulanov wrote: > please change this to uint64_t for consistency. Done. https://codereview.chromium.org/1345583004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/p2p/socket_host_udp.h:75: uint64 tick_received, On 2015/09/15 23:38:40, Sergey Ulanov wrote: > Use base::TimeTicks here instead of uint64? Done. https://codereview.chromium.org/1345583004/diff/1/content/common/p2p_socket_t... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/1345583004/diff/1/content/common/p2p_socket_t... content/common/p2p_socket_type.h:56: : packet_id(0), transport_sequence_number(-1), send_time_ms(-1) {} On 2015/09/15 23:38:40, Sergey Ulanov wrote: > add default initializers where the members are defined. Done. https://codereview.chromium.org/1345583004/diff/1/content/common/p2p_socket_t... content/common/p2p_socket_type.h:59: int64_t send_time_ms) On 2015/09/15 23:38:40, Sergey Ulanov wrote: > use base::TimeTicks? Done.
holmer@chromium.org changed reviewers: + wfh@chromium.org
+wfh for changes to content/common/p2p_socket_type.h
I am not an owner on content/common/p2p_socket_type.h, sorry
wfh@chromium.org changed reviewers: - wfh@chromium.org
https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:294: base::TimeTicks tick_received = base::TimeTicks::Now(); nit: call it time_received? https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:310: base::Bind(&P2PSocketHostUdp::OnSend, base::Unretained(this), packet.id, This duplicates the code above. Maybe store the result of base::Bind() and use it in both SendTo() invocations. https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:312: tick_received)); So you assign tick_received to sent_time_tick. There may be some delay caused by IPC, i.e. this field is not going to reflect when the renderer is actually tried to to send this packet. Is that what you want? https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:346: void P2PSocketHostUdp::HandleSendResult(uint64 packet_id, Please change this to uint64_t https://codereview.chromium.org/1345583004/diff/20001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/1345583004/diff/20001/content/common/p2p_sock... content/common/p2p_socket_type.h:65: int32_t transport_sequence_number = -1; It's not clear what transport_sequence_number is and how it's different from packet_id. Add some comments here? https://codereview.chromium.org/1345583004/diff/20001/content/common/p2p_sock... content/common/p2p_socket_type.h:66: base::TimeTicks send_time_ticks; nit: suggest calling it |send_time|, i.e. remove _ticks suffix.
https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:294: base::TimeTicks tick_received = base::TimeTicks::Now(); On 2015/09/17 17:19:31, Sergey Ulanov wrote: > nit: call it time_received? Done. https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:310: base::Bind(&P2PSocketHostUdp::OnSend, base::Unretained(this), packet.id, On 2015/09/17 17:19:31, Sergey Ulanov wrote: > This duplicates the code above. Maybe store the result of base::Bind() and use > it in both SendTo() invocations. Done. https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:312: tick_received)); On 2015/09/17 17:19:31, Sergey Ulanov wrote: > So you assign tick_received to sent_time_tick. There may be some delay caused by > IPC, i.e. this field is not going to reflect when the renderer is actually tried > to to send this packet. Is that what you want? I want this to represent the time when the packet hits the wire. My understanding is that tick_received here is taken immediately before we call SendTo() on the socket, which is about as close as we can get. So if I understand this correctly (and please correct me if I'm wrong!), this is running in the browser process, and I'm sending back this timestamp to the render process through P2PMsg_OnSendComplete below. If this is the case, then this is the timestamp I'm interested in. We don't want the timestamp when the renderer sends the packet, as we are measuring the time the packet spends on the internet, and not the time it spends in the browser. https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:346: void P2PSocketHostUdp::HandleSendResult(uint64 packet_id, On 2015/09/17 17:19:31, Sergey Ulanov wrote: > Please change this to uint64_t Done. https://codereview.chromium.org/1345583004/diff/20001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/1345583004/diff/20001/content/common/p2p_sock... content/common/p2p_socket_type.h:65: int32_t transport_sequence_number = -1; On 2015/09/17 17:19:31, Sergey Ulanov wrote: > It's not clear what transport_sequence_number is and how it's different from > packet_id. Add some comments here? Done. https://codereview.chromium.org/1345583004/diff/20001/content/common/p2p_sock... content/common/p2p_socket_type.h:66: base::TimeTicks send_time_ticks; On 2015/09/17 17:19:31, Sergey Ulanov wrote: > nit: suggest calling it |send_time|, i.e. remove _ticks suffix. Done.
holmer@chromium.org changed reviewers: + jln@chromium.org
Adding jln for reviewing changes to content/common/p2p_socket_type.h. Please take a look.
lgtm https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/1345583004/diff/20001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:312: tick_received)); On 2015/09/18 07:56:51, Stefan wrote: > On 2015/09/17 17:19:31, Sergey Ulanov wrote: > > So you assign tick_received to sent_time_tick. There may be some delay caused > by > > IPC, i.e. this field is not going to reflect when the renderer is actually > tried > > to to send this packet. Is that what you want? > > I want this to represent the time when the packet hits the wire. My > understanding is that tick_received here is taken immediately before we call > SendTo() on the socket, which is about as close as we can get. > > So if I understand this correctly (and please correct me if I'm wrong!), this is > running in the browser process, and I'm sending back this timestamp to the > render process through P2PMsg_OnSendComplete below. If this is the case, then > this is the timestamp I'm interested in. > > We don't want the timestamp when the renderer sends the packet, as we are > measuring the time the packet spends on the internet, and not the time it spends > in the browser. I see. Note that send() completion just means that the data was placed in the buffer, it's not necessarily on the wire at that point. This shouldn't make much difference with UDP if send buffer is small. Also it's probably better to get the time in OnSend(). This might give you more precise time (particularly when send() call blocks because the buffer is full), but I don't feel very strongly about it. https://codereview.chromium.org/1345583004/diff/60001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/1345583004/diff/60001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:345: base::TimeTicks time_received, Should this be called send_time, or something like that? time_received makes little sense given that we are sending data here.
On 2015/09/18 07:57:48, Stefan wrote: > Adding jln for reviewing changes to content/common/p2p_socket_type.h. Please > take a look. jln's entry in content/common/OWNERS # For Linux sandboxing jln@chromium.org you need to find a content owner here.
holmer@chromium.org changed reviewers: + sievers@chromium.org - jln@chromium.org
Sorry, I have a hard time manually parsing content/common/OWNERS. Trying with git cl owners instead. :) sievers, could you take a look at content/common/p2p_socket_type.h?
lgtm https://codereview.chromium.org/1345583004/diff/60001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/1345583004/diff/60001/content/common/p2p_sock... content/common/p2p_socket_type.h:54: // timestamps when packet arrives at various points. nit: you might want to update the comment, or shorten it
Thanks a lot for your comments. https://codereview.chromium.org/1345583004/diff/60001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/1345583004/diff/60001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:345: base::TimeTicks time_received, On 2015/09/18 21:16:11, Sergey Ulanov wrote: > Should this be called send_time, or something like that? time_received makes > little sense given that we are sending data here. I definitely agree. Changing. https://codereview.chromium.org/1345583004/diff/60001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/1345583004/diff/60001/content/common/p2p_sock... content/common/p2p_socket_type.h:54: // timestamps when packet arrives at various points. On 2015/09/21 19:45:42, sievers wrote: > nit: you might want to update the comment, or shorten it Significantly shortened.
The CQ bit was checked by holmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1345583004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1345583004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Address change in libjingle
The CQ bit was checked by holmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1345583004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1345583004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This CL is now ready for a final review as the webrtc / libjingle CLs have landed and been rolled into Chromium. PTAL on the diff against patchset 5. I have made some name changes and added missing IPC definitions in p2p_messages.h. https://codereview.chromium.org/1345583004/diff/120001/content/browser/render... File content/browser/renderer_host/p2p/socket_host_udp.h (right): https://codereview.chromium.org/1345583004/diff/120001/content/browser/render... content/browser/renderer_host/p2p/socket_host_udp.h:20: #include "net/udp/diff_serv_code_point.h" Merge -- not part of this CL.
Ping. I'd much prefer to get a second look at this even though you gave me a green light on the previous revision.
still lgtm
The CQ bit was checked by holmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1345583004/#ps120001 (title: "Address change in libjingle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1345583004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1345583004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
holmer@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng, could you please review the changes to p2p_messages.h? Thanks!
lgtm https://codereview.chromium.org/1345583004/diff/120001/content/browser/render... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/1345583004/diff/120001/content/browser/render... content/browser/renderer_host/p2p/socket_host_udp.cc:299: base::Bind(&P2PSocketHostUdp::OnSend, base::Unretained(this), packet.id, Would be nice to document why Unretained() is safe here.
https://codereview.chromium.org/1345583004/diff/120001/content/browser/render... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/1345583004/diff/120001/content/browser/render... content/browser/renderer_host/p2p/socket_host_udp.cc:299: base::Bind(&P2PSocketHostUdp::OnSend, base::Unretained(this), packet.id, On 2015/10/20 22:43:31, dcheng wrote: > Would be nice to document why Unretained() is safe here. Sorry, I don't have the background to answer that question. This was mostly a change in code formatting.
The CQ bit was checked by holmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1345583004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1345583004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0fbb02dc39e4da03f5901f5b6ef7f3d15c1cbd22 Cr-Commit-Position: refs/heads/master@{#355256} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
