Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ekr, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC], [blocking-webrtc+] [qa-])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Also adjust vcm to support.
Created attachment 653982 [details] [diff] [review]
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
Attachment #653982 - Flags: feedback?(tterribe)
Attachment #653982 - Flags: feedback?(ethanhugg)
Attachment #653982 - Flags: feedback?(ekr)
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.
Attachment #653982 - Flags: feedback?(tterribe) → feedback+
Created attachment 654018 [details] [diff] [review]
Support OPUS audio codec in SDP

Added siz new config items for fmtp options.
Attachment #653982 - Attachment is obsolete: true
Attachment #653982 - Flags: feedback?(ethanhugg)
Attachment #653982 - Flags: feedback?(ekr)
Attachment #654018 - Flags: feedback?(tterribe)
Attachment #654018 - Flags: feedback?(ethanhugg)
Attachment #654018 - Flags: feedback?(ekr)
(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.
Created attachment 654380 [details] [diff] [review]
revised: Support OPUS audio codec in SDP
Attachment #654018 - Attachment is obsolete: true
Attachment #654018 - Flags: feedback?(tterribe)
Attachment #654018 - Flags: feedback?(ethanhugg)
Attachment #654018 - Flags: feedback?(ekr)
Attachment #654380 - Flags: feedback?(ethanhugg)
Attachment #654380 - Flags: feedback?(ekr)

Comment 6

5 years ago
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?
(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.
Created attachment 654559 [details] [diff] [review]
revised: Support OPUS audio codec in SDP
Attachment #654380 - Attachment is obsolete: true
Attachment #654380 - Flags: feedback?(ethanhugg)
Attachment #654380 - Flags: feedback?(ekr)
Created attachment 654680 [details] [diff] [review]
revised: Support OPUS audio codec in SDP

g.711 is negotiated in favour of opus for now.
Attachment #654559 - Attachment is obsolete: true
Attachment #654680 - Flags: feedback?(ethanhugg)

Comment 10

5 years ago
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
Attachment #654680 - Flags: feedback?(ethanhugg) → feedback+

Comment 11

5 years ago
Created attachment 654826 [details] [diff] [review]
Fix warnings from Opus signaling patch

Comment 12

5 years ago
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.
Attachment #654826 - Flags: feedback?(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.
Attachment #654826 - Flags: feedback?(emannion) → feedback+

Comment 14

5 years ago
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
Created attachment 655269 [details] [diff] [review]
Reprioritize Opus over G.711
Attachment #655269 - Flags: feedback?(ethanhugg)
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.
(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.

Updated

5 years ago
Attachment #655269 - Flags: feedback?(ethanhugg) → feedback+
> 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.
(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.
Depends on: 785834

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc+]
this is fixed, right?
(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.
Depends on: 810363
per derf's comment 21, closing
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+] [qa-]
You need to log in before you can comment on or make changes to this bug.