880879 - Update WebRTC.org code from stable branch 3.30

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #830247 +++

3.30 is what Google has been stabilizing for Chrome XX

Import is still largely by-hand due to mercurial patch-history size issues.

Comment 1

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

See Try at https://tbpl.mozilla.org/?tree=Try&rev=15d01d383ae3
Main patch is too large to upload.

Note: no updating of anything outside media/webrtc/trunk/webrtc (other than a few tweaks in media/webrtc/shared_libs.mk, etc)

Comment 2

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

Attachment: Rollup of changes previously applied to media/webrtc/trunk/webrtc

tested on Linux and Windows so far, green builds on all platforms

Comment 3

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

All green on Try.  libyuv will need a license patch (gerv), but the rest of it can land.  Need to figure out the right way and time...  Now, or at the work week?  Tempted to do it at the work week, in case I break any b2g stuff.

Comment 4

[Eric Rescorla (:ekr)]

+1 on doing it during work week

Comment 5

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

Attachment: Rollup of changes previously applied to media/webrtc/trunk/webrtc rs=derf

ready for review/landing.  Works on B2G too (with b2g webrtc patch queue)

Comment 6

[Timothy B. Terriberry (:derf)]

Comment on attachment 761208 [details] [diff] [review]
Rollup of changes previously applied to media/webrtc/trunk/webrtc rs=derf

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

rs=me with a couple of nits.

::: media/webrtc/trunk/webrtc/modules/audio_coding/neteq/neteq_defines.h
@@ +345,4 @@
>      #define NETEQ_32KHZ_WIDEBAND
>      #define NETEQ_G722_1C_CODEC
>      #define NETEQ_CELT_CODEC
> +    #define NETEQ_OPUS_CODEC

Pretty sure we don't need to define this four times, when it's defined right below upstream.

@@ +347,5 @@
>      #define NETEQ_CELT_CODEC
> +    #define NETEQ_OPUS_CODEC
> +
> +    /* hack in 48 kHz support */
> +    #define NETEQ_48KHZ_WIDEBAND

We don't need to "hack" this in anymore... it's also defined right below.

::: media/webrtc/trunk/webrtc/modules/audio_coding/neteq/packet_buffer.c
@@ +681,5 @@
> +        else if (codecID[i] == kDecoderOpus)
> +        {
> +            codecBytes = 15300; /* 240ms @ 510kbps (60ms frames) */
> +            codecBuffers = 30;  /* ?? Codec supports down to 2.5-60 ms frames */
> +        }

We already have a kDecoderOpus test above, because this has landed upstream. I don't think we need another.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/92fc4bf7a9e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5522d403c1

Comment 8

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/73b1a6ee8d12 for Android mochitest-4 failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=24040949&tree=Mozilla-Inbound

Comment 9

[Jason Smith [:jsmith]]

Probably should have mentioned this before we tried to land this on inbound, but we really should do the following before landing this:

1. Try run to ensure mochitests and crashtests still pass on Desktop and FxAndroid
2. Sanity testing with a try build on desktop and FxAndroid to make sure nothing seriously is broken with this import

Comment 10

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #9)
> Probably should have mentioned this before we tried to land this on inbound,
> but we really should do the following before landing this:
> 
> 1. Try run to ensure mochitests and crashtests still pass on Desktop and
> FxAndroid

Oh wait, saw above a try run was done. Hmm...did something else land recently that broke this?

> 2. Sanity testing with a try build on desktop and FxAndroid to make sure
> nothing seriously is broken with this import

Missed the part above on testing on Win & Linux. Although we probably should check FxAndroid as well.

Comment 11

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

With fixed merge:

https://tbpl.mozilla.org/?tree=Try&rev=0ed638def45d
and with the 1-char typo fix
https://tbpl.mozilla.org/?tree=Try&rev=11db932dcef2

and tried locally on android and desktop

Comment 12

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

Attachment: Rollup of changes previously applied to media/webrtc/trunk/webrtc rs=derf f=gcp

Comment 13

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

Comment on attachment 773121 [details] [diff] [review]
Rollup of changes previously applied to media/webrtc/trunk/webrtc rs=derf f=gcp

Carry forward derf's review, plus Android merge fixes from gcp reviewed by me, and generic rs=me as module owner for updating an external library.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6063eaf3633f
https://hg.mozilla.org/integration/mozilla-inbound/rev/edd376cd77bf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6063eaf3633f
https://hg.mozilla.org/mozilla-central/rev/edd376cd77bf

Comment 16

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

Attachment: re-land changes lost in the original merge of bug 880879 rs=jesup

In light of bug 896235, found that 1 patch (bug 832579) was missed in the merge of 3.30, and after reviewing all the changes to media/webrtc/trunk/webrtc line-by-line, found one additional change that was applied incorrectly (though with limited impact - two optional arguments were omitted in getting the recording device's name and GUID, leading to the GUID being internally generated).  Extending rubber-stamp as these patches had already landed and were simply missed and now match the equivalent files in Aurora/before this landed.

Comment 17

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

Per discussion with Tim, there's probably a TSAN bug with the upstream code here (ReceivedRTP/RTCPPacket() don't grab the rtp_rtcp_cs critical section when checking receiving_/receiving_rtcp_).  That exists with or without re-landing this change, so we'll file a separate bug on that and issue with upstream

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4c769b8baf

Derf looked at these changes, so extending rs=derf as well

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ca4c769b8baf