Tentative first pass at making simulcast egest possible #312

robashton · 2022-10-07T14:35:37Z

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!)

This is not done "properly", and is an (almost) one for one copy of changes from: pion/webrtc@37e16a3

k0nserv · 2022-10-07T15:10:17Z

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

robashton · 2022-10-07T15:18:22Z

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

robashton · 2022-10-07T15:21:55Z

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 · 2022-10-07T15:24:23Z

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

(most of our operations are reads)

robashton · 2022-10-10T12:50:24Z

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

k0nserv

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

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

webrtc/src/peer_connection/mod.rs

webrtc/src/error.rs

interceptor/src/stream_info.rs

webrtc/src/peer_connection/peer_connection_internal.rs

webrtc/src/peer_connection/sdp/mod.rs

webrtc/src/rtp_transceiver/rtp_sender/mod.rs

webrtc/src/track/track_local/mod.rs

robashton · 2022-10-10T13:59:09Z

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.

webrtc/src/peer_connection/peer_connection_internal.rs

…this tranceiver

robashton · 2022-10-11T11:22:55Z

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 · 2022-10-12T17:08:32Z

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?

webrtc/src/peer_connection/sdp/mod.rs

k0nserv · 2022-10-12T17:12:44Z

webrtc/src/rtp_transceiver/rtp_receiver/mod.rs

@@ -198,7 +198,7 @@ impl RTPReceiverInternal {

        let tracks = self.tracks.read().await;
        for t in &*tracks {
-            if t.track.rid() == rid {
+            if t.track.rid().map_or(false, |r| r == rid) {


I think when comparing to an option option = Some(value) might be easier to read than map_or

IIRC, you can't just do a Some(value) because the types don't line up, track.rid() is Option<String> (I toyed with having it be something else, I don't know what the rust convention is and it seemed to match the other types across the codebase) and rid is a &str, so I'd have to make it owned in order to do the comparison which seems even more of a stretch than doing a map_or, which is fundamentally equivalent to the oft-used fromMaybe in a functional language like haskell/purescript.

If the left hand type can be changed then this would go away. If you have a suggestion for this then please do tell (I did start with &Option<String> to avoid the clone but that just pushed the clone to the consumers which was 🤮 )

You can do track.rid().as_deref() == Some(rid) but I guess that's not much better than this. By using as_deref the Deref coercion mechanism kicks in, resulting in a left hand side type of Option<&str>

That's a neat little feature - would there be benefit in changing the type of track.rid()? It's mostly used in checks just like this anyway

That's a good point actually, it should be Option<&str> to avoid the allocation each time it's called. The caller can still clone it if they want

Yeah okay, Option<String> on the struct and Option<&str> on the trait is much nicer (and analogous to how you'd do a String anyway), I note that there are plenty of inconsistencies in how strings are passed around in the codebase in general and I've left most of them alone in this change so there are a few unnecessary String::from's here, but on the whole it's much nicer

k0nserv · 2022-10-12T17:15:16Z

webrtc/src/rtp_transceiver/rtp_sender/mod.rs

+
+/// RTPSender allows an application to control how a given Track is encoded and transmitted to a remote peer
+pub struct RTCRtpSender {
+    pub(crate) track_encodings: RwLock<Vec<Arc<TrackEncoding>>>,


I don't expect you need to use an async lock here. As long as the await point isn't being held across awaits a regular sync lock is preferred. See https://tokio.rs/tokio/tutorial/shared-state#on-using-stdsyncmutex

Hadn't even occured to me which RwLock I'd imported, I'll watch for that in the future

There is another of these on for the list of tracks on RTCRtpReceiver which I added a few months ago, I guess the same applies for that too

Yeah generally there are a lot of asynchronous locks in this project that should be reconsidered. I suspect a big chunk of these can become sync

k0nserv · 2022-10-12T17:16:51Z

webrtc/src/rtp_transceiver/rtp_sender/mod.rs

+            rtp_write_session: Mutex::new(None),
+        });
+
+        let srtp_rtcp_reader = Arc::clone(&srtp_stream) as Arc<dyn RTCPReader + Send + Sync>;


Hmm, if you are leaving it like this can we add a TODO about this point?

k0nserv · 2022-10-12T17:17:46Z

webrtc/src/rtp_transceiver/rtp_sender/mod.rs

+        let mut send_parameters = RTCRtpSendParameters {
+            rtp_parameters: self
+                .media_engine
+                .get_rtp_parameters_by_kind(self.kind, &[RTCRtpTransceiverDirection::Sendonly])


This will conflict with #321 FYI

webrtc/src/rtp_transceiver/rtp_sender/mod.rs

webrtc/src/track/track_local/track_local_static_sample.rs

This is not done "properly", and is an (almost) one for one copy of changes from: pion/webrtc@37e16a3

(most of our operations are reads)

…this tranceiver

…mulcast-egest

robashton · 2022-10-13T12:06:05Z

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 · 2022-10-13T12:16:12Z

Codecov Report

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

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%)`	⬇️
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%)`	⬇️
...c/src/rtp_transceiver/rtp_transceiver_direction.rs	`95.74% <ø> (+3.15%)`	⬆️
... 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.

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

…xist

robashton · 2022-10-17T09:46:30Z

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?

k0nserv

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

k0nserv · 2022-10-21T10:57:35Z

ice/CHANGELOG.md

@@ -1,7 +1,8 @@
 # webrtc-ice changelog

 ## Unreleased
-* Add IP filter to ICE `AgentConfig` [#306](https://github.com/webrtc-rs/webrtc/pull/306)
+
+* Add IP filter to ICE `AgentConfig` [#306](https://github.com/webrtc-rs/webrtc/pull/306) and [#318](https://github.com/webrtc-rs/webrtc/pull/318).


Something is off here this change shouldn't be in this branch

k0nserv · 2022-10-21T10:58:23Z

ice/src/util/mod.rs

@@ -118,17 +118,13 @@ pub async fn local_interfaces(

            if !ipaddr.is_loopback()
                && ((ipv4requested && ipaddr.is_ipv4()) || (ipv6requested && ipaddr.is_ipv6()))
+                && ip_filter


Why is this diff here, this is already the code on master, something must've gone wrong with rebasing

robashton · 2022-10-21T11:33:22Z

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.

robashton added 3 commits October 7, 2022 15:02

Tentative first pass at making simulcast egest possible

2742632

This is not done "properly", and is an (almost) one for one copy of changes from: pion/webrtc@37e16a3

Fix the tests (for now)

d4160e1

clippy warnings (I guess I need to update/downgrade my rust)

9fdf70b

robashton added 2 commits October 10, 2022 13:36

Re-instate changes from webrtc-rs#217

ec670b4

Replace the mutex on track_encodings with a RwLock

4974cf4

(most of our operations are reads)

k0nserv reviewed Oct 10, 2022

View reviewed changes

robashton added 5 commits October 10, 2022 15:13

Initial feedback from PR

379f403

Make Rid an Option<String> and see how that falls out

21607ac

revert change to log (that I did when I was debugging my own stuff\!)

a3c2a20

unique track ids in stats

d6ff71d

debug impl for TrackLocalContext

d4c27fc

k0nserv reviewed Oct 11, 2022

View reviewed changes

webrtc/src/peer_connection/peer_connection_internal.rs Outdated Show resolved Hide resolved

robashton added 5 commits October 11, 2022 11:02

ensure we only do simulcast send directives if we're doing a send on …

19fc8e9

…this tranceiver

apparently format! is a thing

acef0fd

a few things in rtp_sender that we didn't ike

ac59df1

unnecessary clone now it's an option

4b04180

typo (got vimmed)

246044e

k0nserv reviewed Oct 12, 2022

View reviewed changes

robashton added 4 commits October 13, 2022 11:13

Tentative first pass at making simulcast egest possible

850fcb5

This is not done "properly", and is an (almost) one for one copy of changes from: pion/webrtc@37e16a3

Fix the tests (for now)

e20c15d

clippy warnings (I guess I need to update/downgrade my rust)

866965e

Re-instate changes from webrtc-rs#217

25cedc0

robashton added 17 commits October 13, 2022 11:15

Replace the mutex on track_encodings with a RwLock

2972dd5

(most of our operations are reads)

Initial feedback from PR

55bc75c

Make Rid an Option<String> and see how that falls out

8a59116

revert change to log (that I did when I was debugging my own stuff\!)

f780884

unique track ids in stats

9707863

debug impl for TrackLocalContext

ffad0b5

ensure we only do simulcast send directives if we're doing a send on …

6b23661

…this tranceiver

apparently format! is a thing

7853a8d

a few things in rtp_sender that we didn't ike

afe4dcb

unnecessary clone now it's an option

b64f163

typo (got vimmed)

6a24250

don't hold onto that lock, or we hang

fd84deb

some more tweaks

f5857dd

merge from upstream

5aa6864

Merge branch 'simulcast-egest' of github.com:robashton/webrtc into si…

6996968

…mulcast-egest

removed comments

7da6936

revert method order

5007093

robashton added 2 commits October 13, 2022 14:28

remove read_simulcast_rtcp as we can't see a reason why it needs to e…

7114d5e

…xist

Option<String> on structs, Option<&str> on traits (for rid anyway)

7c03a11

k0nserv reviewed Oct 21, 2022

View reviewed changes

k0nserv mentioned this pull request Oct 22, 2022

obs-webrtc: prototype / playground obsproject/obs-studio#7192

Closed

6 tasks

rainliu force-pushed the master branch from c2f4c33 to fb59f4b Compare January 1, 2024 18:43

haaspors mentioned this pull request Jun 28, 2024

Simulcast Egest #289

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tentative first pass at making simulcast egest possible #312

Tentative first pass at making simulcast egest possible #312

robashton commented Oct 7, 2022 •

edited

Loading

k0nserv commented Oct 7, 2022 •

edited

Loading

robashton commented Oct 7, 2022

robashton commented Oct 7, 2022

k0nserv commented Oct 7, 2022

robashton commented Oct 10, 2022

k0nserv left a comment

robashton commented Oct 10, 2022

robashton commented Oct 11, 2022

k0nserv commented Oct 12, 2022

k0nserv Oct 12, 2022

robashton Oct 13, 2022 •

edited

Loading

k0nserv Oct 13, 2022

robashton Oct 13, 2022

k0nserv Oct 13, 2022

robashton Oct 17, 2022

k0nserv Oct 12, 2022

robashton Oct 13, 2022

robashton Oct 13, 2022

k0nserv Oct 13, 2022

k0nserv Oct 12, 2022

k0nserv Oct 12, 2022

robashton commented Oct 13, 2022

codecov bot commented Oct 13, 2022 •

edited

Loading

robashton commented Oct 17, 2022

k0nserv left a comment

k0nserv Oct 21, 2022

k0nserv Oct 21, 2022

robashton commented Oct 21, 2022

Tentative first pass at making simulcast egest possible #312

Are you sure you want to change the base?

Tentative first pass at making simulcast egest possible #312

Conversation

robashton commented Oct 7, 2022 • edited Loading

k0nserv commented Oct 7, 2022 • edited Loading

robashton commented Oct 7, 2022

robashton commented Oct 7, 2022

k0nserv commented Oct 7, 2022

robashton commented Oct 10, 2022

k0nserv left a comment

Choose a reason for hiding this comment

robashton commented Oct 10, 2022

robashton commented Oct 11, 2022

k0nserv commented Oct 12, 2022

Choose a reason for hiding this comment

robashton Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robashton commented Oct 13, 2022

codecov bot commented Oct 13, 2022 • edited Loading

Codecov Report

robashton commented Oct 17, 2022

k0nserv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robashton commented Oct 21, 2022

robashton commented Oct 7, 2022 •

edited

Loading

k0nserv commented Oct 7, 2022 •

edited

Loading

robashton Oct 13, 2022 •

edited

Loading

codecov bot commented Oct 13, 2022 •

edited

Loading