Closed Bug 1395849 Opened 7 years ago Closed 7 years ago

G.722 audio codec broken in 56

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: pehrsons, Assigned: dminor)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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: → 1395853
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
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 :( ).
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)
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+
Rank: 15
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/b876f2c2f663
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1397872
" 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?
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)
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+
You need to log in before you can comment on or make changes to this bug.