Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
159353002:
This CL adds methods to manipulate RTP header extension, particularly (Closed)
|
Created:
6 years, 10 months ago by Mallinath (Gone from Chromium) Modified:
3 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Solis, mflodman_chromium_OOO Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis CL adds methods to manipulate RTP header extension, particularly
AbsoulteSendTime extension. If there is matching extension ID present
in RTP packet, we will update with the current time.
R=solenberg@chromium.org, juberti@chromium.org, sergeyu@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256580
Patch Set 1 #Patch Set 2 : Updating TS in SendTime extension header #
Total comments: 40
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #
Total comments: 29
Patch Set 6 : #
Total comments: 50
Patch Set 7 : #
Total comments: 22
Patch Set 8 : #Patch Set 9 : #
Total comments: 10
Patch Set 10 : #Patch Set 11 : #
Total comments: 22
Patch Set 12 : #Patch Set 13 : #
Total comments: 31
Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #
Total comments: 13
Patch Set 19 : #Patch Set 20 : #
Total comments: 2
Patch Set 21 : #
Total comments: 2
Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #Patch Set 25 : #Patch Set 26 : #Patch Set 27 : #Patch Set 28 : #Messages
Total messages: 36 (0 generated)
Justin, This is CL not complete yet.
https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:208: const char* start = data; We can always return without doing anything, if PacketOption doesn't has any auth info.
https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:30: bool IsDtlsPacket(const char* data, int len) { need to think about whether this can be demuxed from turn trivially https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:47: class HmacCalculator { Is there no Chrome class to do this? Or can you use talk_base::ComputeHmac? https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:123: rtp_sendtime_extn_id_(3) { this will come down in each packet, right? https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:222: // Do we need to care about STUN DATA Indication? yes, unfortunately. until we finish channel bind, we use data indication. edge case but we would probably be a super hard problem to debug, so we should plan to handle it. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:228: if (IsRtcpPacket(start)) { i think you can do this right before ParseRtpPacket, so you only need to do it once https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:249: bool P2PSocketHost::ParseRtpPacket(const char* data, int in_len) { Parse is probably not the right term here, since you are finding and replacing at the same time https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:264: int rtp_hdr_len = kMinRtpHdrLen + 4 * cc_count; this is confusing since it doesnt include the full header len. maybe rtp_hdr_len_no_ext, or something similar https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:279: uint16 extn_length = talk_base::GetBE16(data + 2) * 4; double check that the length is still good now that we know the total header size https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:284: if (profile_id && 0xBEDE) { // OneByte extension header ==, not && https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:313: consumed += SkipPaddingBytes(data + consumed, extn_length - consumed); this seems overly complicated - feels like you could just do while (*data == 0) ++data, ++consumed https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:378: hmac.Compute(auth_tag, options.tag_len); don't forget that the tag is shorter than the total HMAC len, so you may need to have a temp buffer and memcpy to the (shorter) auth tag https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.h:52: void MaybeUpdateRtpSendTimeExtn(const char* data, int len); The naming could be clearer here - you have 3 functions that do similar things with different (and slightly confusing) names: MaybeURSTE works on an arbitrary packet ParseRtpPacket works on a RTP packet UpdateRtpSendTimeExtn works on the extension suggest having a more unified set of names, like MaybeUpdatePacketSendTime MaybeUpdateRtpSendTime UpdateRtpExtnSendTime UpdateRtpAuthTag https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_tcp.cc:521: MaybeUpdateRtpSendTimeExtn(&*data.begin(), data.size()); think you need to do this before the memcpy
not all addressed yet. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:47: class HmacCalculator { On 2014/02/14 01:53:27, juberti2 wrote: > Is there no Chrome class to do this? Or can you use talk_base::ComputeHmac? ComputeHmac takes only one input. For RTP we have to first auth with message then index, so I think we need this. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:123: rtp_sendtime_extn_id_(3) { yea absolutely, this was just for testing purpose. On 2014/02/14 01:53:27, juberti2 wrote: > this will come down in each packet, right? https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:222: // Do we need to care about STUN DATA Indication? sure. On 2014/02/14 01:53:27, juberti2 wrote: > yes, unfortunately. until we finish channel bind, we use data indication. edge > case but we would probably be a super hard problem to debug, so we should plan > to handle it.
https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:334: void P2PSocketHost::UpdateRtpSendTimeExtn(const char* data, int len) { Would prefer UpdateRtpAbsSendTimeExtension https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:353: uint32 now_second = base::Time::Now().ToInternalValue() / Chromium doesn't recommend using this method. I'm really not familiar with it, so I can't tell why. Can't you use InMicroseconds() instead? Something like: uint32 now_second = ((base::Time::Now().InMicroseconds() << 18) / 1000000) & 0x00FFFFFF; https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=86
On 2014/02/14 02:19:07, mallinath2 wrote: > not all addressed yet. > > https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... > File content/browser/renderer_host/p2p/socket_host.cc (right): > > https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... > content/browser/renderer_host/p2p/socket_host.cc:47: class HmacCalculator { > On 2014/02/14 01:53:27, juberti2 wrote: > > Is there no Chrome class to do this? Or can you use talk_base::ComputeHmac? > > ComputeHmac takes only one input. For RTP we have to first auth with message > then index, so I think we need this. you can just write the index to the first 4 bytes of the bogus auth tag, and then computedigest on the data + 4 bytes. > > https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... > content/browser/renderer_host/p2p/socket_host.cc:123: rtp_sendtime_extn_id_(3) { > yea absolutely, this was just for testing purpose. > > On 2014/02/14 01:53:27, juberti2 wrote: > > this will come down in each packet, right? > > https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... > content/browser/renderer_host/p2p/socket_host.cc:222: // Do we need to care > about STUN DATA Indication? > sure. > On 2014/02/14 01:53:27, juberti2 wrote: > > yes, unfortunately. until we finish channel bind, we use data indication. edge > > case but we would probably be a super hard problem to debug, so we should plan > > to handle it.
https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:206: void P2PSocketHost::MaybeUpdateRtpSendTimeExtn(const char* data, int len) { probably don't want the data pointer to be const if you are going to update it.
https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:308: if (id == rtp_sendtime_extn_id_) { IIUC rtp_sendtime_extn_id_ is the only P2PSocketHost field that is being used in these functions. Can you make it an argument to MaybeUpdate...() instead and pull the functions into a separate utility class/namespace, or even tuck them away in the anonymous ns at the top of this file? https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:357: // we can avoid manipulation everywhere than where it's required. IMO that's broken. If MaybeUpdateRtpSendTimeExtn() can actually change the buffer, it shouldn't take a const pointer. Alternatively, break the MaybeUpdate...() into two functions: char* FindRtpHeaderExtension(const char* data, int len, int extension_id, int extension_len) (returns null if not present in packet for whatever reason, knows nothing about the internals of the header extension) void UpdateAbsoluteSenderTimeExtension(char* data); https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.h:52: void MaybeUpdateRtpSendTimeExtn(const char* data, int len); I assume you mean to update the absolute sender time extension, if present. Change name to MaybeUpdateAbsSendTimeExtn() or MaybeUpdateAbsoluteSenderTimeExtension() Not sure whether the "Rtp" part is necessary or not. Maybe you could pull all these functions into a utility namespace? https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.h:98: bool ParseRtpPacket(const char* data, int len); I think you can remove these functions from the class - see comment in .cc. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_tcp.cc:521: MaybeUpdateRtpSendTimeExtn(&*data.begin(), data.size()); Avoid implicitly returning an iterator by using: &data[0]
PTAL https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:206: void P2PSocketHost::MaybeUpdateRtpSendTimeExtn(const char* data, int len) { On 2014/02/14 09:23:56, juberti2 wrote: > probably don't want the data pointer to be const if you are going to update it. Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:228: if (IsRtcpPacket(start)) { On 2014/02/14 01:53:27, juberti2 wrote: > i think you can do this right before ParseRtpPacket, so you only need to do it > once Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:249: bool P2PSocketHost::ParseRtpPacket(const char* data, int in_len) { On 2014/02/14 01:53:27, juberti2 wrote: > Parse is probably not the right term here, since you are finding and replacing > at the same time Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:264: int rtp_hdr_len = kMinRtpHdrLen + 4 * cc_count; On 2014/02/14 01:53:27, juberti2 wrote: > this is confusing since it doesnt include the full header len. maybe > rtp_hdr_len_no_ext, or something similar Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:279: uint16 extn_length = talk_base::GetBE16(data + 2) * 4; On 2014/02/14 01:53:27, juberti2 wrote: > double check that the length is still good now that we know the total header > size Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:284: if (profile_id && 0xBEDE) { // OneByte extension header On 2014/02/14 01:53:27, juberti2 wrote: > ==, not && Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:308: if (id == rtp_sendtime_extn_id_) { Yea, this I added for testing purpose. Extension id is part of PacketOptions. On 2014/02/14 10:05:29, Solis wrote: > IIUC rtp_sendtime_extn_id_ is the only P2PSocketHost field that is being used in > these functions. Can you make it an argument to MaybeUpdate...() instead and > pull the functions into a separate utility class/namespace, or even tuck them > away in the anonymous ns at the top of this file? https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:313: consumed += SkipPaddingBytes(data + consumed, extn_length - consumed); On 2014/02/14 01:53:27, juberti2 wrote: > this seems overly complicated - feels like you could just do while (*data == 0) > ++data, ++consumed Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:334: void P2PSocketHost::UpdateRtpSendTimeExtn(const char* data, int len) { On 2014/02/14 09:02:44, Stefan wrote: > Would prefer UpdateRtpAbsSendTimeExtension Done. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:353: uint32 now_second = base::Time::Now().ToInternalValue() / On 2014/02/14 09:02:44, Stefan wrote: > Chromium doesn't recommend using this method. I'm really not familiar with it, > so I can't tell why. Can't you use InMicroseconds() instead? Something like: > uint32 now_second = ((base::Time::Now().InMicroseconds() << 18) / 1000000) & > 0x00FFFFFF; > > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=86 I think you got confused with base::TimeDelta. Time::ToInternalValue() is good to use. One in TimeDelta is not recommended. https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:357: // we can avoid manipulation everywhere than where it's required. On 2014/02/14 10:05:29, Solis wrote: > IMO that's broken. If MaybeUpdateRtpSendTimeExtn() can actually change the > buffer, it shouldn't take a const pointer. > > Alternatively, break the MaybeUpdate...() into two functions: > > char* FindRtpHeaderExtension(const char* data, int len, int extension_id, int > extension_len) > (returns null if not present in packet for whatever reason, knows nothing about > the internals of the header extension) > > void UpdateAbsoluteSenderTimeExtension(char* data); Done.
https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:353: uint32 now_second = base::Time::Now().ToInternalValue() / Looks like it says the same n Time::ToInternalValue(): https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... But to be honest I haven't worked with this class much, so I'm not sure about the convention. I think I have seen it be used as (base:Time::Now() - base:Time::UnixEpoch).InMilliseconds() before. Maybe something like that is preferable? If you know it is often used like this then it's ok with me, but I couldn't find any such usage when searching cs.chromium.org. On 2014/02/18 22:40:57, mallinath2 wrote: > On 2014/02/14 09:02:44, Stefan wrote: > > Chromium doesn't recommend using this method. I'm really not familiar with it, > > so I can't tell why. Can't you use InMicroseconds() instead? Something like: > > uint32 now_second = ((base::Time::Now().InMicroseconds() << 18) / 1000000) & > > 0x00FFFFFF; > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=86 > > I think you got confused with base::TimeDelta. Time::ToInternalValue() is good > to use. One in TimeDelta is not recommended. https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:212: char* data, int len, const talk_base::PacketOptions& options) { len -> length https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:223: // will be larger than actuan payload len. Skipping wrapper to point to the actual payload length https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:252: // We should find extension id in rtp packet. We should find an extension id in the rtp packet.
https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:30: bool IsDtlsPacket(const char* data, int len) { I don't think we need any of these methods now. On 2014/02/14 01:53:27, juberti2 wrote: > need to think about whether this can be demuxed from turn trivially https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:353: uint32 now_second = base::Time::Now().ToInternalValue() / Ok, I was thinking the discouragement is only for doing arithmetic on the value, heck we are doing it. But there is TimeTicks class which doesn't have any restrictions and searched the code base, there are quite few usages of that method. On 2014/02/19 08:17:32, Stefan wrote: > Looks like it says the same n Time::ToInternalValue(): > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... > > But to be honest I haven't worked with this class much, so I'm not sure about > the convention. I think I have seen it be used as > (base:Time::Now() - base:Time::UnixEpoch).InMilliseconds() before. Maybe > something like that is preferable? > > If you know it is often used like this then it's ok with me, but I couldn't find > any such usage when searching http://cs.chromium.org. > > On 2014/02/18 22:40:57, mallinath2 wrote: > > On 2014/02/14 09:02:44, Stefan wrote: > > > Chromium doesn't recommend using this method. I'm really not familiar with > it, > > > so I can't tell why. Can't you use InMicroseconds() instead? Something > like: > > > uint32 now_second = ((base::Time::Now().InMicroseconds() << 18) / 1000000) & > > > 0x00FFFFFF; > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=86 > > > > I think you got confused with base::TimeDelta. Time::ToInternalValue() is good > > to use. One in TimeDelta is not recommended. > https://codereview.chromium.org/159353002/diff/70001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host.cc:378: hmac.Compute(auth_tag, options.tag_len); On 2014/02/14 01:53:27, juberti2 wrote: > don't forget that the tag is shorter than the total HMAC len, so you may need to > have a temp buffer and memcpy to the (shorter) auth tag Done. https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:212: char* data, int len, const talk_base::PacketOptions& options) { On 2014/02/19 08:17:32, Stefan wrote: > len -> length Done. https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:223: // will be larger than actuan payload len. Skipping wrapper to point to the On 2014/02/19 08:17:32, Stefan wrote: > actual payload length Done. https://codereview.chromium.org/159353002/diff/220001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:252: // We should find extension id in rtp packet. On 2014/02/19 08:17:32, Stefan wrote: > We should find an extension id in the rtp packet. Done.
https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:355: uint64 now_ms = base::TimeTicks::Now().ToInternalValue(); TimeTicks::Now() seems to have a resolution of 1-15 ms. That sounds pretty bad... If we get 15 ms jitter just from the inaccuracy of the clock we are in trouble. Maybe we should consider using TimeTicks::HighResNow() instead? It is more expensive, but I don't have a good understanding of how expensive... Note that this function returns the time in microseconds, so you'll have to divide by 1000 to get milliseconds. Or just change the unit.
https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:51: class HMAC { There is HMAC implamentation in crypto/hmac.h. Is it necessary to duplicate it here?
https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:51: class HMAC { Yea, I saw that. I am also exploring that option, but they all including one in libjingle(ComputeHmac) all take one message and compute HMAC on it. But in our case we will have 2 inputs. One message it self and other is SRTP packet index. That's why I call Update twice below and then call Compute at the end. On 2014/02/20 19:30:28, Sergey Ulanov wrote: > There is HMAC implamentation in crypto/hmac.h. Is it necessary to duplicate it > here? https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:355: uint64 now_ms = base::TimeTicks::Now().ToInternalValue(); OK. After some searching in code base, HighResNow is used in some media code bases. Also HighResNow implement differs from Now() only on Windows. Sergey, any insights? On 2014/02/20 11:22:58, Stefan wrote: > TimeTicks::Now() seems to have a resolution of 1-15 ms. That sounds pretty > bad... If we get 15 ms jitter just from the inaccuracy of the clock we are in > trouble. > > Maybe we should consider using TimeTicks::HighResNow() instead? It is more > expensive, but I don't have a good understanding of how expensive... > > Note that this function returns the time in microseconds, so you'll have to > divide by 1000 to get milliseconds. Or just change the unit.
https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:355: uint64 now_ms = base::TimeTicks::Now().ToInternalValue(); On 2014/02/20 19:34:32, mallinath2 wrote: > OK. After some searching in code base, HighResNow is used in some media code > bases. Also HighResNow implement differs from Now() only on Windows. > > Sergey, any insights? I'm not an expert, but I think it should be acceptable to use HighResNow() here, just make sure it's not called more than once per package. Also use InMicroseconds() instead of ToInternalValue() as you don't want to depend on internal details of TimeTicks (e.g. it may be changed to measure time in nanoseconds in the future)
Need unit tests for the packet processing. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:51: class HMAC { On 2014/02/20 19:34:32, mallinath2 wrote: > Yea, I saw that. I am also exploring that option, but they all including one in > libjingle(ComputeHmac) all take one message and compute HMAC on it. But in our > case we will have 2 inputs. One message it self and other is SRTP packet index. > That's why I call Update twice below and then call Compute at the end. > > On 2014/02/20 19:30:28, Sergey Ulanov wrote: > > There is HMAC implamentation in crypto/hmac.h. Is it necessary to duplicate it > > here? > Update(x + y), Compute is the same as Update(x), Update(y), Compute. So I think we can make it work with the existing classes. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:213: if (options.packet_time_params.rtp_sendtime_extension_id == -1 && The comment doesn't seem to match the code. || instead of &&? https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:219: int raw_packet_len = options.packet_time_params.payload_len; Need to split RTP/TURN Channel Data/TURN Send Indication here. This demux is easy, some work needed to loop through headers when doing the Send Indication. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:235: UpdateHmac(start, raw_packet_len, options); Seems like we could structure this logic more cleanly - basically, only update the extension if there is an extension id, but always update the HMAC https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:244: DCHECK(false); DCHECK(!IsRtcpPacket(start)) right after we check that there is a id and key (above) https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:324: return true; can just break here https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:327: data += consumed; I don't think this is right - this should increment data by the same amount as consumed. Overall it seems like we should not be incrementing two counters - can you get rid of consumed by having something like char* extn_start = data and while (data - extn_start < extn_length) https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:329: while(*data != 0) { while (*data) https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:368: char* auth_tag = packet + DCHECK that the tag is 0xbaddbaddbaddbadd, and srtp_auth_tag_len is < len, and < HMAC output size https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:370: HMAC hmac; Just realized - not an issue now, but we will need to pass down the HMAC algorithm in the PacketOptions to handle non-SHA-1 HMACs in the future https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:377: char output[20]; In the future, this buffer may need to be bigger (e.g. for SHA-256). Suggest making this 64 bytes or more
PTAL. Working on test code. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:51: class HMAC { Thanks, Let me try that. On 2014/02/20 23:43:56, juberti2 wrote: > On 2014/02/20 19:34:32, mallinath2 wrote: > > Yea, I saw that. I am also exploring that option, but they all including one > in > > libjingle(ComputeHmac) all take one message and compute HMAC on it. But in our > > case we will have 2 inputs. One message it self and other is SRTP packet > index. > > That's why I call Update twice below and then call Compute at the end. > > > > On 2014/02/20 19:30:28, Sergey Ulanov wrote: > > > There is HMAC implamentation in crypto/hmac.h. Is it necessary to duplicate > it > > > here? > > > > Update(x + y), Compute is the same as Update(x), Update(y), Compute. So I think > we can make it work with the existing classes. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:213: if (options.packet_time_params.rtp_sendtime_extension_id == -1 && We should update HMAC even if extension is not present. But for debugging purpose if we disable SRTP, we should still be able to update timestamp before sending packet. Hence I added &&. Otherwise it should bail out only if keys are not present. On 2014/02/20 23:43:56, juberti2 wrote: > The comment doesn't seem to match the code. || instead of &&? https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:219: int raw_packet_len = options.packet_time_params.payload_len; On 2014/02/20 23:43:56, juberti2 wrote: > Need to split RTP/TURN Channel Data/TURN Send Indication here. This demux is > easy, some work needed to loop through headers when doing the Send Indication. Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:219: int raw_packet_len = options.packet_time_params.payload_len; On 2014/02/20 23:43:56, juberti2 wrote: > Need to split RTP/TURN Channel Data/TURN Send Indication here. This demux is > easy, some work needed to loop through headers when doing the Send Indication. Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:235: UpdateHmac(start, raw_packet_len, options); On 2014/02/20 23:43:56, juberti2 wrote: > Seems like we could structure this logic more cleanly - basically, only update > the extension if there is an extension id, but always update the HMAC Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:244: DCHECK(false); On 2014/02/20 23:43:56, juberti2 wrote: > DCHECK(!IsRtcpPacket(start)) right after we check that there is a id and key > (above) Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:324: return true; On 2014/02/20 23:43:56, juberti2 wrote: > can just break here Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:327: data += consumed; On 2014/02/20 23:43:56, juberti2 wrote: > I don't think this is right - this should increment data by the same amount as > consumed. Overall it seems like we should not be incrementing two counters - can > you get rid of consumed by having something like char* extn_start = data and > while (data - extn_start < extn_length) Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:329: while(*data != 0) { On 2014/02/20 23:43:56, juberti2 wrote: > while (*data) Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:355: uint64 now_ms = base::TimeTicks::Now().ToInternalValue(); On 2014/02/20 19:46:10, Sergey Ulanov wrote: > On 2014/02/20 19:34:32, mallinath2 wrote: > > OK. After some searching in code base, HighResNow is used in some media code > > bases. Also HighResNow implement differs from Now() only on Windows. > > > > Sergey, any insights? > > I'm not an expert, but I think it should be acceptable to use HighResNow() here, > just make sure it's not called more than once per package. > Also use InMicroseconds() instead of ToInternalValue() as you don't want to > depend on internal details of TimeTicks (e.g. it may be changed to measure time > in nanoseconds in the future) Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:368: char* auth_tag = packet + On 2014/02/20 23:43:56, juberti2 wrote: > DCHECK that the tag is 0xbaddbaddbaddbadd, and srtp_auth_tag_len is < len, and < > HMAC output size Done. https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:370: HMAC hmac; Agree. On 2014/02/20 23:43:56, juberti2 wrote: > Just realized - not an issue now, but we will need to pass down the HMAC > algorithm in the PacketOptions to handle non-SHA-1 HMACs in the future https://codereview.chromium.org/159353002/diff/300001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:377: char output[20]; On 2014/02/20 23:43:56, juberti2 wrote: > In the future, this buffer may need to be bigger (e.g. for SHA-256). Suggest > making this 64 bytes or more Done.
https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:27: static const unsigned char kFakeHmac[10] = { kFakeAuthTag. Add a comment describing this https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:31: bool IsTurnChannelData(const char* data) { can just look at first byte, like below: (*data & 0xC0) == 0x40 https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:47: uint16 type = talk_base::GetBE16(data); can just look at first byte and do ((*data & 0xC0) == 0) https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:51: bool IsTurnDataIndicationPacket(const char* data) { SendIndication https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:57: if (len < kMinRtpHdrLen) probably not necessary to check this here, since you will check it later https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:60: return ((static_cast<int>((*data >> 6) & 0x3)) == 2) ? true : false; To be consistent with STUN check, do (*data & 0xC0) == 0x80 https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:171: // This method should never return false. Use NOTREACHED then? https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:186: DCHECK(false); NOTREACHED; return; https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:199: // Parsing a TURN SEND INDICATION message is bit tricky as it can have I think it would be better to iterate the STUN attributes manually, since that will let you find the DATA start more directly. The iteration is pretty easy, just skip the header, then read the type/length and skip forward as needed, adjusting for padding. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:219: *rtp_start_pos = length - payload_len; This is fragile since there is no guarantee that the DATA attrib will be the last attribute. I think you want to output the start and the length of the data. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:224: DCHECK(!IsRtpPacket(packet + *rtp_start_pos, length - *rtp_start_pos)); shouldn't there be no ! https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:228: bool P2PSocketHost::UpdateRtpAbsSendTimeExtension(char* data, int length, instead of |data|, perhaps call this |rtp| to make it clear it is a RTP packet we are operating on https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:246: DCHECK(false); NOTREACHED https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:263: DCHECK(false); NOTREACHED https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:290: while(data - extn_start < extn_length) { while ( https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:301: while (*data != 0) { need to check that we don't go off the end here - might be better to compute padding explicitly, e.g. data += ((kOneByteHdrLen + len + 3) % 4) https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:309: void P2PSocketHost::UpdateAbsSendtime(char* data, int len) { instead of |data|, call this |extn_data| or something to make it clear it is just updating the extension payload https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:340: void P2PSocketHost::UpdateHmac(char* packet, int len, packet -> rtp https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:347: DCHECK_LT(tag_length, 64); // HMAC output. make sure it is less than hmac.Size https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:355: DCHECK(false); NOTREACHED https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:360: memcpy(auth_tag, reinterpret_cast<const void*>( don't need to cast https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:103: bool UpdateRtpAbsSendTimeExtension(char* data, int length, This should probably also be a Maybe function. Perhaps call the first function MaybeUpdatePacketSendTimeExtn, the second MaybeUpdateRtpSendTimeExtn, and the last UpdateSendTimeExtnValue. The last function should be MaybeUpdateRtpAuthTag https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:104: const int extension_id); const not needed on value types https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_test_utils.cc (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_test_utils.cc:217: *reinterpret_cast<uint16*>(&*packet->begin() + kRtpHeaderSize) = 0xBEDE; Need to use SetBE16 to get the right endianness https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:68: memcpy(&packet_options.packet_time_params.srtp_auth_key[0], no need to do this copy
PTAL https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:27: static const unsigned char kFakeHmac[10] = { On 2014/02/22 01:16:05, juberti2 wrote: > kFakeAuthTag. Add a comment describing this Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:31: bool IsTurnChannelData(const char* data) { On 2014/02/22 01:16:05, juberti2 wrote: > can just look at first byte, like below: (*data & 0xC0) == 0x40 Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:47: uint16 type = talk_base::GetBE16(data); On 2014/02/22 01:16:05, juberti2 wrote: > can just look at first byte and do ((*data & 0xC0) == 0) Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:51: bool IsTurnDataIndicationPacket(const char* data) { On 2014/02/22 01:16:05, juberti2 wrote: > SendIndication Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:57: if (len < kMinRtpHdrLen) On 2014/02/22 01:16:05, juberti2 wrote: > probably not necessary to check this here, since you will check it later Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:60: return ((static_cast<int>((*data >> 6) & 0x3)) == 2) ? true : false; On 2014/02/22 01:16:05, juberti2 wrote: > To be consistent with STUN check, do (*data & 0xC0) == 0x80 Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:171: // This method should never return false. On 2014/02/22 01:16:05, juberti2 wrote: > Use NOTREACHED then? Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:186: DCHECK(false); On 2014/02/22 01:16:05, juberti2 wrote: > NOTREACHED; return; Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:199: // Parsing a TURN SEND INDICATION message is bit tricky as it can have On 2014/02/22 01:16:05, juberti2 wrote: > I think it would be better to iterate the STUN attributes manually, since that > will let you find the DATA start more directly. The iteration is pretty easy, > just skip the header, then read the type/length and skip forward as needed, > adjusting for padding. Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:219: *rtp_start_pos = length - payload_len; On 2014/02/22 01:16:05, juberti2 wrote: > This is fragile since there is no guarantee that the DATA attrib will be the > last attribute. I think you want to output the start and the length of the data. Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:224: DCHECK(!IsRtpPacket(packet + *rtp_start_pos, length - *rtp_start_pos)); On 2014/02/22 01:16:05, juberti2 wrote: > shouldn't there be no ! Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:228: bool P2PSocketHost::UpdateRtpAbsSendTimeExtension(char* data, int length, On 2014/02/22 01:16:05, juberti2 wrote: > instead of |data|, perhaps call this |rtp| to make it clear it is a RTP packet > we are operating on Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:246: DCHECK(false); On 2014/02/22 01:16:05, juberti2 wrote: > NOTREACHED Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:263: DCHECK(false); On 2014/02/22 01:16:05, juberti2 wrote: > NOTREACHED Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:290: while(data - extn_start < extn_length) { On 2014/02/22 01:16:05, juberti2 wrote: > while ( Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:301: while (*data != 0) { I am not sure we can calculate padding explicitly as you suggested. From RFC 5285, it's not clear that one byte header extension will rounded to a 32-bit boundary. It says "A sequence of extension elements, possibly with padding, forms the header extension defined in the RTP specification. There are as many extension elements as fit into the length as indicated in the RTP header extension length. Since this length is signaled in full 32- bit words, padding bytes are used to pad to a 32-bit boundary. The entire extension is parsed byte-by-byte to find each extension element (no alignment is required), and parsing stops at the earlier of the end of the entire header extension, or, in one-byte headers, on encountering an identifier with the reserved value of 15." Also example in the RFC doesn't clearly shows how padding bytes are calculated for individual extensions. On 2014/02/22 01:16:05, juberti2 wrote: > need to check that we don't go off the end here - might be better to compute > padding explicitly, e.g. data += ((kOneByteHdrLen + len + 3) % 4) https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:309: void P2PSocketHost::UpdateAbsSendtime(char* data, int len) { On 2014/02/22 01:16:05, juberti2 wrote: > instead of |data|, call this |extn_data| or something to make it clear it is > just updating the extension payload Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:340: void P2PSocketHost::UpdateHmac(char* packet, int len, On 2014/02/22 01:16:05, juberti2 wrote: > packet -> rtp Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:347: DCHECK_LT(tag_length, 64); // HMAC output. On 2014/02/22 01:16:05, juberti2 wrote: > make sure it is less than hmac.Size Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:355: DCHECK(false); On 2014/02/22 01:16:05, juberti2 wrote: > NOTREACHED Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:360: memcpy(auth_tag, reinterpret_cast<const void*>( On 2014/02/22 01:16:05, juberti2 wrote: > don't need to cast Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:103: bool UpdateRtpAbsSendTimeExtension(char* data, int length, On 2014/02/22 01:16:05, juberti2 wrote: > This should probably also be a Maybe function. > > Perhaps call the first function MaybeUpdatePacketSendTimeExtn, the second > MaybeUpdateRtpSendTimeExtn, and the last UpdateSendTimeExtnValue. > > The last function should be MaybeUpdateRtpAuthTag Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:104: const int extension_id); On 2014/02/22 01:16:05, juberti2 wrote: > const not needed on value types Done. https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:68: memcpy(&packet_options.packet_time_params.srtp_auth_key[0], On 2014/02/22 01:16:05, juberti2 wrote: > no need to do this copy Done.
https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/410001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:301: while (*data != 0) { On 2014/02/25 00:20:02, mallinath2 wrote: > I am not sure we can calculate padding explicitly as you suggested. From RFC > 5285, it's not clear that one byte header extension will rounded to a 32-bit > boundary. It says > > "A sequence of extension elements, possibly with padding, forms the > header extension defined in the RTP specification. There are as many > extension elements as fit into the length as indicated in the RTP > header extension length. Since this length is signaled in full 32- > bit words, padding bytes are used to pad to a 32-bit boundary. The > entire extension is parsed byte-by-byte to find each extension > element (no alignment is required), and parsing stops at the earlier > of the end of the entire header extension, or, in one-byte headers, > on encountering an identifier with the reserved value of 15." > > Also example in the RFC doesn't clearly shows how padding bytes are calculated > for individual extensions. > > > On 2014/02/22 01:16:05, juberti2 wrote: > > need to check that we don't go off the end here - might be better to compute > > padding explicitly, e.g. data += ((kOneByteHdrLen + len + 3) % 4) > I see. Should this while be checking *rtp == 0? https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:152: void P2PSocketHost::MaybeUpdatePacketSendTimeExtn( Check for zero length packets (will cause IsDtlsPacket to read off end) https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:209: *rtp_packet_length = talk_base::GetBE16(&packet[2]); need to check len before reading this field, and also check that the returned length is compatible with |length| https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:231: attr_type = talk_base::GetBE16(&packet[*rtp_start_pos]); need to check len here https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:237: *rtp_start_pos += (4- (attr_length % 4)); 4 - https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:242: *rtp_packet_length = attr_length; need to verify length is legit https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:282: uint16 profile_id = talk_base::GetBE16(rtp); need to validate this length https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:294: bool found = false; not sure we need this - i think we can return true even if not found (i.e. same as no X bit case) https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:393: DCHECK(!hmac.Sign(rtp, output, out_len)); how does Sign know how many bytes to hash? https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_test_utils.cc (right): https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_test_utils.cc:215: void CreateRtpPacketWithSendtimeExtension(std::vector<char>* packet, SendTime->SendTime https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_test_utils.cc:228: size_t size = kRtpHeaderSize + kTurnChannelDataHdr + rand() % 1000; instead of checking random sizes, i think we should form a valid packet and test that packet by chopping off a byte at a time until it gets down to a zero-len packet. That will ensure we cover all the too-short cases. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_test_utils.cc:234: *reinterpret_cast<uint8*>(&*packet->begin() + 4) = 0x80; this should call CreateRtpPacketWithSendTimeExtension to fill in a valid packet with the extension.
PTAL https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:152: void P2PSocketHost::MaybeUpdatePacketSendTimeExtn( On 2014/02/25 00:48:24, juberti2 wrote: > Check for zero length packets (will cause IsDtlsPacket to read off end) Done. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:209: *rtp_packet_length = talk_base::GetBE16(&packet[2]); On 2014/02/25 00:48:24, juberti2 wrote: > need to check len before reading this field, and also check that the returned > length is compatible with |length| Done. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:231: attr_type = talk_base::GetBE16(&packet[*rtp_start_pos]); On 2014/02/25 00:48:24, juberti2 wrote: > need to check len here Done. Also there is check in while condition. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:237: *rtp_start_pos += (4- (attr_length % 4)); On 2014/02/25 00:48:24, juberti2 wrote: > 4 - Done. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:242: *rtp_packet_length = attr_length; On 2014/02/25 00:48:24, juberti2 wrote: > need to verify length is legit Done. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:282: uint16 profile_id = talk_base::GetBE16(rtp); On 2014/02/25 00:48:24, juberti2 wrote: > need to validate this length It's done @ line 287. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:294: bool found = false; Agree. I added for testing this method when we pass different extension id then what set in the packet. On 2014/02/25 00:48:24, juberti2 wrote: > not sure we need this - i think we can return true even if not found (i.e. same > as no X bit case) https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:393: DCHECK(!hmac.Sign(rtp, output, out_len)); It calculates on whole |rtp|. Yes, there will be a problem if there are other attributes in stun indication after data_attributes. But TurnPort always puts it at the end. On 2014/02/25 00:48:24, juberti2 wrote: > how does Sign know how many bytes to hash? https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_test_utils.cc (right): https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_test_utils.cc:215: void CreateRtpPacketWithSendtimeExtension(std::vector<char>* packet, On 2014/02/25 00:48:24, juberti2 wrote: > SendTime->SendTime Done. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_test_utils.cc:228: size_t size = kRtpHeaderSize + kTurnChannelDataHdr + rand() % 1000; Yea, removing these methods and adding test vector in stun_host_unittest.cc On 2014/02/25 00:48:24, juberti2 wrote: > instead of checking random sizes, i think we should form a valid packet and test > that packet by chopping off a byte at a time until it gets down to a zero-len > packet. That will ensure we cover all the too-short cases. https://codereview.chromium.org/159353002/diff/530001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_test_utils.cc:234: *reinterpret_cast<uint8*>(&*packet->begin() + 4) = 0x80; As above. On 2014/02/25 00:48:24, juberti2 wrote: > this should call CreateRtpPacketWithSendTimeExtension to fill in a valid packet > with the extension.
https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:185: if (options.packet_time_params.rtp_sendtime_extension_id != -1) { You know this from the check at the beginning of the function. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:215: *rtp_start_pos = kTurnChannelHdrLen; I strongly advise against changing an output parameter unless the function succeeds. You should keep the start pos and length on the stack and only write to rtp_start_pos and rtp_packet_length on success. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:57: void MaybeUpdatePacketSendTimeExtn(char* data, int length, AFAICT none of the methods you've added need to access state in the class. Therefore they should be pulled out from the class and into a namespace instead (or at the very least be made static). By doing so there would be no need to subclass P2PSocketHost in the test. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:108: void UpdateSendTimeExtnValue(char* data, int len); We are using the names PacketSendTimeExtn RtpSendTimeExtn SendTimeExtn and in the .cc also AbsSendTimeExtn to refer to the same header extension. Can we stick to a single nomenclature? I would prefer AbsoluteSenderTimeExtension, or if you must abbreviate, AbsSendTimeExtn. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_tcp.cc:458: talk_base::PacketOptions options; So I guess initializing 'options' to something other than default will come in a later CL?
PTAL https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:185: if (options.packet_time_params.rtp_sendtime_extension_id != -1) { At the beginning i am checking both extension id and srtp auth params. So we can be here if extn id = -1 and non empty srtp auth params, i.e. function beginning condition will fail. On 2014/02/26 15:49:34, Solis wrote: > You know this from the check at the beginning of the function. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:215: *rtp_start_pos = kTurnChannelHdrLen; On 2014/02/26 15:49:34, Solis wrote: > I strongly advise against changing an output parameter unless the function > succeeds. You should keep the start pos and length on the stack and only write > to rtp_start_pos and rtp_packet_length on success. Done. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:57: void MaybeUpdatePacketSendTimeExtn(char* data, int length, On 2014/02/26 15:49:34, Solis wrote: > AFAICT none of the methods you've added need to access state in the class. > Therefore they should be pulled out from the class and into a namespace instead > (or at the very least be made static). By doing so there would be no need to > subclass P2PSocketHost in the test. That's a good point. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:108: void UpdateSendTimeExtnValue(char* data, int len); On 2014/02/26 15:49:34, Solis wrote: > We are using the names > > PacketSendTimeExtn > RtpSendTimeExtn > SendTimeExtn > and in the .cc also > AbsSendTimeExtn > > to refer to the same header extension. Can we stick to a single nomenclature? I > would prefer AbsoluteSenderTimeExtension, or if you must abbreviate, > AbsSendTimeExtn. Done. Using AbsSendTimeExtn. https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/570001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_tcp.cc:458: talk_base::PacketOptions options; Yes. On 2014/02/26 15:49:34, Solis wrote: > So I guess initializing 'options' to something other than default will come in a > later CL?
https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:152: void MaybeUpdatePacketAbsSendTimeExtn( Use same order as in .h. Makes it easier to find things. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:154: if (length <= 0) { No need to DCHECK(data != NULL); ? https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:235: char* start = packet + *rtp_start_pos; rtp_begin + add test case to catch. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:257: if (length < attr_length + *rtp_start_pos) { should be rtp_begin? add test case for this. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:271: if (length < rtp_length + rtp_begin) Be consistent with braces around single statements (and prefer to have them: #gotofail). https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:29: // Helper methods for processing a packet. put these in a nested namespace: namespace packet_processing_helpers { obviating the need for a comment. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:30: bool GetRtpPacketStartPositionAndLength(char* data, int length, Only GetRtpPacketStartPositionAndLength() and MaybeUpdatePacketAbsSendTimeExtn() need to be exposed at this level. The others should be private helpers in the .cc. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:127: TEST(P2PSocketHostTest, TestInvalidRtpMessages) { Consider breaking this multi-test into several small test cases. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:130: reinterpret_cast<char*>(kRtpMsgWithInvalidLength), Why not change the interface of GetRtpPacketStartPositionAndLength() to take an unsigned char, or even uint8_t, to avoid the need for this cast? https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:132: &start_pos, &rtp_length)); You should initialize start_pos and rtp_length to something unlikely (negative in this case) and verify that they're untouched at this point. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:167: TEST(P2PSocketHostTest, TestUpdateRtpAbsSendTimeExtension) { Likewise, consider splitting up. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:168: int start_pos, rtp_length; initialize these
PTAL https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:152: void MaybeUpdatePacketAbsSendTimeExtn( On 2014/02/27 09:36:10, Solis wrote: > Use same order as in .h. Makes it easier to find things. Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:154: if (length <= 0) { On 2014/02/27 09:36:10, Solis wrote: > No need to DCHECK(data != NULL); ? Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:235: char* start = packet + *rtp_start_pos; On 2014/02/27 09:36:10, Solis wrote: > rtp_begin + add test case to catch. Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:257: if (length < attr_length + *rtp_start_pos) { On 2014/02/27 09:36:10, Solis wrote: > should be rtp_begin? add test case for this. Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:271: if (length < rtp_length + rtp_begin) On 2014/02/27 09:36:10, Solis wrote: > Be consistent with braces around single statements (and prefer to have them: > #gotofail). Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:29: // Helper methods for processing a packet. On 2014/02/27 09:36:10, Solis wrote: > put these in a nested namespace: > > namespace packet_processing_helpers { > > obviating the need for a comment. Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:30: bool GetRtpPacketStartPositionAndLength(char* data, int length, On 2014/02/27 09:36:10, Solis wrote: > Only GetRtpPacketStartPositionAndLength() and MaybeUpdatePacketAbsSendTimeExtn() > need to be exposed at this level. The others should be private helpers in the > .cc. Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:127: TEST(P2PSocketHostTest, TestInvalidRtpMessages) { On 2014/02/27 09:36:10, Solis wrote: > Consider breaking this multi-test into several small test cases. Done. https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:130: reinterpret_cast<char*>(kRtpMsgWithInvalidLength), The reason i kept char* because whole pipeline from renderer to browser processing is using char* for packet. So I think it's better to cast in unittests than changing whole pipe. On 2014/02/27 09:36:10, Solis wrote: > Why not change the interface of GetRtpPacketStartPositionAndLength() to take an > unsigned char, or even uint8_t, to avoid the need for this cast? https://codereview.chromium.org/159353002/diff/610001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:132: &start_pos, &rtp_length)); On 2014/02/27 09:36:10, Solis wrote: > You should initialize start_pos and rtp_length to something unlikely (negative > in this case) and verify that they're untouched at this point. Done.
https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:67: bool ValidateRtpHeader(char* rtp, int length); These can go in the anonymous namespace? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:113: MaybeUpdateRtpAuthTag(start, rtp_length, options); It seems MaybeUpdatePacketAbsSendTimeExtn() should be called something else, since one of its primary purposes seems to be setting the auth_tag properly, not just updating it in case we altered the packet contents? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:29: void MaybeUpdatePacketAbsSendTimeExtn(char* data, int length, Why aren't there any tests for this function? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:24: static unsigned char kRtpMessageWithInvalidExtnLength[] = { Stick to either Msg or Message. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:187: // Initializing out params to very large value. You should only need one test per message. Pick a default that will 1. cause indexing to shoot off into unallocated memory if the inputs are used without internal initialization (causing an access fault), and 2. is un unlikely result. -1 doesn't satisfy the first criteria. INT_MAX is a better choice. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:219: EXPECT_EQ(-1, ...) here as well https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:243: reinterpret_cast<char*>(kRtpPacketWithAbsSendTimeExtension), Why is this in a separate test case here, and another one in TestValidTurnSendIndicationMessages? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:312: // This test verifies seraching of RTP exension ID supplied as input in a spelling it looks to me the comment is wrong btw.
https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:67: bool ValidateRtpHeader(char* rtp, int length); yea, they can. The reason I kept in the same namespace because they are not declared in header file ( so outsiders can't invoke) and they are doing the packet processing. On 2014/03/03 10:22:29, Solis wrote: > These can go in the anonymous namespace? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:113: MaybeUpdateRtpAuthTag(start, rtp_length, options); I don't have better options than MaybeUpdatePacketAbsSendTimeExtnaAndRtpAuthTag() :). Any other suggestions? On 2014/03/03 10:22:29, Solis wrote: > It seems MaybeUpdatePacketAbsSendTimeExtn() should be called something else, > since one of its primary purposes seems to be setting the auth_tag properly, not > just updating it in case we altered the packet contents? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:29: void MaybeUpdatePacketAbsSendTimeExtn(char* data, int length, I am not sure how can I test this method in particular. As we can't test any thing deterministically in this method. Both timestamp and HMAC will vary. That's why I had to expose below two methods and test those instead. As you can see the major change is around parsing of the packet. On 2014/03/03 10:22:29, Solis wrote: > Why aren't there any tests for this function? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:24: static unsigned char kRtpMessageWithInvalidExtnLength[] = { On 2014/03/03 10:22:29, Solis wrote: > Stick to either Msg or Message. Done. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:24: static unsigned char kRtpMessageWithInvalidExtnLength[] = { Sticking to Msg On 2014/03/03 10:22:29, Solis wrote: > Stick to either Msg or Message. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:187: // Initializing out params to very large value. On 2014/03/03 10:22:29, Solis wrote: > You should only need one test per message. Pick a default that will 1. cause > indexing to shoot off into unallocated memory if the inputs are used without > internal initialization (causing an access fault), and 2. is un unlikely result. > -1 doesn't satisfy the first criteria. INT_MAX is a better choice. Done. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:219: On 2014/03/03 10:22:29, Solis wrote: > EXPECT_EQ(-1, ...) here as well Done. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:243: reinterpret_cast<char*>(kRtpPacketWithAbsSendTimeExtension), It was by mistake. On 2014/03/03 10:22:29, Solis wrote: > Why is this in a separate test case here, and another one in > TestValidTurnSendIndicationMessages? https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:312: // This test verifies seraching of RTP exension ID supplied as input in a On 2014/03/03 10:22:29, Solis wrote: > spelling > it looks to me the comment is wrong btw. Done.
https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:67: bool ValidateRtpHeader(char* rtp, int length); But putting them in the anonymous namespace signals to a casual reader 6 months from now that they are not exposed to outsiders, without reading the header file. In the old days these functions would have been declared "static" to signal and enforce the same concept. On 2014/03/04 00:15:46, mallinath2 wrote: > yea, they can. The reason I kept in the same namespace because they are not > declared in header file ( so outsiders can't invoke) and they are doing the > packet processing. > > On 2014/03/03 10:22:29, Solis wrote: > > These can go in the anonymous namespace? > https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:100: // If there a non negetive sendtime extension id present in packet options, spelling https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:113: MaybeUpdateRtpAuthTag(start, rtp_length, options); It's a bit of swiss-army-knife type of function - it has multiple duties depending on the input data. This is rarely a good idea. Options that come to mind are: 1. Split it into multiple, well-defined functions that have a single purpose. This will make the logic easier to follow as well. 2. Use an even more generic name such as "UpdateFromPacketOptions", "ApplyPacketOptions", etc. On 2014/03/04 00:15:46, mallinath2 wrote: > I don't have better options than > MaybeUpdatePacketAbsSendTimeExtnaAndRtpAuthTag() :). Any other suggestions? > > On 2014/03/03 10:22:29, Solis wrote: > > It seems MaybeUpdatePacketAbsSendTimeExtn() should be called something else, > > since one of its primary purposes seems to be setting the auth_tag properly, > not > > just updating it in case we altered the packet contents? > https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:366: &options.packet_time_params.srtp_auth_key[0]), nit: formatting https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:381: size_t out_len = sizeof(output); nit: no need for this temp variable. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:29: void MaybeUpdatePacketAbsSendTimeExtn(char* data, int length, 1. There's logic in the function which isn't that straightforward. This should be verified. 2. The function to update the auth tag is currently untested. One strategy for testing would be to feed this function with the same packets you have for the other tests and compare the output to some expected result (should you decide to do this, I would suggest creating a helper macro/function/template). Another strategy is to expose the auth tag update function here so it can be tested separately. On 2014/03/04 00:15:46, mallinath2 wrote: > I am not sure how can I test this method in particular. As we can't test any > thing deterministically in this method. Both timestamp and HMAC will vary. > That's why I had to expose below two methods and test those instead. As you can > see the major change is around parsing of the packet. > > On 2014/03/03 10:22:29, Solis wrote: > > Why aren't there any tests for this function? >
Finally I was able to test this with a real webrtc call. It needed some tweaks in libjingle mainly srtp packet index shifting by 16 and enabling external authentication only to send stream, before we had for all RTP streams. I did verify with TURN messages. Yet to verify for TCP packets, will try that tomorrow. I would like to commit this soon, so that we will get some cycles in testing before it goes to beta. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:67: bool ValidateRtpHeader(char* rtp, int length); On 2014/03/04 09:53:24, Solis wrote: > But putting them in the anonymous namespace signals to a casual reader 6 months > from now that they are not exposed to outsiders, without reading the header > file. In the old days these functions would have been declared "static" to > signal and enforce the same concept. > > On 2014/03/04 00:15:46, mallinath2 wrote: > > yea, they can. The reason I kept in the same namespace because they are not > > declared in header file ( so outsiders can't invoke) and they are doing the > > packet processing. > > > > On 2014/03/03 10:22:29, Solis wrote: > > > These can go in the anonymous namespace? > > > Done. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:100: // If there a non negetive sendtime extension id present in packet options, On 2014/03/04 09:53:24, Solis wrote: > spelling Done. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:113: MaybeUpdateRtpAuthTag(start, rtp_length, options); You are talking about MaybeUpdatePacketAbsSendTimeExtn or MaybeUpdateRtpAuthTag. MaybeUpdateRtpAuthTag is maybe because it will return without doing anything if key is null. But that's just error handling and i took maybe from that method. But the main reason I hid everything behind MaybeUpdatePacketAbsSendTimeExtn because I don't want caller to do all the logic checking and decide what to do. I still think it's reasonable thing to do in this case. On 2014/03/04 09:53:24, Solis wrote: > It's a bit of swiss-army-knife type of function - it has multiple duties > depending on the input data. This is rarely a good idea. Options that come to > mind are: > > 1. Split it into multiple, well-defined functions that have a single purpose. > This will make the logic easier to follow as well. > 2. Use an even more generic name such as "UpdateFromPacketOptions", > "ApplyPacketOptions", etc. > > On 2014/03/04 00:15:46, mallinath2 wrote: > > I don't have better options than > > MaybeUpdatePacketAbsSendTimeExtnaAndRtpAuthTag() :). Any other suggestions? > > > > On 2014/03/03 10:22:29, Solis wrote: > > > It seems MaybeUpdatePacketAbsSendTimeExtn() should be called something else, > > > since one of its primary purposes seems to be setting the auth_tag properly, > > not > > > just updating it in case we altered the packet contents? > > > https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:366: &options.packet_time_params.srtp_auth_key[0]), On 2014/03/04 09:53:24, Solis wrote: > nit: formatting Done. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:381: size_t out_len = sizeof(output); On 2014/03/04 09:53:24, Solis wrote: > nit: no need for this temp variable. Done. https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:29: void MaybeUpdatePacketAbsSendTimeExtn(char* data, int length, I added test cases for MaybeUpdatePacketAbsSendTimeExtn which also verifies auth tag function. On 2014/03/04 09:53:24, Solis wrote: > 1. There's logic in the function which isn't that straightforward. This should > be verified. > 2. The function to update the auth tag is currently untested. > > One strategy for testing would be to feed this function with the same packets > you have for the other tests and compare the output to some expected result > (should you decide to do this, I would suggest creating a helper > macro/function/template). > > Another strategy is to expose the auth tag update function here so it can be > tested separately. > > On 2014/03/04 00:15:46, mallinath2 wrote: > > I am not sure how can I test this method in particular. As we can't test any > > thing deterministically in this method. Both timestamp and HMAC will vary. > > That's why I had to expose below two methods and test those instead. As you > can > > see the major change is around parsing of the packet. > > > > On 2014/03/03 10:22:29, Solis wrote: > > > Why aren't there any tests for this function? > > >
https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:113: MaybeUpdateRtpAuthTag(start, rtp_length, options); The comment was in response to a thread concerning the name of MaybeUpdatePacketAbsSendTimeExtn(). I still think the long name could be shortened to something like ApplyPacketOptions() but that breaking it into two may be better. If breaking into several (two) functions, you could still check the packet options inside the functions, so at the call site(s) it would just be two separate calls in a row, if you need both. Another benefit is the functions that are exposed purely for testing right now could be hidden and just one more test be added for the case when the options are not set, i.e. simpler. On 2014/03/06 07:39:43, mallinath2 wrote: > You are talking about MaybeUpdatePacketAbsSendTimeExtn or MaybeUpdateRtpAuthTag. > > MaybeUpdateRtpAuthTag is maybe because it will return without doing anything if > key is null. But that's just error handling and i took maybe from that method. > > But the main reason I hid everything behind MaybeUpdatePacketAbsSendTimeExtn > because I don't want caller to do all the logic checking and decide what to do. > I still think it's reasonable thing to do in this case. > > On 2014/03/04 09:53:24, Solis wrote: > > It's a bit of swiss-army-knife type of function - it has multiple duties > > depending on the input data. This is rarely a good idea. Options that come to > > mind are: > > > > 1. Split it into multiple, well-defined functions that have a single purpose. > > This will make the logic easier to follow as well. > > 2. Use an even more generic name such as "UpdateFromPacketOptions", > > "ApplyPacketOptions", etc. > > > > On 2014/03/04 00:15:46, mallinath2 wrote: > > > I don't have better options than > > > MaybeUpdatePacketAbsSendTimeExtnaAndRtpAuthTag() :). Any other suggestions? > > > > > > On 2014/03/03 10:22:29, Solis wrote: > > > > It seems MaybeUpdatePacketAbsSendTimeExtn() should be called something > else, > > > > since one of its primary purposes seems to be setting the auth_tag > properly, > > > not > > > > just updating it in case we altered the packet contents? > > > > > > https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:195: // If there is a non negative sendtime extension id present in packet options, Note that the comment says "non negative" but the test is for != -1 https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_tcp.cc:528: // Update HMAC before writing padding bytes at the end. Comment is not true. AST will also be updated if present. https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:307: // Making sure we have't updated the HMAC. Should check that we haven't touched AST field as well. https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:314: TestMaybeUpdatePacketAbsSendTimeExtnAndRtpAuthTagWithOptions) { You should add a test that this method sets AST when id != -1 too. And ideally you should have another test for the case when both auth key and AST id are set.
https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/640001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:113: MaybeUpdateRtpAuthTag(start, rtp_length, options); I would like to get this CL in first before doing other major modifications. I will take up breaking later. I don't want to do any major changes and end up spending few more days in review, due timezone difference. I agree that ApplyPacketOptions is a better than MaybeUpdatePacketAbsSendTimeExtn. Incorporating that suggestion. Wdyt? On 2014/03/06 09:41:53, Solis wrote: > The comment was in response to a thread concerning the name of > MaybeUpdatePacketAbsSendTimeExtn(). I still think the long name could be > shortened to something like ApplyPacketOptions() but that breaking it into two > may be better. > > If breaking into several (two) functions, you could still check the packet > options inside the functions, so at the call site(s) it would just be two > separate calls in a row, if you need both. Another benefit is the functions that > are exposed purely for testing right now could be hidden and just one more test > be added for the case when the options are not set, i.e. simpler. > > On 2014/03/06 07:39:43, mallinath2 wrote: > > You are talking about MaybeUpdatePacketAbsSendTimeExtn or > MaybeUpdateRtpAuthTag. > > > > MaybeUpdateRtpAuthTag is maybe because it will return without doing anything > if > > key is null. But that's just error handling and i took maybe from that method. > > > > But the main reason I hid everything behind MaybeUpdatePacketAbsSendTimeExtn > > because I don't want caller to do all the logic checking and decide what to > do. > > I still think it's reasonable thing to do in this case. > > > > On 2014/03/04 09:53:24, Solis wrote: > > > It's a bit of swiss-army-knife type of function - it has multiple duties > > > depending on the input data. This is rarely a good idea. Options that come > to > > > mind are: > > > > > > 1. Split it into multiple, well-defined functions that have a single > purpose. > > > This will make the logic easier to follow as well. > > > 2. Use an even more generic name such as "UpdateFromPacketOptions", > > > "ApplyPacketOptions", etc. > > > > > > On 2014/03/04 00:15:46, mallinath2 wrote: > > > > I don't have better options than > > > > MaybeUpdatePacketAbsSendTimeExtnaAndRtpAuthTag() :). Any other > suggestions? > > > > > > > > On 2014/03/03 10:22:29, Solis wrote: > > > > > It seems MaybeUpdatePacketAbsSendTimeExtn() should be called something > > else, > > > > > since one of its primary purposes seems to be setting the auth_tag > > properly, > > > > not > > > > > just updating it in case we altered the packet contents? > > > > > > > > > > https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:195: // If there is a non negative sendtime extension id present in packet options, OK, -1 is also negative :). On 2014/03/06 09:41:53, Solis wrote: > Note that the comment says "non negative" but the test is for != -1 https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_tcp.cc:528: // Update HMAC before writing padding bytes at the end. comment is still true, I just not mentioned AST in it. The reason i mentioned only HMAC is, we may calculate different HMAC if we write padding bytes. On 2014/03/06 09:41:53, Solis wrote: > Comment is not true. AST will also be updated if present. https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:307: // Making sure we have't updated the HMAC. On 2014/03/06 09:41:53, Solis wrote: > Should check that we haven't touched AST field as well. Done. https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:314: TestMaybeUpdatePacketAbsSendTimeExtnAndRtpAuthTagWithOptions) { On 2014/03/06 09:41:53, Solis wrote: > You should add a test that this method sets AST when id != -1 too. > And ideally you should have another test for the case when both auth key and AST > id are set. Done.
LGTM, but please fix the last few comments. https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:195: // If there is a non negative sendtime extension id present in packet options, Remember that you're not writing comments for yourself. You're writing comments for someone who will have to understand this code later on. That person will wonder why the code does not do what the comment says. The best code can be written *without* comments and still be easy to understand. On 2014/03/06 16:18:13, mallinath2 wrote: > OK, -1 is also negative :). > On 2014/03/06 09:41:53, Solis wrote: > > Note that the comment says "non negative" but the test is for != -1 > https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_tcp.cc:528: // Update HMAC before writing padding bytes at the end. Don't you think that is misleading? Remove the comment. On 2014/03/06 16:18:13, mallinath2 wrote: > comment is still true, I just not mentioned AST in it. The reason i mentioned > only HMAC is, we may calculate different HMAC if we write padding bytes. > > On 2014/03/06 09:41:53, Solis wrote: > > Comment is not true. AST will also be updated if present. > https://codereview.chromium.org/159353002/diff/730009/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/730009/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:369: EXPECT_NE(0, memcmp(&rtp_packet[kAstIndexInRtpMsg], Note that this test currently is flaky. You can fix by making it possible to supply a clock to the AST update function, or by checking the time here and waiting a short time if the resulting time would be close to 0.
https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:195: // If there is a non negative sendtime extension id present in packet options, I am not sure you saw my modified comment. On 2014/03/06 16:36:28, Solis wrote: > Remember that you're not writing comments for yourself. You're writing comments > for someone who will have to understand this code later on. That person will > wonder why the code does not do what the comment says. The best code can be > written *without* comments and still be easy to understand. > > On 2014/03/06 16:18:13, mallinath2 wrote: > > OK, -1 is also negative :). > > On 2014/03/06 09:41:53, Solis wrote: > > > Note that the comment says "non negative" but the test is for != -1 > > > https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_tcp.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_tcp.cc:528: // Update HMAC before writing padding bytes at the end. On 2014/03/06 16:36:28, Solis wrote: > Don't you think that is misleading? Remove the comment. > > On 2014/03/06 16:18:13, mallinath2 wrote: > > comment is still true, I just not mentioned AST in it. The reason i mentioned > > only HMAC is, we may calculate different HMAC if we write padding bytes. > > > > On 2014/03/06 09:41:53, Solis wrote: > > > Comment is not true. AST will also be updated if present. > > > Done. https://codereview.chromium.org/159353002/diff/730009/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_unittest.cc (right): https://codereview.chromium.org/159353002/diff/730009/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_unittest.cc:369: EXPECT_NE(0, memcmp(&rtp_packet[kAstIndexInRtpMsg], On 2014/03/06 16:36:28, Solis wrote: > Note that this test currently is flaky. You can fix by making it possible to > supply a clock to the AST update function, or by checking the time here and > waiting a short time if the resulting time would be close to 0. Done.
https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/159353002/diff/740001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:195: // If there is a non negative sendtime extension id present in packet options, Oops, sorry. I must be using this diff tool wrong - when I go to comments I don't see them inline with the latest rev. :/ On 2014/03/07 04:02:37, mallinath2 wrote: > I am not sure you saw my modified comment. > > On 2014/03/06 16:36:28, Solis wrote: > > Remember that you're not writing comments for yourself. You're writing > comments > > for someone who will have to understand this code later on. That person will > > wonder why the code does not do what the comment says. The best code can be > > written *without* comments and still be easy to understand. > > > > On 2014/03/06 16:18:13, mallinath2 wrote: > > > OK, -1 is also negative :). > > > On 2014/03/06 09:41:53, Solis wrote: > > > > Note that the comment says "non negative" but the test is for != -1 > > > > > > https://codereview.chromium.org/159353002/diff/790001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/790001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:37: uint64 time_in_sec); I'm sorry, but "time_in_sec" is wrong. This value is poked directly into the ast field so it is definitely not in seconds. Maybe it's better to call it abs_send_time and make it a uint32.
https://codereview.chromium.org/159353002/diff/790001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.h (right): https://codereview.chromium.org/159353002/diff/790001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.h:37: uint64 time_in_sec); That's very good point. Thanks. On 2014/03/07 09:39:42, Solis wrote: > I'm sorry, but "time_in_sec" is wrong. This value is poked directly into the ast > field so it is definitely not in seconds. Maybe it's better to call it > abs_send_time and make it a uint32.
Message was sent while issue was closed.
Committed patchset #28 manually as r256580 (presubmit successful). |
