Closed Bug 880879 Opened 11 years ago Closed 11 years ago

Update WebRTC.org code from stable branch 3.30

Categories

(Core :: WebRTC, defect, P1)

24 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Whiteboard: [webrtc][blocking-webrtc-])

Attachments

(2 files, 2 obsolete files)

+++ 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.
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)
tested on Linux and Windows so far, green builds on all platforms
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.
+1 on doing it during work week
ready for review/landing.  Works on B2G too (with b2g webrtc patch queue)
Attachment #759993 - Attachment is obsolete: true
Attachment #761208 - Flags: review?(tterribe)
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.
Attachment #761208 - Flags: review?(tterribe) → review+
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
(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.
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
Attachment #761208 - Attachment is obsolete: true
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.
Attachment #773121 - Flags: review+
Depends on: 892102
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.
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4c769b8baf

Derf looked at these changes, so extending rs=derf as well
Depends on: 899240
Depends on: 947862
You need to log in before you can comment on or make changes to this bug.