Add rtppacketuil.h/cc

Add ApplyPacketOptions()

When libjingle is compied with ENABLE_EXTERNAL_AUTH the sending socket
needs to update RTP header in order for the outgoing packet to be
valid. The corresponding code was in chromium in
content/browser/renderer_host/p2p/socket_host.cc and it was impossible
to reuse it anywhere else. This CL moves this code to
talk/media/base/rtputils.h/cc, so it can be used outside of chrome.

BUG=crbug.com/547158
R=pthatcher@webrtc.org

Committed: https://crrev.com/dc305db0598b81fb01fc7891a5e29c422593ff4a
Cr-Commit-Position: refs/heads/master@{#11261}

[Sergey Ulanov, 2016-01-12 19:15:06]

Patchset #2 (id:20001) has been deleted

[Sergey Ulanov, 2016-01-12 19:15:30]

sergeyu@chromium.org changed reviewers:
+ pthatcher@webrtc.org

[Sergey Ulanov, 2016-01-12 19:17:07]

Please note that this is the same code that existed in chromium before. There
are no functional changes. The only significant change is that now
rtc::ComputeHmac() is used to compute HMAC instead of chromium's crypto::HMAC.

[pthatcher1, 2016-01-13 23:21:33]

I know this code is exactly the same, but it doesn't quite fit, since there's so
much non-RTP code in it, and since it overlaps with the existing rtputils.cc.  I
think we need to make some changes to merge this in correctly.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
File webrtc/p2p/base/rtppacketutil.cc (right):

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:26: const size_t kMinRtpHeaderLength = 12;
There's a lot of overlap between this and rtputils.cc

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:35: // Fake auth tag written by the render
process if external authentication is
Does this still make sense being here?  It doesn't make sense having it refer to
"render process".

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:53: }
The above are not RTP, and so seem awkward being in rtputils/rtppacketutils.cc.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:88: }
It seems like it would be better to pass in the time rather than make this file
depend on rtc::TimeMicros().

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:97: void UpdateRtpAuthTag(char* rtp, size_t
length, const PacketOptions& options) {
It seems like the key and len should be passed in instead of a PacketOptions. 
PacketOptions is strange dependency for an rtputils class.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:144: }
This doesn't seem to belong in an rtputils file either.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:208: uint32_t abs_send_time) {
This seems like it should be UpdateRtpPacket, with time, key and length passed
in, instead of taking a PacketOptions.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:253: size_t* rtp_packet_length) {
This is kind of a funny function.  "Find an RTP packet in this random packet". 
Is that really for something in rtputils? It seems like it would belong more in
STUN or TURN specific code.

[Sergey Ulanov, 2016-01-14 05:43:50]

Description was changed from

==========
Add rtppacketuil.h/cc

When libjingle is compied with ENABLE_EXTERNAL_AUTH the sending socket
needs to update RTP header in order for the outgoing packet to be
valid. The corresponding code was in chromium in
content/browser/renderer_host/p2p/socket_host.cc and it was impossible
to reuse it anywhere else. This CL moves this code to
webrtc/p2p/base/rtppacketuil.h/cc, so it can be used outside of chrome.

BUG=crbug.com/547158
==========

to

==========
Add ApplyPacketOptions()

When libjingle is compied with ENABLE_EXTERNAL_AUTH the sending socket
needs to update RTP header in order for the outgoing packet to be
valid. The corresponding code was in chromium in
content/browser/renderer_host/p2p/socket_host.cc and it was impossible
to reuse it anywhere else. This CL moves this code to
talk/media/base/rtputils.h/cc, so it can be used outside of chrome.

BUG=crbug.com/547158
==========

[Sergey Ulanov, 2016-01-14 06:02:21]

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
File webrtc/p2p/base/rtppacketutil.cc (right):

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:26: const size_t kMinRtpHeaderLength = 12;
On 2016/01/13 23:21:33, pthatcher1 wrote:
> There's a lot of overlap between this and rtputils.cc

I didn't know about rtputils.cc. Move all the code there.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:35: // Fake auth tag written by the render
process if external authentication is
On 2016/01/13 23:21:33, pthatcher1 wrote:
> Does this still make sense being here?  It doesn't make sense having it refer
to
> "render process".

Updated the comment

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:53: }
On 2016/01/13 23:21:33, pthatcher1 wrote:
> The above are not RTP, and so seem awkward being in
rtputils/rtppacketutils.cc.

Moved to turnutils.cc

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:88: }
On 2016/01/13 23:21:33, pthatcher1 wrote:
> It seems like it would be better to pass in the time rather than make this
file
> depend on rtc::TimeMicros().

Done.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:97: void UpdateRtpAuthTag(char* rtp, size_t
length, const PacketOptions& options) {
On 2016/01/13 23:21:33, pthatcher1 wrote:
> It seems like the key and len should be passed in instead of a PacketOptions. 
> PacketOptions is strange dependency for an rtputils class.

Updated it to pass rtc::PacketTimeUpdateParams. There are 4 separate values that
would need to be passed without this struct.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:144: }
On 2016/01/13 23:21:33, pthatcher1 wrote:
> This doesn't seem to belong in an rtputils file either.

Done.

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:208: uint32_t abs_send_time) {
On 2016/01/13 23:21:33, pthatcher1 wrote:
> This seems like it should be UpdateRtpPacket, with time, key and length passed
> in, instead of taking a PacketOptions.

Changed it to use rtc::PacketTimeUpdateParams

https://codereview.chromium.org/1578323002/diff/1/webrtc/p2p/base/rtppacketut...
webrtc/p2p/base/rtppacketutil.cc:253: size_t* rtp_packet_length) {
On 2016/01/13 23:21:33, pthatcher1 wrote:
> This is kind of a funny function.  "Find an RTP packet in this random packet".

> Is that really for something in rtputils? It seems like it would belong more
in
> STUN or TURN specific code.

Moved to turnutils.cc and renamed to UnwrapTurnPacket()

[Sergey Ulanov, 2016-01-14 16:31:52]

Patchset #3 (id:60001) has been deleted

[pthatcher1, 2016-01-14 19:34:03]

lgtm, with nits

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
File talk/media/base/rtputils.cc (right):

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
talk/media/base/rtputils.cc:31: #include "webrtc/base/asyncpacketsocket.h"
Dependency on a socket is weird for an rtputils class.  

But it's also weird that webrtc/base/asyncpacketsocket.h has RTP-specific code
in it. 

Can you at least put a comment about this an a TODO to find a better home for
PacketTimeUpdateParams?

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputils.h
File talk/media/base/rtputils.h (right):

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
talk/media/base/rtputils.h:83: // Helper method which updates absolute send time
extension if present.
updates absolute send time => updates the absolute send time

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
talk/media/base/rtputils.h:89: // Applies specified |options| to the packet. It
updates absolute send time
Same here

[Sergey Ulanov, 2016-01-14 20:13:06]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-01-14 20:13:06]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.chromium.org/1578323002/#ps100001
(title: " ")

[Sergey Ulanov, 2016-01-14 20:13:10]

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
File talk/media/base/rtputils.cc (right):

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
talk/media/base/rtputils.cc:31: #include "webrtc/base/asyncpacketsocket.h"
On 2016/01/14 19:34:02, pthatcher1 wrote:
> Dependency on a socket is weird for an rtputils class.  
> 
> But it's also weird that webrtc/base/asyncpacketsocket.h has RTP-specific code
> in it. 
> 
> Can you at least put a comment about this an a TODO to find a better home for
> PacketTimeUpdateParams?

Done.

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputils.h
File talk/media/base/rtputils.h (right):

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
talk/media/base/rtputils.h:83: // Helper method which updates absolute send time
extension if present.
On 2016/01/14 19:34:02, pthatcher1 wrote:
> updates absolute send time => updates the absolute send time

Done.

https://codereview.chromium.org/1578323002/diff/80001/talk/media/base/rtputil...
talk/media/base/rtputils.h:89: // Applies specified |options| to the packet. It
updates absolute send time
On 2016/01/14 19:34:02, pthatcher1 wrote:
> Same here

Done.

[commit-bot: I haz the power, 2016-01-14 20:13:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578323002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578323002/100001

[commit-bot: I haz the power, 2016-01-14 20:26:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 20:26:27]

Try jobs failed on following builders:
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/11871)

[Sergey Ulanov, 2016-01-14 20:36:08]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-01-14 20:36:08]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1578323002/#ps120001 (title:
" ")

[commit-bot: I haz the power, 2016-01-14 20:36:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578323002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578323002/120001

[commit-bot: I haz the power, 2016-01-14 20:43:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 20:43:38]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/10320)

[Sergey Ulanov, 2016-01-14 21:51:50]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-01-14 21:51:50]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.chromium.org/1578323002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-01-14 21:51:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578323002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578323002/140001

[commit-bot: I haz the power, 2016-01-14 23:52:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 23:52:22]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Sergey Ulanov, 2016-01-15 01:15:09]

Description was changed from

==========
Add ApplyPacketOptions()

When libjingle is compied with ENABLE_EXTERNAL_AUTH the sending socket
needs to update RTP header in order for the outgoing packet to be
valid. The corresponding code was in chromium in
content/browser/renderer_host/p2p/socket_host.cc and it was impossible
to reuse it anywhere else. This CL moves this code to
talk/media/base/rtputils.h/cc, so it can be used outside of chrome.

BUG=crbug.com/547158
==========

to

==========
Add ApplyPacketOptions()

When libjingle is compied with ENABLE_EXTERNAL_AUTH the sending socket
needs to update RTP header in order for the outgoing packet to be
valid. The corresponding code was in chromium in
content/browser/renderer_host/p2p/socket_host.cc and it was impossible
to reuse it anywhere else. This CL moves this code to
talk/media/base/rtputils.h/cc, so it can be used outside of chrome.

BUG=crbug.com/547158
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/dc305db0598b81fb01fc7891a...
==========

[commit-bot: I haz the power, 2016-01-15 01:15:10]

Description was changed from

==========
Add ApplyPacketOptions()

When libjingle is compied with ENABLE_EXTERNAL_AUTH the sending socket
needs to update RTP header in order for the outgoing packet to be
valid. The corresponding code was in chromium in
content/browser/renderer_host/p2p/socket_host.cc and it was impossible
to reuse it anywhere else. This CL moves this code to
talk/media/base/rtputils.h/cc, so it can be used outside of chrome.

BUG=crbug.com/547158
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/dc305db0598b81fb01fc7891a...
==========

to

==========
Add ApplyPacketOptions()

When libjingle is compied with ENABLE_EXTERNAL_AUTH the sending socket
needs to update RTP header in order for the outgoing packet to be
valid. The corresponding code was in chromium in
content/browser/renderer_host/p2p/socket_host.cc and it was impossible
to reuse it anywhere else. This CL moves this code to
talk/media/base/rtputils.h/cc, so it can be used outside of chrome.

BUG=crbug.com/547158
R=pthatcher@webrtc.org

Committed: https://crrev.com/dc305db0598b81fb01fc7891a5e29c422593ff4a
Cr-Commit-Position: refs/heads/master@{#11261}
==========

[Sergey Ulanov, 2016-01-15 01:15:11]

Committed patchset #6 (id:140001) manually as
dc305db0598b81fb01fc7891a5e29c422593ff4a (presubmit successful).

[commit-bot: I haz the power, 2016-01-15 01:15:11]

Patchset 6 (id:??) landed as
https://crrev.com/dc305db0598b81fb01fc7891a5e29c422593ff4a
Cr-Commit-Position: refs/heads/master@{#11261}