Add simulcast sending support

[kc5nra]

Adds support for specifying the RID when writing packets to str0m as well as for serialization of the VLA extended header.

There are a couple points that need to be resolved:

• Carrying the active/inactive state of streams from simulcast specification (adding the media) to when the SDP is created
• A couple of the VLA tests I wasn't sure about. Specifically the one that I did convert add the bidirectional serialization to (it says 3 active streams but it has no active spatial layers, which would end up being condensed to the shared bitmask)

[algesten]

Thanks!

This looks good overall. I left some comments.

In a later PR we should also toy with the idea of creating a new type MidRid that looks something like:

pub(crate) struct MidRid {
   pub(crate) mid: Mid,
   pub(crate) rid: Option<Rid>,
}

to see if it would tidy up code to avoid passing around tuples in many places.

I'll leave it to @k0nserv to say whether it's correct to run the pacer per-mid/rid combo. I think it is, but better check with the expert.

[kc5nra]

I also thought that but just wanted your opinion first - will quickly refactor it

[kc5nra]

I figured since we are adding new code we could switch (or not!) to midrid, and possibly move the stats in another commit if we like it. If we want it in a separate pr, easy enough to pull out

[algesten]

I turned MidRid into a tuple because it seemed more natural and makes for tidier log messages for the fmt::Debug.

I also implemented in many more places. I think I'll break the whole MidRid refactoring out to a separate PR to make this PR focused on simulcast.

[algesten]

Heads up: I force pushed a rebase off latest main.

[algesten]

MidRid refactoring in #604

[thomaseizinger]

Didn't mean to elicit a review request with my drive-by comments :D
I am not familiar with these parts of the codebase so I am going to dismiss the review request.

[algesten]

Didn't mean to elicit a review request with my drive-by comments :D I am not familiar with these parts of the codebase so I am going to dismiss the review request.

All code must be scrutinized! :D

[thomaseizinger]

Didn't mean to elicit a review request with my drive-by comments :D I am not familiar with these parts of the codebase so I am going to dismiss the review request.

All code must be scrutinized! :D

Say no more!

[k0nserv]

I'll leave it to @k0nserv to say whether it's correct to run the pacer per-mid/rid combo. I think it is, but better check with the expert.

Yes it's fine. The pacer only uses mid as a lookup key, it could equally use SSRC or this new combo of (Mid, Option<Rid>).

[algesten]

@kc5nra I rebased/squashed commits into two commits. One for the simulcast sending support and one for the VLA serialization. This is how I want this PR to land in main. If we do more work, we can fixup to those two.

There are two places I think we need to address before this PR is complete:

• https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L898
• https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L1305

[algesten]

@kc5nra I will now leave this PR in peace (no more force pushing from my side) to let you catch up.

[kc5nra]

@kc5nra I rebased/squashed commits into two commits. One for the simulcast sending support and one for the VLA serialization. This is how I want this PR to land in main. If we do more work, we can fixup to those two.

There are two places I think we need to address before this PR is complete:

• https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L898
• https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L1305

For https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L898, you mean something different than this? Though I agree we currently hand wave the active state

ad2e78c#diff-5fd5a802394cab016425ddca33d909c07672077403390ccafa359c7d55dcbed0R903-R907

The latter isn't totally necessary for simulcast + VLA (late binding rids to ssrc), but it's 4AM and I'm probably not thinking of all the other permutations

[algesten]

@kc5nra I rebased/squashed commits into two commits. One for the simulcast sending support and one for the VLA serialization. This is how I want this PR to land in main. If we do more work, we can fixup to those two.
There are two places I think we need to address before this PR is complete:

• https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L898
• https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L1305

For https://github.com/algesten/str0m/blob/main/src/change/sdp.rs#L898, you mean something different than this? Though I agree we currently hand wave the active state

ad2e78c#diff-5fd5a802394cab016425ddca33d909c07672077403390ccafa359c7d55dcbed0R903-R907

The latter isn't totally necessary for simulcast + VLA (late binding rids to ssrc), but it's 4AM and I'm probably not thinking of all the other permutations

Go to bed!

When we negotiate simulcast with Firefox, we get these rows:

There are two (three?) mechanisms at play, and we need to support them all, to some extent.

1. Old school, pre-communicated SSRC in the SDP. The last three group:FID are declared in the order of the a=simulcast, i.e. l, m, h in this case. However for Firefox this is incorrect, so we use mechanism 2

   str0m/src/change/sdp.rs

   Lines 1113 to 1116 in 2135586

   	// Only use pre-communicated SSRC if we are running without simulcast.
   	// We found a bug in FF where the order of the simulcast lines does not
   	// correspond to the order of the simulcast declarations. In this case
   	// it's better to fall back on mid/rid dynamic mapping.

2. Dynamic SSRC mapping using RTP header extensions. https://github.com/algesten/str0m/blob/main/src/session.rs#L309-L311
3. (I'm unsure here) VLA?

For incoming simulcast, we should ideally handle 1, but we're currently relying only on 2 because of a bug in FF. For outgoing simulcast I think we should make both 1 and 2 work correctly.

[kc5nra]

I'll try to get to this soon - going on a family trip for 4 days. Thanks for the prompt attention and help :-)

[kc5nra]

Hey @algesten, added a gentle attempt at an implementation to get a discussion started. I wasn't sure the best place to put the idea of forcing the ssrc-group creation, so I just went with one of the possible ways. (Versus a new argument to add_media, etc.)

In addition I added an optimization for mid/rid sending that I ran across in libwebrtc that seemed apropos

[algesten]

Hi @kc5nra, I don't think we need a force_ssrc_group. If we just ensure we allocated SSRC for the simulast layers, we can always send the information as pre-communicated SSRC (case 1). Similarly case 2, we just need to double check we are setting the RTP headers, and always send it.

Whether this will trigger a bug in Firefox, we will see, but the ambition is directionally correct :)

[kc5nra]

Sounds good - from a publisher to SFU (str0m not acting as the SFU but just the publisher), I was just trying to allow SDP generation of both variants (1) all the SSRCs enumerated + groups (2) just the simulcast and rids. This just allowed it to more closely resemble a libwebrtc offer. Not critical

[algesten]

Just went down an SDP rabbit hole trying to reason about RID.

When the OFFER sends RIDs, I assume we must interpret the intention the same as for PT. I.e. if the direction is sendrecv or recvonly, the OFFER means: "I will only accept to receive these RIDs". And if the direction is sendonly, it means "I propose to send these RIDs".

If this is consistent with PT logic, it means if the ANSWER contains different RIDs to the OFFER:

1. ANSWER with sendrecv similarly means this is what I expect to receive. This is asymetrical RIDs per direction. We might want to support it even if we don't support it for PT?
2. ANSWER with sendonly (mirroring recvonly OFFER), means, this is what I'm going to send, and it MUST match the receiver (since the "I am prepared to receive" intention is the highest prio). This is definitely grounds for rejecting the ANSWER. There maybe a case for allowing a subset of the RIDs in the OFFER, but completely different is probably incorrect.
3. ANSWER with recvonly (mirroring sendonly OFFER), we must follow, the other side said how they MUST receive. This allows the other side to remove RID they don't need/want.

It seems reasonable that a side can say "I don't want the high layer". Wonder if there are any specs around this.

Bidirectional m-lines. The bane of my life. SDP sucks.

[kc5nra]

This is great info, thanks - and your changes make a lot more sense.. I'm still getting a lay of the land!

8853 5.3.2

An answerer that receives an offer with simulcast that lists a number of simulcast streams MAY reduce the number of simulcast streams in the answer, but it MUST NOT add simulcast streams.

[algesten]

8853 5.3.2
An answerer that receives an offer with simulcast that lists a number of simulcast streams MAY reduce the number of simulcast streams in the answer, but it MUST NOT add simulcast streams.

Excellent!

Ok. So There are few things to do still:

1. Reject incorrect ANSWERs (i.e. that introduces new RIDs when they are not allowed to)
2. Change Writer to only allow the RIDs that are int he new Media::rids_tx
3. Fix so DirectApi makes the correct changes to rids_tx

[algesten]

Rebased off main to get rid of clippy errors.

[algesten]

I think we're ready to merge

[kc5nra]

I'll run one round of testing in the morning if we are okay waiting a little bit - how do we want to squash these?

And thanks for the attentiveness in this :-)

[algesten]

@kc5nra I just now squashed all the commits together to the two commits I think we should have.

Thanks for persevering through this!

I'm fine with waiting to hit merge. Test away!