Update libsrtp to upstream 1.5.0

Update libsrtp to upstream 1.5.0

With this change, libsrtp can use OpenSSL crypto primitives, which is
enabled in the .gyp file if compiled against OpenSSL/BoringSSL. This
gives a nice speed improvement for SRTP as used by WebRTC. With my
test, relative CPU usage drops from ~36% each for SrtpProtect/-Unprotect
down to ~15% each.

I tried to not change the upstream code too much regarding code layout
(whitespaces, indent) while re-applying the Chromium changes.

Timings without the CL:
-------------------------------------------------------
$ ./out/Release/srtp_test_cipher_driver -t
cipher test driver
David A. McGrew
Cisco Systems, Inc.
timing null cipher throughput, key length 0:
msg len: 32	gigabits per second: 25.477707
msg len: 64	gigabits per second: 94.885100
msg len: 128	gigabits per second: 222.802437
msg len: 256	gigabits per second: 375.297783
msg len: 512	gigabits per second: 770.069562
msg len: 1024	gigabits per second: 1722.093757
msg len: 2048	gigabits per second: 3527.993109
timing aes integer counter mode throughput, key length 30:
msg len: 32	gigabits per second: 0.626706
msg len: 64	gigabits per second: 0.632999
msg len: 128	gigabits per second: 0.646651
msg len: 256	gigabits per second: 0.610196
msg len: 512	gigabits per second: 0.623500
msg len: 1024	gigabits per second: 0.648507
msg len: 2048	gigabits per second: 0.649695
timing aes integer counter mode throughput, key length 46:
msg len: 32	gigabits per second: 0.462445
msg len: 64	gigabits per second: 0.466739
msg len: 128	gigabits per second: 0.445170
msg len: 256	gigabits per second: 0.436243
msg len: 512	gigabits per second: 0.465987
msg len: 1024	gigabits per second: 0.472398
msg len: 2048	gigabits per second: 0.474392

Timings with the CL:
-------------------------------------------------------
$ ./out/Release/srtp_test_cipher_driver -t
cipher test driver
David A. McGrew
Cisco Systems, Inc.
timing null cipher throughput, key length 0:
msg len: 32	gigabits per second: 21.317345
msg len: 64	gigabits per second: 105.155063
msg len: 128	gigabits per second: 232.252211
msg len: 256	gigabits per second: 427.022519
msg len: 512	gigabits per second: 840.032814
msg len: 1024	gigabits per second: 1907.334109
msg len: 2048	gigabits per second: 3803.156917
timing AES-128 counter mode using openssl throughput, key length 30:
msg len: 32	gigabits per second: 0.406409
msg len: 64	gigabits per second: 0.520612
msg len: 128	gigabits per second: 1.563435
msg len: 256	gigabits per second: 2.044086
msg len: 512	gigabits per second: 2.457887
msg len: 1024	gigabits per second: 2.688393
msg len: 2048	gigabits per second: 2.807566
timing AES-256 counter mode using openssl throughput, key length 46:
msg len: 32	gigabits per second: 0.320314
msg len: 64	gigabits per second: 0.392381
msg len: 128	gigabits per second: 1.230553
msg len: 256	gigabits per second: 1.561504
msg len: 512	gigabits per second: 1.834276
msg len: 1024	gigabits per second: 2.010586
msg len: 2048	gigabits per second: 2.109853
timing AES-128 GCM using openssl throughput, key length 28:
msg len: 32	gigabits per second: 0.187080
msg len: 64	gigabits per second: 0.283558
msg len: 128	gigabits per second: 0.650450
msg len: 256	gigabits per second: 0.900447
msg len: 512	gigabits per second: 1.109531
msg len: 1024	gigabits per second: 1.253113
msg len: 2048	gigabits per second: 1.268890
timing AES-256 GCM using openssl throughput, key length 44:
msg len: 32	gigabits per second: 0.126774
msg len: 64	gigabits per second: 0.169385
msg len: 128	gigabits per second: 0.370561
msg len: 256	gigabits per second: 0.534079
msg len: 512	gigabits per second: 0.683315
msg len: 1024	gigabits per second: 0.779735
msg len: 2048	gigabits per second: 0.824227

BUG=328475

[fancycode, 2015-01-31 01:10:20]

ptal

[fancycode, 2015-02-06 10:14:34]

mail@joachim-bauch.de changed reviewers:
+ jiayl@chromium.org, juberti@chromium.org, sergeyu@chromium.org,
tvsriram@google.com

[fancycode, 2015-02-06 10:15:38]

On 2015/01/31 01:10:20, fancycode wrote:
> ptal

ping?

[jiayl, 2015-02-06 16:56:08]

On 2015/02/06 10:15:38, fancycode wrote:
> On 2015/01/31 01:10:20, fancycode wrote:
> > ptal
> 
> ping?

lgtm rubber stamp

[juberti2, 2015-02-06 19:08:59]

juberti@chromium.org changed reviewers:
+ davidben@chromium.org
- sergeyu@chromium.org

[juberti2, 2015-02-06 19:21:14]

On 2015/02/06 16:56:08, jiayl wrote:
> On 2015/02/06 10:15:38, fancycode wrote:
> > On 2015/01/31 01:10:20, fancycode wrote:
> > > ptal
> > 
> > ping?
> 
> lgtm rubber stamp

Can you upload a patchset with just the raw 1.5.0 changes? I'd like to be able
to see how much patching is still needed.

[fancycode, 2015-02-08 11:48:31]

New patchsets have been uploaded after l-g-t-m from jiayl@chromium.org

[fancycode, 2015-02-08 11:54:02]

On 2015/02/06 19:21:14, juberti2 wrote:
> Can you upload a patchset with just the raw 1.5.0 changes? I'd like to be able
> to see how much patching is still needed.

Updated CL to have raw 1.5.0 changes.

Modifications I did in my initial patchset:
- replaced 'inline' with 'INLINE'
- removed any "config.h" includes
- added copyright headers if missing
- re-added additional header check in 'srtp.c'
- disabled AES-192 if compiled against BoringSSL (not supported there)
- updated .gyp file / 'README.chromium'

[fancycode, 2015-02-16 15:06:44]

On 2015/02/08 11:54:02, fancycode wrote:
> On 2015/02/06 19:21:14, juberti2 wrote:
> > Can you upload a patchset with just the raw 1.5.0 changes? I'd like to be
able
> > to see how much patching is still needed.
> 
> Updated CL to have raw 1.5.0 changes.
> 
> Modifications I did in my initial patchset:
> - replaced 'inline' with 'INLINE'
> - removed any "config.h" includes
> - added copyright headers if missing
> - re-added additional header check in 'srtp.c'
> - disabled AES-192 if compiled against BoringSSL (not supported there)
> - updated .gyp file / 'README.chromium'

juberti2, did you get a chance to take a look yet?

[juberti2, 2015-02-18 18:54:30]

On 2015/02/16 15:06:44, fancycode wrote:
> On 2015/02/08 11:54:02, fancycode wrote:
> > On 2015/02/06 19:21:14, juberti2 wrote:
> > > Can you upload a patchset with just the raw 1.5.0 changes? I'd like to be
> able
> > > to see how much patching is still needed.
> > 
> > Updated CL to have raw 1.5.0 changes.
> > 
> > Modifications I did in my initial patchset:
> > - replaced 'inline' with 'INLINE'
> > - removed any "config.h" includes
> > - added copyright headers if missing
> > - re-added additional header check in 'srtp.c'
> > - disabled AES-192 if compiled against BoringSSL (not supported there)
> > - updated .gyp file / 'README.chromium'
> 
> juberti2, did you get a chance to take a look yet?

Can you make sure this info all is present in README.chromium?

Also, is it necessary to remove the config.h includes? I think the number of
patched files would be a lot less without those changes and the copyright header
tweaks (which would be nice to fix upstream).

[fancycode, 2015-02-18 20:39:18]

On 2015/02/18 18:54:30, juberti2 wrote:
> Can you make sure this info all is present in README.chromium?

Most of the entries were already present, I added the info about AES-192 being
removed with BoringSSL as that was the only new modification I added.

> Also, is it necessary to remove the config.h includes? I think the number of
> patched files would be a lot less without those changes and the copyright
header
> tweaks (which would be nice to fix upstream).

I tried to keep the diff to the previous version in Chromium as small as
possible, so I also removed the config.h includes (these were the only changes
in some of the files). If required I can take a look at keeping them and see if
I get it to compile.
Same applies to the copyright headers which I will try to push upstream in the
next days.

[juberti2, 2015-02-18 21:10:03]

On 2015/02/18 20:39:18, fancycode wrote:
> On 2015/02/18 18:54:30, juberti2 wrote:
> > Can you make sure this info all is present in README.chromium?
> 
> Most of the entries were already present, I added the info about AES-192 being
> removed with BoringSSL as that was the only new modification I added.
> 
> > Also, is it necessary to remove the config.h includes? I think the number of
> > patched files would be a lot less without those changes and the copyright
> header
> > tweaks (which would be nice to fix upstream).
> 
> I tried to keep the diff to the previous version in Chromium as small as
> possible, so I also removed the config.h includes (these were the only changes
> in some of the files). If required I can take a look at keeping them and see
if
> I get it to compile.
> Same applies to the copyright headers which I will try to push upstream in the
> next days.

I would like to keep this as close to upstream as possible, so if we can do some
tweaking either here or upstream to facilitate that, let's do that now.

Overall, I'd like to be able to take libsrtp drops more often, so reducing the
patching needed will make that process easier.

[fancycode, 2015-02-18 23:05:17]

On 2015/02/18 21:10:03, juberti2 wrote:
> I would like to keep this as close to upstream as possible, so if we can do
some
> tweaking either here or upstream to facilitate that, let's do that now.
> 
> Overall, I'd like to be able to take libsrtp drops more often, so reducing the
> patching needed will make that process easier.

Agreed. The CL is updated with some minimal changes against upstream 1.5.0:
- Replaced 'inline' with 'INLINE'.
- Use '_byteswap_*' functions to convert between host and network byte order on
Windows.
- AES-192 is disabled when compiled against BoringSSL.

I did some testing on Linux but don't have a Windows/Mac available.

The '_byteswap_*' change would not be required if 'winsock2.h' was included on
Windows. Was that intentionally not enabled in the existing gyp file?

I'll try to push the missing copyright headers and the check for BoringSSL
upstream, so they hopefully will be included in the next drop.

[fancycode, 2015-02-18 23:35:53]

On 2015/02/18 23:05:17, fancycode wrote:
> I'll try to push the missing copyright headers and the check for BoringSSL
> upstream, so they hopefully will be included in the next drop.

Pull requests are pending upstream now:
https://github.com/cisco/libsrtp/pull/93
https://github.com/cisco/libsrtp/pull/94

[juberti2, 2015-02-18 23:42:55]

We probably want to try to upstream INLINE too; I imagine others will want to
compile on VS2012.

https://codereview.chromium.org/889083003/diff/40001/README.chromium
File README.chromium (left):

https://codereview.chromium.org/889083003/diff/40001/README.chromium#oldcode22
README.chromium:22: Adding additional RTP header check
Did you mean to remove this? I think this is fine to retain (since it only
touches one file), although we should try to upstream it.

[fancycode, 2015-02-19 00:00:34]

On 2015/02/18 23:42:55, juberti2 wrote:
> We probably want to try to upstream INLINE too; I imagine others will want to
> compile on VS2012.

They are checking for 'inline' in the 'configure' script and have a custom
'config.h' for Windows where 'inline' #defines to '__inline'. Similar to what
Chromium is doing in the gyp file with 'INLINE'.

We could however add the #define for Windows (i.e. MSVS and non C++) to our
custom 'config.h' instead and get rid of 'INLINE'. Should I change this, too?

> https://codereview.chromium.org/889083003/diff/40001/README.chromium
> File README.chromium (left):
> 
> https://codereview.chromium.org/889083003/diff/40001/README.chromium#oldcode22
> README.chromium:22: Adding additional RTP header check
> Did you mean to remove this? I think this is fine to retain (since it only
> touches one file), although we should try to upstream it.

Sorry, didn't commit this before uploading the CL. It's in again now.

[fancycode, 2015-02-19 00:39:24]

Additional header check also provided upstream:
https://github.com/cisco/libsrtp/pull/95

[fancycode, 2015-02-19 14:36:20]

All pull requests (additional RTP header check, support for BoringSSL and
missing copyright headers) have been accepted upstream and will be included in
the next release.

I would suggest to #define 'inline' to '__inline' in our custom 'config.h' for
MSVS, so the 'INLINE' changes are no longer needed and we should be able to use
stock libsrtp without any local modifications. What do you think?

[juberti2, 2015-02-19 16:56:44]

On 2015/02/19 14:36:20, fancycode wrote:
> All pull requests (additional RTP header check, support for BoringSSL and
> missing copyright headers) have been accepted upstream and will be included in
> the next release.
> 
> I would suggest to #define 'inline' to '__inline' in our custom 'config.h' for
> MSVS, so the 'INLINE' changes are no longer needed and we should be able to
use
> stock libsrtp without any local modifications. What do you think?

SGTM

[fancycode, 2015-02-19 17:51:00]

On 2015/02/19 16:56:44, juberti2 wrote:
> On 2015/02/19 14:36:20, fancycode wrote:
> > All pull requests (additional RTP header check, support for BoringSSL and
> > missing copyright headers) have been accepted upstream and will be included
in
> > the next release.
> > 
> > I would suggest to #define 'inline' to '__inline' in our custom 'config.h'
for
> > MSVS, so the 'INLINE' changes are no longer needed and we should be able to
> use
> > stock libsrtp without any local modifications. What do you think?
> 
> SGTM

Done and updated to just-released libsrtp 1.5.1. Unfortunately the BoringSSL
support didn't make it to that upstream release, so I patched it manually here.
Ran "libjingle_peerconnection_unittest" on Linux for testing.

"srtpfilter.cc" from WebRTC uses "be64_to_cpu" from libsrtp which is not exposed
by default, so this requires a patch in libsrtp (done in the latest patchset).
We should think about adding "be64_to_cpu" in "srtpfilter.cc" to avoid having to
patch libsrtp for it.

[juberti2, 2015-02-19 19:44:48]

lgtm

jiayl, can you land this?

[jiayl, 2015-02-19 19:48:25]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2015-02-19 19:51:40]

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

[commit-bot: I haz the power, 2015-02-19 19:51:42]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[jiayl, 2015-02-19 20:18:36]

jiayl@chromium.org changed reviewers:
+ kjellander@chromium.org

[jiayl, 2015-02-19 20:18:37]

I failed to land using "git cl land" with this error:

fatal: remote error: Git access forbidden
Failed to push. If this persists, please file a bug.

Adding Henrik for help.

[kjellander_chromium, 2015-02-22 18:16:56]

On 2015/02/19 20:18:37, jiayl wrote:
> I failed to land using "git cl land" with this error:
> 
> fatal: remote error: Git access forbidden
> Failed to push. If this persists, please file a bug.
> 
> Adding Henrik for help.

(pasting the same text I wrote in another e-mail thread):

It seems you're using a Git mirror for a SVN repo (which can identified by the
git-svn-id entries in the commit messages at
https://chromium.googlesource.com/chromium/deps/libsrtp).
Those have to be submitted using "git cl dcommit". When you attempt "git cl
land" you should get an error message like this:

 # git cl land
This appears to be an SVN repository.
Are you sure you didn't mean 'git cl dcommit'?
[Press enter to push or ctrl-C to quit]

Didn't you get that? Either way you probably also need to run "git auto-svn" to
setup the SVN remote for the repo first. Then commit using "git cl dcommit".
I'm not sure it's widely known that all the SVN repos with Git-mirrors needs
this - maybe the error message for git cl land should be improved covering that
as well?

[jiayl, 2015-02-23 19:17:28]

Landed in https://codereview.chromium.org/936663005/