Closed Bug 1047487 Opened 10 years ago Closed 10 years ago

Add support for G.722 to WebRTC

Categories

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

33 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ekr, Assigned: ekr)

Details

Attachments

(1 file, 2 obsolete files)

This is available in WebRTC. We need to:

1. Turn it on.
2. Enable it in the SDP.
Attached patch Add support for G.722. WIP. (obsolete) — Splinter Review
Assignee: nobody → ekr
Status: NEW → ASSIGNED
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)
Nils,

See:
https://github.com/mozilla/webrtc-landing/pull/6

Things seem OK in my tests.
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+
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.
Attached patch Add support for G.722 (obsolete) — Splinter Review
Removed G.722.1 and G.722.1c from the original patch.
Attachment #8466731 - Attachment is obsolete: true
Attachment #8470171 - Flags: review?(rjesup)
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+
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)
Attachment #8471099 - Flags: review?(rjesup) → review+
I don't see any new issues in the try run.
Keywords: checkin-needed
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.