Skip to content

Gracefully handle m=text (T.140 RTT) instead of rejecting the entire SDP#3624

Open
yakimenko73 wants to merge 2 commits intomeetecho:masterfrom
yakimenko73:sdp-graceful-rtt-handling
Open

Gracefully handle m=text (T.140 RTT) instead of rejecting the entire SDP#3624
yakimenko73 wants to merge 2 commits intomeetecho:masterfrom
yakimenko73:sdp-graceful-rtt-handling

Conversation

@yakimenko73
Copy link
Copy Markdown

Summary

This PR adds graceful handling of m=text media lines (T.140 Real-Time Text, RFC 4103) in SDP processing. I know, that Janus does not implement RTT yet, at least its not merged, but i think m=text line in an incoming SDP offer should not cause the entire session to be rejected. Instead, Janus now recognizes the m=text type, accepts it at the SDP parsing level, and just disables it by setting port to zero - allowing call with compatible media (e.g., m=audio) to proceed normally.

Problem

Many SIP clients (e.g., PJSIP lib) include m=text in their SDP to advertise T.140 Real-Time Text support. When such an SDP reaches Janus, Janus currently treats m=text as an unknown media type, causing the SDP to be treated as invalid and returning a 488 Not Acceptable Here, even though the offer contains perfectly compatible audio/video streams.

Per RFC 3264 Section 6.1, unsupported media streams should be handled gracefully:

If the answerer has no media formats in common for a particular offered stream, the answerer MUST reject that media stream by setting the port to zero.

If there are no media formats in common for all streams, the entire offered session is rejected.

So, i think, Janus should reject only the unsupported m=text stream (with port 0), not the entire SDP session

Changes

1. Recognize m=text as a known media type

sdp-utils.h, sdp-utils.c
Added JANUS_SDP_TEXT to the janus_sdp_mtype enum and the corresponding parsing/serialization logic.
Previously, m=text was parsed as JANUS_SDP_OTHER, which triggered an error. Now it is recognized as a known (but unsupported) media type.

2. Remove connection address for rejected unsupported media

sdp.c
In janus_sdp_merge, the code path for unsupported media types already sets port=0 and direction=inactive. This PR additionally removes the connection address c= for such lines.

This is necessary because Janus uses BUNDLE (RFC 8843) for WebRTC, and rejected m-lines are not included in a=group:BUNDLE.

If a rejected m-line retains a real connection address while not being in the BUNDLE group, WebRTC browsers attempt to establish a separate ICE transport for it, which fails. The RFC 8843 Section 18.5 example confirms that disabled/rejected m-lines omit the c= line entirely

Use Case

SIP-to-WebRTC bridging via the Janus SIP plugin, where SIP endpoints include m=text for T.140 accessibility support alongside m=audio. Without this fix, the call fails entirely despite audio being fully compatible.

@januscla
Copy link
Copy Markdown

Thanks for your contribution, @yakimenko73! Please make sure you sign our CLA, as it's a required step before we can merge this.

@lminiero
Copy link
Copy Markdown
Member

As you noticed, the m=text bit is indeed part of #3231, where it's handled in a context aware way. There's one thing I don't get, though:

SIP-to-WebRTC bridging via the Janus SIP plugin, where SIP endpoints include m=text for T.140 accessibility support alongside m=audio. Without this fix, the call fails entirely despite audio being fully compatible.

This seems to suggest that, the way your patch works, if a SIP endpoint offers m=audio and m=text, the WebRTC SDP that Janus generates towards the WebRTC user will include both too, but offering m=text too, which could cause issues if the WebRTC user for some reason ends up accepting that m-line instead of rejecting as it should. Besides, I'm not 100% sure WebRTC browsers like seeing unsupported media lines in the SDP at all: I seem to remember that, when I was playing with RTT support in my own patch, if the SDP had m=text in there, the browser would completely reject the SDP, as it only wants audio/video/application (but things may have changed in the meanwhile).

If the use case is the SIP plugin, a possibly better and less intrusive approach (as far as WebRTC is concerned) is simply removing the "offending" media line from the SDP on the way to WebRTC, and re-injecting it on the SIP side as a rejected media line. This does introduce the issue of SDP asymmetry, but one that's already going to be an issue once #3514 and #3231 meet, since in that case we translate m=text to m=application, and if multiple m=text are there (or m=text and other application protocols) on the SIP side, we can only have a single m=application on the WebRTC side anyway, which forces us to handle the asymmetry.

@yakimenko73
Copy link
Copy Markdown
Author

Hi, first of all, thanks for the review and the suggestion :)

I agree that passing m=text through to the WebRTC client is not ideal, even as a rejected line. I tested it in my browser, it works well, but i agree that some browsers, especially old ones, may behave unpredictably with unknown media types in the offer, so keeping the WebRTC SDP clean makes much more sense.

I havee reworked the approach to implement the "strip and re-inject" strategy you described. Here is what the new commit does:

  1. Strip m=text from the WebRTC offerr.
    When an incoming SIP INVITE contains m=text lines, they are removed from the parsed SDP before the offer is forwarded to the browser. The stripped m-lines (including their original index, protocol, format, and payload type) are stored in the session (session->text_mlines) for later use.

  2. Re-inject as rejected in the SIP answer.
    When the browser sends its answer, the previously stripped m=text lines are re-injected into the SDP at their original positions as rejected streams (port=0, direction=inactive), preserving the m-line count matching required by RFC 3264. The stored m-lines are then freed.

  3. Skip text m-lines.
    Non-audio/video m-lines are now skipped during SDP manipulation, so the re-injected rejected text lines are not modified with audio/video-specific logic (port assignment, SRTP attrs, c_addr replacement etc).

And regarding the SDP asymmetry between the SIP and WebRTC legs, i thinks its expected and safe in this context, cause Janus operates as a B2BUA, so the two legs already maintain fundamentally different SDPs, the WebRTC side uses bundles/ice and other specific stuff while SIP side uses RTP/AVP etc. So, i think, from each sides perspective, the SDP remains fully compliant, we actually can not avoid asymmetry anyway :/

@lminiero
Copy link
Copy Markdown
Member

And regarding the SDP asymmetry between the SIP and WebRTC legs, i thinks its expected and safe in this context

Well, it won't be once #3514 lands. At the moment it's mostly safe because, even in Janus 1.x, the SIP plugin doesn't do multistream, so an audio packet coming from the SIP side will be routed with index -1 and type "audio", and the core will know what to do with it (there will only be a single audio m-line and so a single index to address the associated medium). As soon as we allow the SIP plugin to do multistream too (which is what #3514 does), that assumption goes out of the window, and you'll need to figure out a mapping between the mindex on the SIP side and the one on the WebRTC side, and viceversa (you can't simply say "I got this packet on mindex 4 via SIP, shoot it out on mindex 4 of WebRTC", because the indexes won't match).

That branch is currently marked as experimental, but I plan to land it in the next few weeks after some fine tuning. As such, I'll have to decide whether to merge your PR as it is (after I review, that is), which would force me to address that change in #3514, or keep it parked until #3514 is merged, so that this PR can address that asymmetry which will be an actual problem.

@yakimenko73
Copy link
Copy Markdown
Author

Ohh, i see. Anyway, im happy to help fix m-line indexing issue when needed whether i should update this PR once #3514 lands, or do it in a follow-up if you decide to merge this first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants