783881 - Negotiate Opus in SDP

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

Implement
http://tools.ietf.org/html/draft-spittka-payload-rtp-opus-01

Also adjust vcm to support.

Comment 1

[enda mannion (:emannion)]

Attachment: Support OPUS audio codec in SDP

So fat I am offering all fmtp options, let me know if I should turn some off or make them more configurable.

I am getting all options also but so far not using them, if some are needed to pass through VCM let me know also.



m=audio 62627 RTP/SAVPF 109 101
c=IN IP4 192.0.2.1
a=rtpmap:109 opus/48000
a=fmtp:109 maxaveragebitrate=40000;usedtx=0;stereo=0;useinbandfec=0;maxcodedaudiobandwidth=fb;cbr=0
a=ptime:20
a=maxptime:120

Comment 2

[Timothy B. Terriberry (:derf)]

Comment on attachment 653982 [details] [diff] [review]
Support OPUS audio codec in SDP

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

f+ with one change and one suggestion.

(In reply to enda mannion (:emannion) from comment #1)
> maxaveragebitrate=40000;usedtx=0;stereo=0;useinbandfec=0;
> maxcodedaudiobandwidth=fb;cbr=0
> a=ptime:20
> a=maxptime:120

Right now we don't have any way to pass these to VoiceEngine, and if we did, it doesn't have any way to use them. But we should probably respect things like minptime, maxptime, and maxcodedaudiobandwidth once we do (we're not required to by the spec, but it's useful to do so).

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
@@ +6277,5 @@
> +    }
> +
> +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> +    if (attr_p == NULL) {
> +        if (sdp_p->debug_flag[SDP_DEBUG_ERRORS]) {

These are all optional parameters, so we shouldn't error out if they're not found. Enda said on IRC he'll add a config item and have them off by default for now, since they're not being passed anywhere yet.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
@@ +157,5 @@
> +    u32                       maxaveragebitrate;
> +    u16                       usedtx;
> +    u16                       stereo;
> +    u16                       useinbandfec;
> +    char                      maxcodedaudiobandwidth[SDP_MAX_STRING_LEN+1];

You've got a nice max_coded_audio_bandwidth_ enum below... it might make more sense to store that value here rather than a string, and validate the string when you parse it from incoming SDP.

Comment 3

[enda mannion (:emannion)]

Attachment: Support OPUS audio codec in SDP

Added siz new config items for fmtp options.

Comment 4

[enda mannion (:emannion)]

(In reply to Timothy B. Terriberry (:derf) from comment #2)
> Comment on attachment 653982 [details] [diff] [review]
> Support OPUS audio codec in SDP
> 
> Review of attachment 653982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ with one change and one suggestion.
> 
> (In reply to enda mannion (:emannion) from comment #1)
> > maxaveragebitrate=40000;usedtx=0;stereo=0;useinbandfec=0;
> > maxcodedaudiobandwidth=fb;cbr=0
> > a=ptime:20
> > a=maxptime:120
> 
> Right now we don't have any way to pass these to VoiceEngine, and if we did,
> it doesn't have any way to use them. But we should probably respect things
> like minptime, maxptime, and maxcodedaudiobandwidth once we do (we're not
> required to by the spec, but it's useful to do so).
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
> @@ +6277,5 @@
> > +    }
> > +
> > +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> > +    if (attr_p == NULL) {
> > +        if (sdp_p->debug_flag[SDP_DEBUG_ERRORS]) {

This code returns SDP_INVALID_PARAMETER which is the way SIPCC SDP handles attributes or options missing in the SDP. The SDP_ERROR line is just a log message, it actually writes the same way to the log as an SDP_WARN and others. I will not raise an alarm in this case.  

> 
> These are all optional parameters, so we shouldn't error out if they're not
> found. Enda said on IRC he'll add a config item and have them off by default
> for now, since they're not being passed anywhere yet.

right, I added config items for all these all set to false for now. In time we can raise them to the API if we need the use to set them.

> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
> @@ +157,5 @@
> > +    u32                       maxaveragebitrate;
> > +    u16                       usedtx;
> > +    u16                       stereo;
> > +    u16                       useinbandfec;
> > +    char                      maxcodedaudiobandwidth[SDP_MAX_STRING_LEN+1];
> 
> You've got a nice max_coded_audio_bandwidth_ enum below... it might make
> more sense to store that value here rather than a string, and validate the
> string when you parse it from incoming SDP.

Removed the string.

Comment 5

[enda mannion (:emannion)]

Attachment: revised: Support OPUS audio codec in SDP

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 654380 [details] [diff] [review]
revised: Support OPUS audio codec in SDP

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

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ -845,5 @@
>                       custom_y=atoi(temp);
>  		  if (iter == 3) 
>                       custom_mpi=atoi(temp);
>                    temp=strtok(NULL,",");
> -		  iter++;

Did you mean to take this iter++ out?  It appears that 2 and 3 can never happen now.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +499,5 @@
>  
>      if (shouldHaveAudio)
>      {
> +      //ASSERT_NE(sdp.find("a=rtpmap:0 PCMU/8000"), std::string::npos);
> +    	ASSERT_NE(sdp.find("a=rtpmap:109 opus/48000"), std::string::npos);

Don't we want to check here for both G711 and Opus?

Comment 7

[enda mannion (:emannion)]

(In reply to Ethan Hugg [:ehugg] from comment #6)
> Comment on attachment 654380 [details] [diff] [review]
> revised: Support OPUS audio codec in SDP
> 
> Review of attachment 654380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
> @@ -845,5 @@
> >                       custom_y=atoi(temp);
> >  		  if (iter == 3) 
> >                       custom_mpi=atoi(temp);
> >                    temp=strtok(NULL,",");
> > -		  iter++;
> 
> Did you mean to take this iter++ out?  It appears that 2 and 3 can never
> happen now.

Thanks for catching this that would have broken the fmtp annex option.

> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +499,5 @@
> >  
> >      if (shouldHaveAudio)
> >      {
> > +      //ASSERT_NE(sdp.find("a=rtpmap:0 PCMU/8000"), std::string::npos);
> > +    	ASSERT_NE(sdp.find("a=rtpmap:109 opus/48000"), std::string::npos);
> 
> Don't we want to check here for both G711 and Opus?

I changed the tests, on an answer I just test for one negotiated codec currently opus, but on offer I check for both of them.

Comment 8

[enda mannion (:emannion)]

Attachment: revised: Support OPUS audio codec in SDP

Comment 9

[enda mannion (:emannion)]

Attachment: revised: Support OPUS audio codec in SDP

g.711 is negotiated in favour of opus for now.

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 654680 [details] [diff] [review]
revised: Support OPUS audio codec in SDP


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/d18e3c7e2dfa

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: Fix warnings from Opus signaling patch

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 654826 [details] [diff] [review]
Fix warnings from Opus signaling patch

Some of these are not completely trivial so perhaps you should take a look.

Comment 13

[enda mannion (:emannion)]

Comment on attachment 654826 [details] [diff] [review]
Fix warnings from Opus signaling patch

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

They all look valid.

Comment 14

[Ethan Hugg [:ehugg]]

Comment on attachment 654826 [details] [diff] [review]
Fix warnings from Opus signaling patch


Pushed warning patch to Alder - http://hg.mozilla.org/projects/alder/rev/f737b0bf2dd2

Comment 15

[Timothy B. Terriberry (:derf)]

Attachment: Reprioritize Opus over G.711

Comment 16

[Timothy B. Terriberry (:derf)]

One additional issue with this patch: currently we only offer telephone-event/8000. We should be offering a payload type for telephone-event/48000, too, so that it can match the Opus one.

Comment 17

[enda mannion (:emannion)]

(In reply to Timothy B. Terriberry (:derf) from comment #16)
> One additional issue with this patch: currently we only offer
> telephone-event/8000. We should be offering a payload type for
> telephone-event/48000, too, so that it can match the Opus one.

I added   telephone-event/48000  and negotiate Opus

Offer

m=audio 58686 RTP/SAVPF 109 0 8 101
c=IN IP4 79.97.176.197
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:109 telephone-event/48000
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15


Answer

m=audio 53885 RTP/SAVPF 109 101
c=IN IP4 79.97.176.197
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:109 telephone-event/48000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15


But is it correct that I use the same payload number as opus in the telephone event, 109?  Should I add a fmtp attribute for opus similat to what we have for 101?

If this is correct I will upload a new patch to the Opus bug.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

> But is it correct that I use the same payload number as opus in the telephone event, 109?  Should I add a fmtp attribute for opus similat to what we have for 101?

You must use a different payload number - two rtpmaps for the same payload is always incorrect.  It should choose another dynamic payload value, add it to the m= line, and have an a=rtpmap for it.

Comment 19

[Timothy B. Terriberry (:derf)]

(In reply to Randell Jesup [:jesup] from comment #18)
> You must use a different payload number - two rtpmaps for the same payload
> is always incorrect.  It should choose another dynamic payload value, add it
> to the m= line, and have an a=rtpmap for it.

Right, this is going to require some changes in the webrtc.org code, too, which will probably need some discussion with them. I filed bug 785834 to track this separately.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

this is fixed, right?

Comment 21

[Timothy B. Terriberry (:derf)]

(In reply to Randell Jesup [:jesup] from comment #20)
> this is fixed, right?

Yes, though on further review, we should be specifying a /2 for the channel count. I'll file a new bug on that.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

per derf's comment 21, closing