Update WebRTC.org code from stable branch 3.30

RESOLVED FIXED in mozilla25

Status

()

Core
WebRTC
P1
normal
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Blocks: 1 bug)

24 Branch
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
+++ 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.
(Assignee)

Comment 1

5 years ago
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)
(Assignee)

Comment 2

5 years ago
Created attachment 759993 [details] [diff] [review]
Rollup of changes previously applied to media/webrtc/trunk/webrtc

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

Comment 3

5 years ago
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

5 years ago
+1 on doing it during work week
(Assignee)

Comment 5

5 years ago
Created attachment 761208 [details] [diff] [review]
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)
(Assignee)

Updated

5 years ago
Attachment #759993 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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
(Assignee)

Comment 12

5 years ago
Created attachment 773121 [details] [diff] [review]
Rollup of changes previously applied to media/webrtc/trunk/webrtc rs=derf f=gcp
(Assignee)

Updated

5 years ago
Attachment #761208 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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+

Updated

5 years ago
Depends on: 892102
(Assignee)

Comment 16

5 years ago
Created attachment 780596 [details] [diff] [review]
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.
(Assignee)

Comment 17

5 years ago
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
(Assignee)

Comment 18

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

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

Updated

5 years ago
Duplicate of this bug: 896235
Depends on: 899240

Updated

5 years ago
Depends on: 947862
Blocks: 818631
You need to log in before you can comment on or make changes to this bug.