Closed
Bug 1406529
Opened 7 years ago
Closed 7 years ago
Ensure unique Extmap IDs in bundled transports
Categories
(Core :: WebRTC: Signaling, enhancement, P3)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla61
| Tracking | Status | |
|---|---|---|
| firefox61 | --- | fixed |
People
(Reporter: drno, Assigned: drno)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
Currently the RTP header extension IDs are generated per m-section. Therefore IDs will be re-used on different m-section for different RTP header extensions.
This is bad because on a bundled transport the RTP parser then does not know which RTP header extension to parse.
Plus with support for MID in RTP the MID RTP header extension always needs to be on the same ID so the RTP parser before the demuxer can parse out the MID from the RTP packet.
|
Assignee | |
Comment 1•7 years ago
|
||
The quick and dirty solution here would be to move the header ID table onto the global level into JsepSession. But then we would get a registry for the whole call, and not per bundled transport.
Instead I was wondering if we should make the JsepTransport class bundle aware and then put the header ID table in there.
Byron what do you think?
Flags: needinfo?(docfaraday)
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → drno
|
|
Assignee | |
Comment 4•7 years ago
|
||
Turns out it's currently unreasonable to actually implement this in the JsepTransport class, as the extmap "registry" needs to be available right from the get go, but the JsepTransport's only get instantiated on sLD(). And moving that code to earlier places breaks all kind of things.
So instead I'm going to the session global registry for now. For your initial offer everything needs to be globally unique any way, as we normally bundle everything together it also needs to be unique on the global level.
Only in scenarios with multiple bundled transports the IDs would need to be unique per bundle section. Which I think can only happen if someone offers Firefox multiple bundled transports. But in that scenario Fx wouldn't control the extmap IDs as the offerer has to set them.
The other scenario which comes to my mind is where Firefox offers everything bundled together, but the answerer chooses to use multiple bundled transports. And then Firefox adds something which adds more extmap's to the existing m-sections. The only feature which I'm aware off which Fx supports would be turning on simulcast via a re-offer.
If we should ever care to handle that scenario it might be easier to annotate the session level extmap "registry" with the bundle transport ID.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8956344 -
Flags: review?(na-g)
|
||
Comment 7•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8956344 [details]
Bug 1406529: added mochitest for extmap verification
https://reviewboard.mozilla.org/r/225212/#review231180
::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoVerifyExtmap.html:31
(Diff revision 1)
> var test;
> runNetworkTest(function (options) {
> test = new PeerConnectionTest(options);
> test.setMediaConstraints([{audio: true}, {video: true}],
> [{audio: true}, {video: true}]);
> +
This doesn't check direction or that it is in an appropriate msection for the media type. Some extensions should be receive only, some are audio only. We should test both bi-directional calls and 1 way calls.
Attachment #8956344 -
Flags: review?(na-g) → review-
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 10•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8956344 [details]
Bug 1406529: added mochitest for extmap verification
https://reviewboard.mozilla.org/r/225212/#review232170
LGTM with one nit.
::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoVerifyExtmapSendonly.html:11
(Diff revision 2)
> <body>
> <pre id="test">
> <script type="application/javascript">
> createHTML({
> - bug: "796890",
> - title: "Basic audio/video (separate) peer connection"
> + bug: "1406529",
> + title: "Verify SDP extmap attribute for sendrecv connection"
sendrecv -> undirectional
Attachment #8956344 -
Flags: review?(na-g) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8956302 -
Flags: review?(docfaraday)
Attachment #8957335 -
Flags: review?(docfaraday)
|
||
Comment 14•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8956302 [details]
Bug 1406529: ensure unique extmap IDs
https://reviewboard.mozilla.org/r/225174/#review234630
r+ with fixes.
::: media/webrtc/signaling/src/jsep/JsepSession.h:65
(Diff revision 3)
>
> +enum JsepMediaType {
> + kNone = 0,
> + kAudio,
> + kVideo,
> + kAudioVideo
I guess this is so the mid extension is the same for audio and video in a bundle?
::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:31
(Diff revision 3)
> +/*
> +class JsepMediaType
> +{
> +public:
> + enum MediaType {
> + kNone = 0,
> + kAudio,
> + kVideo,
> + kAudioVideo
> + };
> +
> + explicit JsepMediaType(MediaType value)
> + : mValue(value)
> + {
> + }
> +
> + MediaType mValue;
> +};
> +
> +class JsepExtmapMediaType
> +{
> +public:
> + struct ExtmapMediaType {
> + JsepMediaType mMediaType;
> + SdpExtmapAttributeList::Extmap mExtmap;
> + };
> +};
> +*/
> +
Remove.
::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:308
(Diff revision 3)
> std::vector<SdpExtmapAttributeList::Extmap> mAudioRtpExtensions;
> std::vector<SdpExtmapAttributeList::Extmap> mVideoRtpExtensions;
Are these still used?
Attachment #8956302 -
Flags: review?(docfaraday) → review+
|
|
||
Comment 15•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8957335 [details]
Bug 1406529: adjust gtest to new extmap handling
https://reviewboard.mozilla.org/r/226246/#review234632
::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4325
(Diff revision 2)
> - offerExtmap[0].extensionname);
> - ASSERT_EQ(1U, offerExtmap[0].entry);
> + answerExtmap[0].extensionname);
> + ASSERT_EQ(1U, answerExtmap[0].entry);
> ASSERT_EQ("urn:ietf:params:rtp-hdrext:sdes:mid",
> answerExtmap[1].extensionname);
> - ASSERT_EQ(2U, offerExtmap[1].entry);
> + ASSERT_EQ(3U, answerExtmap[1].entry);
> // We ensure that the entry for "bar" matches what was in the offer
> ASSERT_EQ("bar", answerExtmap[2].extensionname);
> - ASSERT_EQ(5U, answerExtmap[2].entry);
> + ASSERT_EQ(7U, answerExtmap[2].entry);
Good catch.
Attachment #8957335 -
Flags: review?(docfaraday) → review+
|
|
Assignee | |
Comment 16•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8956302 [details]
Bug 1406529: ensure unique extmap IDs
https://reviewboard.mozilla.org/r/225174/#review234630
> I guess this is so the mid extension is the same for audio and video in a bundle?
Yep this is to allow setting extensions like MID for audio and video m-sections and use the same ID in that case.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 20•7 years ago
|
||
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/3d85483ef79f
ensure unique extmap IDs r=bwc
https://hg.mozilla.org/integration/autoland/rev/fb17397caff2
adjust gtest to new extmap handling r=bwc
https://hg.mozilla.org/integration/autoland/rev/87a99640fb81
added mochitest for extmap verification r=ng
|
||
Comment 21•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/3d85483ef79f
https://hg.mozilla.org/mozilla-central/rev/fb17397caff2
https://hg.mozilla.org/mozilla-central/rev/87a99640fb81
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
|
||
Updated•6 years ago
|
status-firefox58:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•