Closed Bug 1406529 Opened 2 years ago Closed 2 years ago

Ensure unique Extmap IDs in bundled transports

Categories

(Core :: WebRTC: Signaling, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: drno, Assigned: drno)

References

(Blocks 2 open bugs)

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.
Blocks: 1405495
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)
Sounds reasonable.
Flags: needinfo?(docfaraday)
Assignee: nobody → drno
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.
Attachment #8956344 - Flags: review?(na-g)
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 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+
Attachment #8956302 - Flags: review?(docfaraday)
Attachment #8957335 - Flags: review?(docfaraday)
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 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+
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.
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
Blocks: hangouts
You need to log in before you can comment on or make changes to this bug.