G.722 audio codec broken in 56

RESOLVED FIXED in Firefox 56

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
15
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: pehrsons, Assigned: dminor)

Tracking

({regression})

56 Branch
mozilla57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

(URL)

Attachments

(1 attachment)

xpeng reported on dev-media that G.722 is broken in 56. I can verify that it works in 55 but not in 56 or 57. Use the sample at [1] to test.

I'm assuming it's fallout from the webrtc.org 57 update.


[1] https://webrtc.github.io/samples/src/content/peerconnection/audio/
I'm also filing a bug for adding rudimentary mochitests of our supported codecs.
See Also: → bug 1395853
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox57: --- → affected
(Assignee)

Comment 2

5 months ago
It looks like WEBRTC_CODEC_G722 is not set here [1] although a quick look at audio_coding.gypi and gyp.mozbuild makes it seem as though it should be.

[1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/rent_a_codec.cc#253
Assignee: nobody → dminor
Component: WebRTC: Signaling → WebRTC: Audio/Video

Comment 3

5 months ago
Just FYI we do have signaling unit tests to ensure using/signaling G722 works. But we don't have any tests ensuring audio works.

When reproduce this last night I noticed on about:webrtc no indication that RTP actually gets send or received (which also means a mochitest should/would have caught this easily :( ).
(Assignee)

Comment 4

4 months ago
Created attachment 8904573 [details] [diff] [review]
Add G.722 defines to gyp file

It appears that we may also not be properly enabling the RED codec based upon the lines just above this patch. If that is not expected, please let me know and I'll expand this patch to include RED as well.
Attachment #8904573 - Flags: review?(rjesup)
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED
Comment on attachment 8904573 [details] [diff] [review]
Add G.722 defines to gyp file

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

We don't normally use RED yet, so that's probably ok for now.
Attachment #8904573 - Flags: review?(rjesup) → review+

Updated

4 months ago
Duplicate of this bug: 1395024
Rank: 15
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/b876f2c2f663
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

4 months ago
Blocks: 1397872

Comment 9

4 months ago
" Target: mozilla57"   -  Will the modification be merged into the coming firefox 56 ?
(In reply to xpeng from comment #9)
> " Target: mozilla57"   -  Will the modification be merged into the coming
> firefox 56 ?

that just means it landed in 57.
Comment on attachment 8904573 [details] [diff] [review]
Add G.722 defines to gyp file

Approval Request Comment
[Feature/Bug causing the regression]: bug 1341285 (webrtc.org 57 update)

[User impact if declined]:  g.722 isn't available.  Various services webrtc interoperates with (like FreeSwitch and Asterisk) and conference mixers like Cisco Spark may use G.722

[Is this code covered by automated tests?]: A separate bug is adding mochitests (bug 1397872); probably that should be simultaneously uplifted (test-only patch).

[Has the fix been verified in Nightly?]: Not yet but will be before landing

[Needs manual test from QE? If yes, steps to reproduce]: Got to https://mozilla.github.io/webrtc-landing/pc_test.html.  Start a test with G722 required.  Unmute one of the large <video> elements to verify audio is flowing.

[List of other uplifts needed for the feature/fix]: bug 1397872

[Is the change risky?]: No.  This was simply left out in the 57 update

[Why is the change risky/not risky?]: see above.  There were no significant changes to g.722, we just weren't enabling it.

[String changes made/needed]: none.
Attachment #8904573 - Flags: approval-mozilla-beta?

Comment 12

4 months ago
I suggest that it shoule be merged into firefox 56 , or many service will do not work. because they negotiate for G722 but it does not work in fact if use firefox 56
Hi Andreas,
Can you help check if this issue was fixed in the latest nightly? Thanks.
Flags: needinfo?(apehrson)
(In reply to Gerry Chang [:gchang] from comment #13)
> Hi Andreas,
> Can you help check if this issue was fixed in the latest nightly? Thanks.

Gerry I verified that the issue is fixed in Nightly 57.0a1 (2017-09-08).
Flags: needinfo?(apehrson)
status-firefox-esr52: --- → unaffected
Version: unspecified → 56 Branch
Comment on attachment 8904573 [details] [diff] [review]
Add G.722 defines to gyp file

Fix a regression and was verified. Beta56+.
Attachment #8904573 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 16

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/46f03d8bdc41
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.