Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simulcast Egest #289

Open
robashton opened this issue Sep 12, 2022 · 12 comments
Open

Simulcast Egest #289

robashton opened this issue Sep 12, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@robashton
Copy link
Member

Bit o' a doozy this one

I'd quite like to send some Simulcast from webrtc-rs, and at the moment as far as I can tell, I can't even hack my way around this (please do correct me if I'm wrong), the setup of the transceivers/tracks/etc seem to be very much working against me.

I see there was this delightful commit over in Pion a few months ago

pion/webrtc@37e16a3

The gist being that we end up with an AddEncoding method on a track, and the rest is mostly worked out for us automatically.

Is there going to be a huge problem with me porting this over to the current version of webrtc-rs, or does this get in the way of the roadmap? I expect it's a bit of a chunk of work, so I'd like to garner some feelings on the matter before I dive in and start slinging code all over the place.

@k0nserv
Copy link
Member

k0nserv commented Sep 12, 2022

Happy to have it included, but I'd like it to be implemented according to the specification, just noting in case Pion isn't. I believe that Pion/Webrtc-rs does not implement ingress simulcast correctly currently.

@robashton
Copy link
Member Author

robashton commented Sep 12, 2022

It is my my understanding that ingest is also a bit sketchy, yes.

 but I'd like it to be implemented according to the specification,

I don't think as far as I can see, anybody is presently even uttering the all important

a=simulcast

At either end of the negotiation at the moment.

I think one of the obstacles to "doing things to spec", is that there is overlap between "doing simulcast/SDP to spec", and "aligning the client APIs to spec", as of course the web browsers currently do simulcast via a largely automatic process (GetParameters, setting encoders, SetParameters, then let the browser do all of the work). My little two hour survey of the scene just now at least points towards browser support for sending simulcast being a bit of a mess at the moment.

Trying to reconcile that world with these server side APIs is a bit awkward, which probably explains why Pion have gone down the route they have (again, at least as far as I can tell from my spelunking just now)

@robashton
Copy link
Member Author

I suspect that what Pion is doing fulfils the "to spec" as far as protocol goes - I haven't dug into the full implementation yet, only the commit linked above so I don't know how or what it ends up generating for the negotiation. I imagine it utters all the right incanctations per spec, because these days the spec for that seems pretty well defined.

As far as API goes, I'd largely trust their judgement, our needs are basically that we're able to get an RTP sender for a particular encode, and send RTP over it (as opposed to now, where you have an RTP sender per track/transceiver), this is something that the browsers don't have because its all internal!

@k0nserv
Copy link
Member

k0nserv commented Sep 12, 2022

Yes I agree that the API question is a bit confusing since the browser handles it behind the scenes. I haven't looked at this in a while, but what I recall being the problem with ingress simulcast is that webrtc-rs treats a simulcast track with three different encodings as three separate tracks. Here I'd like it to be aligned with the spec i.e. it's still a single track(on_track only fires a single time) even though there are three distinct SSRCs(+RTX). Likewise for egress simulcast I think we should match the spec i.e., there's a single egress track, and then have a way to specify when sending which rid the RTP packet in question applies for.

@robashton
Copy link
Member Author

Correct - I think Pion has made steps to make it a bit more like that. (There are some mutterings of convenience methods ReadSimulcast where you pass in the RID you want to get the data for), so converging on that makes sense.

@robashton
Copy link
Member Author

I guess this work comes in two passes then, which is to make the current code a bit more like the Pion code with respect to the above (a single on_track firing), and then adding the convenience methods to set up multiple encodes on the outbound and filter inbounds by RID when reading.

@k0nserv
Copy link
Member

k0nserv commented Sep 12, 2022

It does look like Pion has implemented egress simulcast by having a track per send encoding, ideally we'd stick to the spec and have each RTCRTPSender have only a single track, but maybe there are complications with this that I'm not aware of.

I guess this work comes in two passes then, which is to make the current code a bit more like the Pion code with respect to the above (a single on_track firing)[...]

AFAIK Pion also fires on_track N times for N send encodings on the remote side. My beef is with both Pion and Webrtc-rs not following the spec in this instance. If you aren't concerned with ingress at the moment I would keep it out of scope

@robashton
Copy link
Member Author

Ah, you've already dived deeper than me then :)

I can live with not following the spec if it is too complicated to make work on first pass, if we agree that morally we should be following the spec and work towards that as a goal.

@robashton
Copy link
Member Author

I'll not get started on this right away, as I need to work out what the timeline is for us requiring this (likely to be short/medium term), but there may be some other priorities that take immediate precedence.

When I do, my first action will simply be to attempt the Pion changes as they are in order to understand what the current gap is between where the webrtc-rs codebase is and where Pion has ended up, and I imagine the result of that exercise will dictate how it is done in practise!

Will update this thread once I start poking it - cheers.

@k0nserv k0nserv added the enhancement New feature or request label Sep 12, 2022
@robashton
Copy link
Member Author

(Just FYI, starting this today, in case anybody else was thinking of looking)

@robashton
Copy link
Member Author

Okay, so here is the first pass which is effecively just a straight rip of the Pion commit.

robashton@2742632

This works, providing you go and create an Interceptor that adds the appropriate headers to the RTP packets as they go out, and for this I needed to add the RID to the information we pack into StreamInfo as as far as I can tell there is no other way of getting it. (MID also needs writing, depending on who you are talking to, but that can be worked out by the user based on the tracks they create (assuming one track = one transceiver and order is maintained, which seems to be the case).

A few things then..

  • The whole "associated stream id" thing didn't seem to be used except for reading it anywhere in the codebase, and the concept doesn't exist in Pion (anymore), the job that is achieved by this is achieved primarily by using the same StreamId across tracks (video and audio being part of the same bundle). The SDP being generated reflects this (and is now inline with what Pion does).

  • In order for this to work, track encodings needed slapping behind a Mutex so it can be mutated when sat behind an arc, this should actually just be a RwLock and I'll change that momentarily (I don't write Rust very often and despite fixing one of these in this very codebase only a few months ago, I managed to make the same mistake as the original author again!)

  • I haven't brought any of the tests over yet, most of them were pretty meaningless "ensure thhis value is set when I set this value" sort of things, but I dare say it'd now be possible to spin up an example where simulcast egest talks to simulcast ingest which would have value


That all said and done, what do we want this to look like, now I've got something that works? At the moment it's the same as Pion which is

RtpWriter (1) -> (*) TrackEncoding -> (1) Track

Which isn't exactly 'right', even if the rest of the codebase pretends that it is. I'll take any smart suggestion that isn't too much effort and make it so.

For now, here is a screenshot of Dolby.Io being happy that what I'm sending it is indeed valid simulcast.

Screenshot from 2022-10-07 15-32-47

@haaspors
Copy link
Contributor

I've submitted #578 which ports several pion patches including the one reference earlier in this issue. My PR was merged earlier this month. So I guess we can close this issue and possibly #312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants