Skip to content

Add force strip extmap flag - #2167

Open
dnygate wants to merge 2 commits into
sipwise:masterfrom
dnygate:force-strip-extmap
Open

Add force strip extmap flag#2167
dnygate wants to merge 2 commits into
sipwise:masterfrom
dnygate:force-strip-extmap

Conversation

@dnygate

@dnygate dnygate commented Sep 4, 2026

Copy link
Copy Markdown

RTP header extensions are only processed, and unknown ones removed, if at
least one side of the call negotiated header extensions through a=extmap
attributes in the SDP. Otherwise RTP is forwarded untouched, including any
header extensions an endpoint sends without having announced them.

This adds a force strip extmap flag that turns on header extension
processing for all streams of a call regardless of whether any extensions
were negotiated, so that unannounced header extensions are removed from
forwarded RTP. The flag is remembered for the lifetime of the call, so it
only needs to be given once.

Use case: media coming from a WebRTC selective forwarding unit carries
header extensions that WebRTC clients use among themselves (audio level,
transport-wide congestion control) but which were never part of the SIP
side's SDP. Some SIP endpoints do not cope with unexpected header
extensions, and with this flag rtpengine can strip them selectively on
such routes.

Tests: a case in t/auto-daemon-tests-rtp-ext.pl sends RTP with header
extensions in both directions on a call without any a=extmap and checks
that they arrive without the extensions.

RTP header extensions are only processed, and unknown ones stripped, if
at least one side of the call negotiated header extensions through
`a=extmap` attributes. Otherwise RTP is forwarded untouched, including
any header extensions an endpoint may include without having announced
them.

Add a `force strip extmap` flag which selects header extension
processing for all streams of a call regardless of whether any
extensions were negotiated, so that unannounced header extensions are
removed. This is useful when forwarding media from sources that always
include header extensions towards endpoints that do not cope with them.
@rfuchs

rfuchs commented Sep 4, 2026

Copy link
Copy Markdown
Member

I feel like this should be handled unconditionally, without the need for an extra flag.

IOW if one side advertised some RTP extensions and the other side didn't accept them, then they should be removed anyway.

There could be reasons not to of course, but intuitively I would say that removing unsupported extensions should be the default behaviour, and possibly leaving them in place should be the optional alternative.

@dnygate

dnygate commented Sep 4, 2026

Copy link
Copy Markdown
Author

Understood. __determine_rtpext_handler() becomes an unconditional
sh->rtpext = &rtpext_printer_extmap;, and the flag itself, CALL_FLAG_FORCE_STRIP_EXTMAP,
the __call_monologue_init_from_flags() hook and the documentation section all come out with
it, which leaves the change as a net deletion. rtpext_printer_copy is still reached from
media_player.c:814, sink_handler_set_generic() and media_socket.c:2618, so it stays
where it is. I will rework the PR on that basis and drop the flag from the test.

On the inverse option, leaving unnegotiated extensions in place, I would rather not add it
until there is a concrete need for it, since an opt-out that nothing exercises tends to rot.
Easy enough to add later, or now if you would prefer it in from the start.

@dnygate

dnygate commented Sep 4, 2026

Copy link
Copy Markdown
Author

Before reworking this I built the unconditional version and ran it against the existing
suite. It breaks six scenarios in t/auto-daemon-tests-rtp-ext.pl, twelve assertions in
total: unsolicited exts and unsolicited exts w tc, each in its plain, SRTP and
DTLS variant. Every one of them sets up a call with no a=extmap on either side, sends RTP
carrying header extensions, and then asserts that those extensions arrive intact, which is
exactly the behaviour the flag was there to opt out of. They arrived with 6c5190a last July.
Nothing else moves: daemon-tests-main, daemon-tests-bundle, daemon-tests-dtls,
daemon-tests-rtcp and daemon-tests-transform all still pass, so the blast radius is
confined to those six.

That makes the change you are describing a deliberate reversal of what those tests pin
rather than a gap in the coverage, and inverting tests you wrote is your call rather than
mine. Two ways forward, depending on which you had in mind:

  1. Unconditional stripping, with the six scenarios inverted to expect the extensions gone.
  2. Unconditional stripping plus an opt-out flag, with the six scenarios given that flag so
    they carry on asserting pass-through.

Keeping the opt-in flag as originally submitted is still on the table as well, if you would
rather leave the default as it is.

My own preference is the second if the pass-through was intentional for endpoints that
depend on receiving what they were sent, and the first if those tests were only ever
recording what the code happened to do at the time. Say which one you want and I will push
it.

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.

2 participants