Closed
Bug 1113443
Opened 10 years ago
Closed 9 years ago
Replace rtpmap:111 NULL/0 with something more sensible
Categories
(Core :: WebRTC, defect, P3)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: drno, Assigned: drno)
References
Details
Attachments
(1 file)
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•9 years ago
|
||
Were other bugs filed on this?
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1113443: use payload type 19 to reject m-sections. r?bwc
Attachment #8670120 -
Flags: review?(docfaraday)
Comment 4•9 years ago
|
||
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.
Attachment #8670120 -
Flags: review?(docfaraday)
Assignee | ||
Comment 5•9 years ago
|
||
(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•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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
Attachment #8670120 -
Flags: review?(docfaraday)
Comment 9•9 years ago
|
||
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.
Attachment #8670120 -
Flags: review?(docfaraday)
Assignee | ||
Comment 10•9 years ago
|
||
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
Attachment #8670120 -
Attachment description: MozReview Request: Bug 1113443: use payload type 19 to reject m-sections. r?bwc → MozReview Request: Bug 1113443: reject each media type with approriate default. r?bwc
Attachment #8670120 -
Flags: review?(docfaraday)
Updated•9 years ago
|
Attachment #8670120 -
Flags: review?(docfaraday) → review+
Comment 11•9 years ago
|
||
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 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a93d3a102f2
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a93d3a102f2
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•