Closed
Bug 1047487
Opened 10 years ago
Closed 10 years ago
Add support for G.722 to WebRTC
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: ekr, Assigned: ekr)
Details
Attachments
(1 file, 2 obsolete files)
14.12 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
This is available in WebRTC. We need to: 1. Turn it on. 2. Enable it in the SDP.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ekr
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8466731 [details] [diff] [review] Add support for G.722. WIP. Review of attachment 8466731 [details] [diff] [review]: ----------------------------------------------------------------- Nils, i tested this using the command line tests. Any chance you could modify one of the landing pages to filter out non-722 and verify we get OK-sounding audio?
Attachment #8466731 -
Flags: feedback?(drno)
Assignee | ||
Comment 3•10 years ago
|
||
Nils, See: https://github.com/mozilla/webrtc-landing/pull/6 Things seem OK in my tests.
Comment 4•10 years ago
|
||
Comment on attachment 8466731 [details] [diff] [review] Add support for G.722. WIP. Review of attachment 8466731 [details] [diff] [review]: ----------------------------------------------------------------- Lgtm with nits. I'm assuming that the plumbing of the codec itself happens via ifdef's in the codec database. Test with modified pc_test.html, but no audio verification (yet). ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +3531,5 @@ > DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, > dcb_p->call_id, fname), codec); > payload_info->audio.packet_size = -1; > payload_info->audio.bitrate = -1; > + MOZ_ASSERT(0); NIT: Is this left over from testing, or do you want to assert that we are not encountering unknown codecs? ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +3234,5 @@ > + > + ASSERT_GE(a1_->GetPacketsSent(0), 40); > + ASSERT_GE(a2_->GetPacketsReceived(0), 40); > +} > + A little concerning to me is that this test according to its console output send only from a2->a1, but nothing a1->a2 although the SDPs indicate two way audio. I'm assuming this is some generic problem and not just specific to this new test. As we are getting more support for different codecs it would also be really helpful to be able to inspect the RTP layer and not simply rely on what the SDP tells us. Inspecting the actual en-/de-coder in use would even nicer, but is probably harder to implement. ::: media/webrtc/trunk/webrtc/modules/audio_coding/main/acm2/audio_coding_module.gypi @@ +50,5 @@ > + 'acm_g722.h', > + 'acm_g7221.cc', > + 'acm_g7221.h', > + 'acm_g7221c.cc', > + 'acm_g7221c.h', Your change in gyp.mozbuild activated g722 only, but not g722_1 or g722_1c. So you should probably remove the G7221 and G7221C files here (or activate these codec variations as well - but that should result in more changes to the tests as well then).
Attachment #8466731 -
Flags: feedback?(drno) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8466731 [details] [diff] [review] Add support for G.722. WIP. Review of attachment 8466731 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +3531,5 @@ > DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, > dcb_p->call_id, fname), codec); > payload_info->audio.packet_size = -1; > payload_info->audio.bitrate = -1; > + MOZ_ASSERT(0); This was intentional. If I am reading this correctly, this is what happens when we negotiate an unknown codec. Seems like an assert would be good there. ::: media/webrtc/trunk/webrtc/modules/audio_coding/main/acm2/audio_coding_module.gypi @@ +50,5 @@ > + 'acm_g722.h', > + 'acm_g7221.cc', > + 'acm_g7221.h', > + 'acm_g7221c.cc', > + 'acm_g7221c.h', Thanks.
Comment 6•10 years ago
|
||
Removed G.722.1 and G.722.1c from the original patch.
Attachment #8466731 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8470171 -
Flags: review?(rjesup)
Comment 7•10 years ago
|
||
Comment on attachment 8470171 [details] [diff] [review] Add support for G.722 Review of attachment 8470171 [details] [diff] [review]: ----------------------------------------------------------------- Verify what controls the priority of audio codecs and verify that the order in offers will be Opus, G.722, G.711, and that if multiple codecs are offered that the correct one is accepted. Add a test per comments or modify it to just offer multiple codecs and verify the answer uses G.722. I don't *need* to see it again, but will look if you want. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp @@ +400,5 @@ > int codecMask = 0; > codecMask |= VCM_CODEC_RESOURCE_G711; > codecMask |= VCM_CODEC_RESOURCE_OPUS; > //codecMask |= VCM_CODEC_RESOURCE_LINEAR; > + codecMask |= VCM_CODEC_RESOURCE_G722; what's controlling the priority of the codecs offered? ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +3212,5 @@ > + a1_->CreateOffer(options, OFFER_AUDIO, SHOULD_SENDRECV_AUDIO); > + a1_->SetLocal(TestObserver::OFFER, a1_->offer(), false); > + ParsedSDP sdpWrapper(a1_->offer()); > + sdpWrapper.ReplaceLine("m=audio", > + "m=audio 65375 RTP/SAVPF 9\r\n"); This only tests offers with G.722 only. We probably should validate that full offers select Opus, that offers with G.722 first and others included select G.722 normally, and that if G.722 is first and the receiver doesn't support it that a lower codec is selected. So, variant to do m=audio ... 9 xx 0 The full offer case going to Opus just means making sure other tests here check Opus was selected. The last case can't be tested easily, since SDP editing doesn't emulate codec preference at the offer side well
Attachment #8470171 -
Flags: review?(rjesup) → review+
Comment 8•10 years ago
|
||
Moved G722 in priority over G711. Added more tests for picking Opus or falling back to G711. Try run: https://tbpl.mozilla.org/?tree=Try&rev=f1e9f99b7bee
Attachment #8470171 -
Attachment is obsolete: true
Attachment #8471099 -
Flags: review?(rjesup)
Updated•10 years ago
|
Attachment #8471099 -
Flags: review?(rjesup) → review+
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c08146793d37
Flags: in-testsuite+
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c08146793d37
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•