webrtc icon indicating copy to clipboard operation
webrtc copied to clipboard

Differing extmaps in an offer cause the answer to be malformed

Open KillingSpark opened this issue 3 years ago • 9 comments

Hi,

So this is a bit of a weird case. I have an offer SDP looking like this (abbreviated)

... (session level stuff)
m=audio 9 UDP/TLS/RTP/SAVPF 111 0 8
a=mid:0
... (codecs)
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
... (misc a= lines)
m=video 9 UDP/TLS/RTP/SAVPF 96 97
a=mid:1
... (codecs)
a=extmap:4 urn:ietf:params:rtp-hdrext:sdes:mid
... (misc a= lines)

Note the two differing extmap:X for the two m-lines.

This might be a malformed offer, I am not exactly sure how to interpret the standard here RFC 5285

A usable mapping MUST use IDs in the valid range, and each ID in this range MUST be used only once for each media (or only once if the mappings are session level)

I think it means that this offer is fine, but there isn't a lot of discussion around that topic, most discussions revolve around mismatching IDs in the offer and answer, not inside one and the same SDP. But it might also be malformed.

In any case, this causes the answer returned by a call to create_answer to be formed like this:

...
a=group:BUNDLE 0 1
m=audio 9 UDP/TLS/RTP/SAVPF 111 0 8
a=mid:0
...
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
m=video 9 UDP/TLS/RTP/SAVPF 96 97
...
a=mid:1

Not the ABSENCE of an extmap ID for urn:ietf:params:rtp-hdrext:sdes:mid in the second m-line.

This is not allowed, every bundeled m-line needs an extmap ID for this extension to facilitate demuxing of the bundled streams.

Regardless of whether this offer is ok but unsupported or just malformed, this should raise an error instead of creating a malformed answer.

I think the most straight foward thing to improve the situation would be to check that the extmap IDs match for each m-line before accepting the offer as valid. But depending on whether this offer is fine and should be supported more subtantial changes might be necessary.

KillingSpark avatar Oct 05 '22 10:10 KillingSpark

Hmm, I recall talking about this with some of the Google folks either on https://bugs.chromium.org or over at https://github.com/w3c/webrtc-pc/ but I cannot remember what the conclusion was. The question at hand here seems to be whether ext map id mappings are globally unique or unique for video and audio respectively.

I can dig later, but if you wanna have a look through those two sources there might be an answer there.

Agree that create_answer should return errors rather than creating bad answers

k0nserv avatar Oct 05 '22 10:10 k0nserv

https://bugs.chromium.org/p/webrtc/issues/detail?id=13564 I believe this might be the case I'm thinking about

k0nserv avatar Oct 05 '22 10:10 k0nserv

Agree that create_answer should return errors rather than creating bad answers

Tbh the error should probably be on set_remote_description

I'll have a look into those sources maybe there are some more relevant discussions

KillingSpark avatar Oct 05 '22 10:10 KillingSpark

I found a source in the RFCs for what the behaviour should be:

https://datatracker.ietf.org/doc/html/rfc8843#section-12

When RTP header extensions [RFC8285] are used in the context of this specification, the identifier used for a given extension MUST identify the same extension across all the bundled media descriptions.

So just failing early in set_remote_description if this case is detected would be the correct handling.

KillingSpark avatar Oct 05 '22 10:10 KillingSpark

Yes, it seems that extension id mappings are unique per bundle. I think our code is currently wrong because it separates them for audio and video. We should both detected the error on the part of the other peer and fix our own code. This is the change that was made in libWebRTC which triggered the conversation I linked https://webrtc-review.googlesource.com/c/src/+/235826

k0nserv avatar Oct 05 '22 11:10 k0nserv

A bit off-topic, but I think it is currently necessary to explictly add the mid extensions for both audio and video, but it is required by the standard if bundling is used. Should I open a second issue for this?

KillingSpark avatar Oct 05 '22 11:10 KillingSpark

Is it only required for unified or plan-b too? I feel like there ought to be a reason for it not being defaulted. Might be a hold over from plan-b(for which I believe there's an issue to remove support).

k0nserv avatar Oct 05 '22 11:10 k0nserv

Dunno, but is there any harm in also adding it for plan-b?

KillingSpark avatar Oct 05 '22 11:10 KillingSpark

I don't think there is, well I guess CPU and bandwidth cost, but my thought was more that we might consider deprecating plan-b support and then always configure mid

k0nserv avatar Oct 06 '22 08:10 k0nserv