1113443 - Replace rtpmap:111 NULL/0 with something more sensible

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Nils Ohlmeier [:drno]]

The code in bug 1016476 uses 'rtpmap:111 NULL/0' as rtpmap for rejected m-line section. I think we should stick to behavior suggested in RFC 4317 and try to copy the first codec from the offer (if one exist) instead.

Comment 1

[Maire Reavy [:mreavy]]

Were other bugs filed on this?

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1113443: reject each media type with approriate default. r?bwc

Bug 1113443: use payload type 19 to reject m-sections. r?bwc

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8670120 [details]
MozReview Request: Bug 1113443: reject each media type with approriate default. r?bwc

https://reviewboard.mozilla.org/r/21335/#review19231

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:146
(Diff revision 1)
> +  if (msection->GetMediaType() == SdpMediaSection::kApplication) {
> +    msection->AddDataChannel("19", "reserved", 0);
> +  } else {
> +    msection->AddCodec("19", "reserved", 8000, 1);
> +  }

Most of the time that we see disabled m-sections in the field will be because neither side has a track, not because of negotiation failure. With that in mind, I think it would be safest to use actual codecs (PCMU for audio, and maybe VP8 for video).

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:849
(Diff revision 1)
> +  ValidateDisabledMSection(SdpMediaSection* msection)

I know the const correctness in this file is terrible, but I think this can be const.

Comment 5

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #4)
> https://reviewboard.mozilla.org/r/21335/#review19231
> 
> ::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:146
> (Diff revision 1)
> > +  if (msection->GetMediaType() == SdpMediaSection::kApplication) {
> > +    msection->AddDataChannel("19", "reserved", 0);
> > +  } else {
> > +    msection->AddCodec("19", "reserved", 8000, 1);
> > +  }
> 
> Most of the time that we see disabled m-sections in the field will be
> because neither side has a track, not because of negotiation failure. With
> that in mind, I think it would be safest to use actual codecs (PCMU for
> audio, and maybe VP8 for video).

This change was triggered by a Chrome bug report https://code.google.com/p/webrtc/issues/detail?id=5040, which turns out to be an issue in apprtc. But this sparked some discussion on IRC http://logs.glob.uno/?c=mozilla%23media&s=5+Oct+2015&e=6+Oct+2015#c200088

We could either implement the standard and copy the first codec from the m-line in question (which is requires some effort). Or we send a hard coded answer like we do. I think what is bad about the current 0 is that it is likely to be used for audio m-lines, but should not appear on video m-lines.
As Chrome seems to have trouble with apprtc adding 111 into the video m-line, I think it would be safer to use a hard coded value like 19, which is not suppose to be used by anyone (according to IANA).

Comment 6

[Byron Campen [:bwc]]

I think using a pt of 19 might be fine, although there is probably an implementation that has validation logic that runs on disabled m-sections that will trip over this. Using the codec from the other side might interop better. This might not be from an offer though! If the other side did not have a remote track, and offerToReceive is set to an insufficiently large value, and we remove our own track we'll have a disabled m-section put in a reoffer.

Comment 7

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #6)
> I think using a pt of 19 might be fine, although there is probably an
> implementation that has validation logic that runs on disabled m-sections
> that will trip over this. Using the codec from the other side might interop
> better. This might not be from an offer though! If the other side did not
> have a remote track, and offerToReceive is set to an insufficiently large
> value, and we remove our own track we'll have a disabled m-section put in a
> reoffer.

Yeah we would have to store the first of the last codecs offered for that m-section, or the last codec used by us on that track. The whole idea of using the first codec from the offer list seems to work only well in case of simple offer & answer (without re-offer etc).
So rather then implementing extra complicated logic to remember the best codec all the time, I'm going to try to limit to the first codec if possible and fall back to 19 otherwise.

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8670120 [details]
MozReview Request: Bug 1113443: reject each media type with approriate default. r?bwc

Bug 1113443: use payload type 19 to reject m-sections. r?bwc

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8670120 [details]
MozReview Request: Bug 1113443: reject each media type with approriate default. r?bwc

https://reviewboard.mozilla.org/r/21335/#review19381

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:145
(Diff revision 2)
> +             !msection->FindRtpmap(msection->GetFormats()[0])) {

This is always going to be true, since we clear the attributes above. We could create this backup earlier, or I guess I could live with us just hard-coding to something reasonable like PCMU for audio and VP8 for video.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:1486
(Diff revision 2)
> +  ValidateDisabledMSection(msection);

We already call ValidateTransport in both CreateOffer and CreateAnswer, so we probably don't need this.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3253
(Diff revision 2)
> +  ValidateDisabledMSection(failed_section);

Same here.

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8670120 [details]
MozReview Request: Bug 1113443: reject each media type with approriate default. r?bwc

Bug 1113443: reject each media type with approriate default. r?bwc

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8670120 [details]
MozReview Request: Bug 1113443: reject each media type with approriate default. r?bwc

https://reviewboard.mozilla.org/r/21335/#review19411

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:156
(Diff revision 3)
> +      // We need to have something here to fit the grammar, this seems safe

I'd be fine with a MOZ_RELEASE_ASSERT here, honestly. Up to you. This would let us get rid of the test code that you noted never runs.

Comment 12

[Nils Ohlmeier [:drno]]

Check try results

Comment 13

[Nils Ohlmeier [:drno]]

Try run looks sane

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a93d3a102f2

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a93d3a102f2