792325 - Import libsrtp from alder to m-c

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

We need to import libsrtp.  Since this is an external 3rd-party library, preference would be to import it directly and not via webrtc.org (counter-argument is that Google has been patching libsrtp in their repo, at least a little, and it guarantees we're roughly in sync).

If imported, the obvious place would be netwerk/srtp

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Build libsrtp in mozilla's system, and update script

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 2: external import of libsrtp from CVS Wed Sep 19 01:02:00 EDT 2012

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Those patches assume independent import
Note that due to -pedantic, we get warnings from srtp_priv.h with that that we don't with the gyp version.

This will need a set of patches-for-upstreaming (which I can do as a libsrtp maintainer)

Comment 4

[Timothy B. Terriberry (:derf)]

(In reply to Randell Jesup [:jesup] from comment #3)
> This will need a set of patches-for-upstreaming (which I can do as a libsrtp
> maintainer)

In the short term, we can just strip the -pedantic flag. See bug 786372.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 1: Build libsrtp in mozilla's system, and update script

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 3: patches from jingle's archive of libsrtp for upstreaming (or replacing with an upstream update)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 662433 [details] [diff] [review]
Patch 2: external import of libsrtp from CVS Wed Sep 19 01:02:00 EDT 2012

Straight import from srtp CVS repo; patches for upstreaming are on another patch

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 662542 [details] [diff] [review]
Patch 3: patches from jingle's archive of libsrtp for upstreaming (or replacing with an upstream update)

Patches for upstreaming - these make it match the webrtc.org archive exactly; I plan to update the upstream from these patches and the list of patches Android is carrying against libsrtp (see the dependent bug here for a link to the ~10 patches they have, some of which are identical to pieces of this patch - but not all are mirrored in this patch).  Some of those also indicate the bugs behind some of replay-check changes here (mostly having to do with wraparounds of sequence numbers)

As I'm a libsrtp maintainer, I'll be doing the upstreaming and then we'll drop this patch when we import the update.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 662537 [details] [diff] [review]
Patch 1: Build libsrtp in mozilla's system, and update script

Review of attachment 662537 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/srtp/src/Makefile.in
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEPTH     = ../../..

DEPTH = @DEPTH@

@@ +5,5 @@
> +
> +DEPTH     = ../../..
> +topsrcdir = @top_srcdir@
> +srcdir    = @srcdir@
> +VPATH     = \

Just use one space before and after the =.

@@ +29,5 @@
> +
> +EXPORTS_NAMESPACES = mozilla/net
> +
> +ifdef GNU_CC
> +OS_CFLAGS := $(filter-out -pedantic,$(OS_CFLAGS))

We should probably add a makefile var for this, like NO_PEDANTIC := 1

@@ +32,5 @@
> +ifdef GNU_CC
> +OS_CFLAGS := $(filter-out -pedantic,$(OS_CFLAGS))
> +endif
> +
> +CSRCS = \

:=

@@ +103,5 @@
> +DEFINES += \
> +  -DHAVE_WINSOCK2_H=1 \
> +  -Dinline=__inline \
> +  $(NULL)
> +endif

The pile of defines here kind of sucks. Can we get better detection integrated upstream, or are we stuck with this?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 662542 [details] [diff] [review]
Patch 3: patches from jingle's archive of libsrtp for upstreaming (or replacing with an upstream update)

rs=biesi, I trust that these changes make sense though if you want me to do an actual review you'll have to explain them to me

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 662433 [details] [diff] [review]
Patch 2: external import of libsrtp from CVS Wed Sep 19 01:02:00 EDT 2012

rs=biesi

+++ b/netwerk/srtp/srtp_update.log

Should probably only have one entry in here

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/765f725ff419
https://hg.mozilla.org/integration/mozilla-inbound/rev/5691ca0b22ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ec63e18d1a

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

FYI, this was approved by gerv a while ago and the license is already in licenses.html

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/765f725ff419
https://hg.mozilla.org/mozilla-central/rev/5691ca0b22ba
https://hg.mozilla.org/mozilla-central/rev/50ec63e18d1a

Comment 15

[Bill Gianopoulos [:WG9s]]

Just my opinion, but this needs to be backed out of mozilla-central and tested on Alder to make sure it can be built on all the supported platforms before it relands on mozilla-central.