Add SSRC option to force a fixed egress SSRC - #2166
Conversation
23e4043 to
ecd4088
Compare
rfuchs
left a comment
There was a problem hiding this comment.
The obvious question of course is: what if two ingress SSRCs appear? They would be rewritten to the same egress SSRC, which may have undesirable side effects for sequence and timestamp tracking, or SRTP contexts.
Also the a=ssrc attribute comes to mind - if we have a fixed egress SSRC, that would lend itself to their usage, no?
| // forced egress SSRC is only implemented in userspace | ||
| if (sh->sink->media && sh->sink->media->monologue | ||
| && sh->sink->media->monologue->force_egress_ssrc) | ||
| goto no_kernel; |
There was a problem hiding this comment.
We do have support for SSRC substitution in the kernel module (see ssrc_out and ssrc_subst)
There was a problem hiding this comment.
Agreed. ssrc_subst and ssrc_out[] are already wired up from media_socket.c:1863,1882
off ssrc_map_out, with the substitution itself at nft_rtpengine.c:6745, so once this
moves onto ssrc_map_out the bailout serves no purpose and forced-SSRC streams can stay in
kernel forwarding. Removing it.
| if (!mp->rtcp && out->media && out->media->monologue && out->media->monologue->force_egress_ssrc) { | ||
| uint32_t ssrc = htonl(out->media->monologue->force_egress_ssrc); | ||
| IQUEUE_FOREACH(&mp->packets_out, p) { | ||
| str payload; | ||
| struct rtp_header *rh = rtp_payload(&payload, &p->s, NULL); | ||
| if (rh) | ||
| rh->ssrc = ssrc; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This whole thing should really be handled by the existing SSRC substitution mechanism (see handler_func_passthrough_ssrc in codec.c) instead of adding another loop over the already-processed output packets (and even parsing the RTP header again)
There was a problem hiding this comment.
Agreed. I will rework this onto __stream_ssrc_out() and ssrc_map_out rather than adding a
second pass over the already-processed output packets, which also does away with parsing the
RTP header again.
| case CSH_LOOKUP("egress-to-offerer"): | ||
| case CSH_LOOKUP("egress to offerer"): | ||
| out->ssrc_force.egress_to_offerer = call_ng_parse_ssrc(parser, value); | ||
| break; | ||
| case CSH_LOOKUP("egress-to-answerer"): | ||
| case CSH_LOOKUP("egress to answerer"): | ||
| out->ssrc_force.egress_to_answerer = call_ng_parse_ssrc(parser, value); |
There was a problem hiding this comment.
This mechanism would also be relevant to methods which don't involve two separate parties for offerer and answerer (e.g. publish/subscribe), so a syntax that is less specific to offer/answer would be beneficial.
There was a problem hiding this comment.
Fair point. source-tag and from-tags look like the established pattern, so keying the
value by tag rather than by offerer or answerer role would cover publish/subscribe alongside
offer/answer.
This has to move from the monologue to the media in any case to handle bundle, so the
addressing is going to change regardless of what the keys end up being called. Unless you
would prefer a different shape, I will use a tag-keyed dictionary and drop
egress-to-offerer and egress-to-answerer.
|
Two ingress SSRCs The deployment this came from involves a sequential succession of SSRCs rather than Collapsing that succession onto one stable egress SSRC only works if For genuinely concurrent ingress SSRCs I do not think there is a defensible answer, so I That also means the value wants to be scoped per media rather than per monologue as it is at
It does follow from a fixed egress SSRC, but it is a larger change than it first appears. |
|
Holding the rework here until you have had a chance to look at the two open points above, |
|
No problem with keeping Concurrent SSRCs for a single media is not something I would expect explicit support for. A single RTP source changing SSRCs over its lifetime is something you might encounter though. And yes, per-media SSRCs is definitely preferred over per-monologue, as the current approach wouldn't work for audio/video calls. On that note I was wondering if it's actually needed that the egress SSRC can be set explicitly to a specific value? Would it not be enough that rtpengine picks its own random egress SSRC (as it can already do), and then just returns the chosen values in the response to the offer/answer? That would alleviate the burden of knowing how many media sections in the SDP from the controlling agent. |
|
Thanks, that's a good idea. Explicit values aren't needed here: the fixed SSRC is on the leg There's no chosen egress SSRC to read off today, though. In the relay path Pinning per media does most of the work by itself. Where do you want the values returned? One entry per And what should the flag be called, or should this just be the default whenever a fixed |
|
One entry per No need for an extra flag to have this information added to the response dict. |
|
The use case is PSTN into a WebRTC SFU. The control plane has to tell the SFU which SSRC to The pinning needs a flag. Pin on every call and the egress SSRC changes everywhere, and the |
|
Same on this one, sorry - going manual now :) Each m= section needs its own entry with the index, the media type and the SSRC, so the control plane knows which SSRC goes with which stream. It does need a flag though. Without one every call would get a new egress SSRC, and most of the existing tests would fail. |
|
Yes, sorry, I didn't mean that no flag is needed to change the egress SSRC to some fixed value, but rather that no additional flag is needed to add the information about the SSRC to the response messages. So just one flag to enable it. |
|
Pushed the redesign. One flag,
After the source changes SSRC the kernel table shows the fixed value in both slots with the Without transcoding the flag is ignored. |
0aa2eb1 to
7874ae7
Compare
| bool have_egress_ssrc = false; | ||
| if (flags.fixed_egress_ssrc && to_ml->medias) { | ||
| for (unsigned int i = 0; i < to_ml->medias->len; i++) { | ||
| struct call_media *media = to_ml->medias->pdata[i]; | ||
| if (media && media->fixed_egress_ssrc) { | ||
| have_egress_ssrc = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (have_egress_ssrc) { |
There was a problem hiding this comment.
Personally I would omit this entire block and just report back the egress SSRC information if the flag was present. I don't think there's an expectation not to have a fixed egress SSRC if the flag was present, no? But up to you.
There was a problem hiding this comment.
Dropped the block. It was there because a build without transcoding allocated nothing, so the
flag reported nothing.
Fixed that properly instead: the rewrite now sits in handler_func_passthrough rather than the
SSRC passthrough handler, so it applies in builds without transcoding support as well. The flag
behaves the same either way now, so your assumption holds.
A controlling agent feeding media into a WebRTC SFU needs to know the SSRC of the incoming stream before it can bind a receiver to it, and the SFU needs that SSRC to stay put for the life of the call. With `fixed egress SSRC` set, rtpengine picks an SSRC for each media at signalling time, uses it for all RTP sent towards that media, and returns the chosen values in the offer or answer response, one entry per m= section along with its index and media type. The egress SSRC outlives the ingress SSRC, so the outgoing sequence numbering is carried across a change of source rather than jumping to wherever the replacement source happens to start. The counter it resumes from is the one in memory shared with the kernel module, so it stays right while the stream is offloaded and userspace sees no packets. `ssrc_subst` is switched on for these streams as well, so the substitution survives kernelisation. One egress entry stands in for every ingress SSRC, so the same value and sequence offset go into each slot the kernel matches on rather than only the first. The rewrite sits in the plain passthrough handler rather than the SSRC passthrough one, so that it applies in builds without transcoding support as well.
Covers the values reported in the offer response, the SSRC used on forwarded RTP, and the sequence numbering continuing across a change of ingress SSRC. `offer_answer()` kept the NG response to itself, so add `last_resp()` to let a test read its fields alongside the SDP.
7874ae7 to
e770be5
Compare
rfuchs
left a comment
There was a problem hiding this comment.
Is support for this in no-transcoding builds actually a requirement? To me, no-transcoding builds have always been a second-class citizen, so I'd be OK with just not supporting this. (Or alternatively, include the required scaffolding like the SSRC-passthrough handler also in no-transcoding builds.)
Overall we're getting there, but this still looks like the LLM just throwing generated code at a problem, instead of actually coming up with a good solution.
| * SSRC, so the loop above only fills the first slot. Repeat that entry | ||
| * across the slots the kernel actually matches on. */ | ||
| if (reti->track_ssrc && sink->media->fixed_egress_ssrc | ||
| && sink->media->ssrc_hash_out.nq.head) { | ||
| struct ssrc_entry_call *se = sink->media->ssrc_hash_out.nq.head->data; | ||
| for (unsigned int u = 0; u < G_N_ELEMENTS(redi->output.ssrc_out); u++) { | ||
| if (!s->ssrc[u]) | ||
| break; | ||
| redi->output.ssrc_out[u] = htonl(sink->media->fixed_egress_ssrc); | ||
| redi->output.seq_offset[u] = se->seq_diff; | ||
| redi->output.ssrc_stats[u] = se->stats; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is just a duplicate of the loop above. The first loop fills in the entries and possibly with a substitute SSRC, and this does the same thing again and fills in different values?
Wouldn't this all be easier if the fixed SSRC would just be put into the existing ssrc_map_out ?
There was a problem hiding this comment.
Yeah, you're right. Done that way the existing loop already covers it, so I've dropped mine
and the extra parameter.
One thing though, that loop goes over the egress hash but the index is really the ingress
SSRC. Point everything at one egress entry and you only get slot 0 filled. That's the current
source so it works, but if a source went back to an SSRC it had used before it wouldn't get
substituted. I've left that alone.
There was a problem hiding this comment.
True. That needs a more thorough rework to allow for an N-to-1 mapping of SSRCs.
| if (phc->mp.media_out->fixed_egress_ssrc && phc->mp.ssrc_out) { | ||
| struct ssrc_entry_call *so = phc->mp.ssrc_out; | ||
| uint32_t in_ssrc = ntohl(phc->mp.rtp->ssrc); | ||
| uint16_t seq = ntohs(phc->mp.rtp->seq_num); | ||
| if (so->fixed_in_ssrc_set && so->fixed_in_ssrc != in_ssrc) { | ||
| /* the egress sequence counter lives in memory shared with | ||
| * the kernel module, so it stays current even while the | ||
| * stream is offloaded and userspace sees no packets */ | ||
| uint16_t last = atomic_get_na(&so->stats->ext_seq); | ||
| so->seq_diff = last + 1 - seq; | ||
| } | ||
| so->fixed_in_ssrc = in_ssrc; | ||
| so->fixed_in_ssrc_set = true; |
There was a problem hiding this comment.
This also feels like something that is already being handled elsewhere with the existing SSRC substitution mechanism (although off the top of my head I can't be more specific)
There was a problem hiding this comment.
Partly, yeah. Now it goes through ssrc_map_out the same egress entry sticks around when the
source changes, so seq_diff just keeps working. Dropped that bit.
Still need to reset seq_diff once at the changeover though, since the new source starts its
sequence numbers wherever it likes. Couldn't find anything that already does that.
There was a problem hiding this comment.
The transcode handler should be in charge of this, but guess it's missing from the passthrough handler because it was never needed. Ok, can leave this here then, and look at how it can be moved later on.
|
Not a requirement, no. Happy either way. It can't be half done though. The kernel side already works in those builds, it's only the |
Depends on what's the easier option 😁 If it's not too much trouble to allow the handler in a no-transcoding build, then that's fine. If it would be too large of a change, can just |
This adds a new
SSRCdictionary to the NG control protocol with two keys,egress-to-offererandegress-to-answerer. Each takes an SSRC value (aninteger, or a decimal or
0x-prefixed hex string). rtpengine then rewritesthe SSRC field of every RTP packet it sends towards that side of the call
to the given value.
Motivation: when rtpengine feeds media into a downstream system such as a
WebRTC selective forwarding unit (SFU), that system needs to know the SSRC
of the incoming stream up front in order to bind a receiver to it. Today
the controlling application can only learn rtpengine's outgoing SSRC after
the fact, from RTCP or from a
query. With this option the control planepicks the SSRC itself and tells both sides.
Implementation notes:
the offer or answer carrying it is processed. It can be set in either
message and stays in effect for the rest of the call.
just before SRTP encryption (
media_packet_encrypt()). It thereforecovers media that is relayed unchanged, media that is transcoded, and
media generated by rtpengine itself, for plain RTP as well as SRTP, and
the SRTP authentication tag is computed over the rewritten header. RTCP
is left untouched.
with a forced SSRC are excluded from kernel packet forwarding. Streams
in the other direction are unaffected.
value as well, I am happy to look into feeding it through the existing
per-stream output SSRC mapping that RTCP generation already uses, which
would also allow the kernel module to apply it.
Tests:
t/auto-daemon-tests-ssrc.plcovers integer, hex string and decimalstring values, setting the value in the offer and in the answer, the flat
string flag syntax used by the SIP proxy modules, and that a value of
0is ignored.
Example in string syntax:
SSRC=[egress-to-answerer=0x12345678]