Tentative first pass at making simulcast egest possible

[robashton]

https://github.com/webrtc-rs/webrtc/issues/289#issuecomment-1271677760

Forgot to make a PR and stick my long-winded text there, but whatever, is is the draft PR :)

(I'll fix the tests momentarily, forgot I'd changed a structural thing for the API!)

[k0nserv]

@robashton This looks great! I'm going to look closer next week. I would advise that you review the changes that undo work from https://github.com/webrtc-rs/webrtc/pull/217 however. These fixed bugs and made us more spec compliant and should definitely remain(might need changing though)

[robashton]

Ah, if I have a source as to "where it came from" then I can do my best to restore it

[robashton]

ah, yes - now I see where it was being set. By the time I realised that code wasn't working any more I'd lost that line.

Fine, that's easy to restore, will sort that along with the pesky Mutex that shouldn't be a Mutex.

[k0nserv]

Great, I think it was only #217 that introduced these changes, but you if you're in doubt it should be clear from git blame. Any part of JSEP that talk about msid and the parts of the spec that discuss RTCRtpSender.[[AssociatedMediaStreamIds]] are relevant to read to make sense of this

[robashton]

That should be a bit closer to the finish line - I think I got the placement right for the msid / simulcast directives in the SDP

[robashton]

I think it might be better to leverage an enum for track_encodings, i.e.

enum TrackEncodings {
  Simulcast(Vec<TrackEncoding>),
  Single(TrackEncoding)
}

I'm not against this, let's get ourselves happy with the rest of the minor bits and I'll have a look at it.

[robashton]

Just tidying up this thread so I can see what is outstanding, quite keen to get this pushed over the finish line so I can move onto the next task in my queue (not webrtc related).

The SRTP Reader stuff seems the most pressing, I've re-looked at this in the whole and it looks as though it's not 'wrong' per se, we've got independent streams on a shared transport and you want separate binds/interceptors for each of those (or that shared state will get hairy quickly).

The comment about the SRTP Stream being shared I don't think is correct, as we create the SRTP stream per encoding at the top of the function

        let ssrc = rand::random::<u32>();
        let srtp_stream = Arc::new(SrtpWriterFuture {
            closed: AtomicBool::new(false),
            ssrc,
            rtp_sender: Arc::downgrade(&self.internal),
            rtp_transport: Arc::clone(&self.transport),
            rtcp_read_stream: Mutex::new(None),
            rtp_write_session: Mutex::new(None),
        });

        let srtp_rtcp_reader = Arc::clone(&srtp_stream) as Arc<dyn RTCPReader + Send + Sync>;
        let rtcp_reader = self.interceptor.bind_rtcp_reader(srtp_rtcp_reader).await;

Calling close on each of these in sequence is fine (I think). This is spiritually quite close to what Pion is doing with its stack (although I appreciate a lot of that has changd and it may well have fixes in to deal with bugs that might arise with this decisio that I haven't implemented here)

[k0nserv]

The comment about the SRTP Stream being shared I don't think is correct, as we create the SRTP stream per encoding at the top of the function

Aight, I misread.

What about RTCP?

[robashton]

Urgh, I didn't mean to push that rebase, I was just looking at what had changed upstream and forgot I'd pulled it when I came back from coffee. What a mess.

[codecov[bot]]

Codecov Report

Base: 56.62% // Head: 56.80% // Increases project coverage by +0.17% :tada:

Coverage data is based on head (7c03a11) compared to base (a000977). Patch coverage: 59.07% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
+ Coverage   56.62%   56.80%   +0.17%     
==========================================
  Files         500      500              
  Lines       47517    47714     +197     
  Branches    12850    12861      +11     
==========================================
+ Hits        26907    27103     +196     
+ Misses       9943     9920      -23     
- Partials    10667    10691      +24     

Impacted Files	Coverage Δ
...hannels-flow-control/data-channels-flow-control.rs	0.00% <0.00%> (ø)
examples/examples/simulcast/simulcast.rs	0.00% <0.00%> (ø)
ice/src/util/util_test.rs	42.85% <0.00%> (ø)
.../src/io/sample_builder/sample_sequence_location.rs	92.30% <ø> (-4.99%)	:arrow_down:
turn/examples/turn_client_udp.rs	0.00% <ø> (ø)
turn/examples/turn_server_udp.rs	0.00% <ø> (ø)
turn/src/relay/relay_range.rs	0.00% <0.00%> (ø)
webrtc/src/error.rs	10.52% <ø> (ø)
.../src/rtp_transceiver/rtp_sender/rtp_sender_test.rs	53.60% <0.00%> (-0.22%)	:arrow_down:
...c/src/rtp_transceiver/rtp_transceiver_direction.rs	95.74% <ø> (+3.15%)	:arrow_up:
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[robashton]

The comment about the SRTP Stream being shared I don't think is correct, as we create the SRTP stream per encoding at the top of the function

Aight, I misread.

What about RTCP?

I've re-read the code around this, and as far as I can see, the infrastructure-per-encoding model works because each encoding has its own SSRC and that is how the SrtpWriterFuture discriminates when it sets itself up. (The SrtpWriterfuture being where the RTCP reader comes from) and that being how read_simulcast then works.

It's a bit of a janky model, but it's the one we've got - I don't know how you'd avoid setting this up per-encoding without changing a heap of other code?

[robashton]

Could you try and clean up the git strangeness and I can do a final pass

If I can work out how, I totally messed this one up, usually I'd just do a git fetch and a rebase on final pass (and from the other side). That I did a pull to check what had changed upstream and then accidentally pushed it is a total cluster.

I imagine my best bet might be to do an interactive rebase locally and re-apply my changes without any of that noise in it.