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

Add Simulcast support #903

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Conversation

Sean-Der
Copy link
Contributor

@Sean-Der Sean-Der commented Jun 5, 2023

This allow senders to send multiple quality levels as one stream.

I also added an example. It just works against the public WHIP/WHEP server I run. I will adjust to w/e you think is best @paullouisageneau. I need to make everything argv driven, but currently you can

  • run example
  • ffmpeg -re -f lavfi -i testsrc=size=640x480:rate=30 -pix_fmt yuv420p -c:v libx264 -g 10 -preset ultrafast -tune zerolatency -f rtp -payload_type 96 'rtp://127.0.0.1:6000?pkt_size=1200
  • Load https://b.siobud.com/seanTest

Then you will see a drop down where you can select different quality levels.

@Sean-Der Sean-Der force-pushed the simulcast branch 3 times, most recently from 9e2fd90 to e8abb32 Compare June 5, 2023 17:36
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Jun 5, 2023

@paullouisageneau Ok everything is green!

  • What additional tests should I add? I will add some simple unit tests, but do you have anything in particular?
  • Are you ok with this API?
  • How would you like the example to flow
  • Anything else?

@paullouisageneau
Copy link
Owner

Thank you for the addition. It looks good overall, I'll look into it over the weekend.

@Sean-Der
Copy link
Contributor Author

Nice! If the C++ API looks ok will add the C equivalent

Hopefully I kept the new API/designs in spirit with current. Don’t want to just bolt something ugly on the side :(

@murillo128
Copy link

Not sure how RTX is handled in libdatachannel, but rtx packets should use the repaired-rtp-stream-id not the rtp-stream-id, is it possible to be handled with this API?

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to implement support for rids inside Description::Media and use the same track for the different quality levels?

@paullouisageneau
Copy link
Owner

Not sure how RTX is handled in libdatachannel, but rtx packets should use the repaired-rtp-stream-id not the rtp-stream-id, is it possible to be handled with this API?

RTX are generated by RtcpNackResponder. I think this is another argument to use the same track for the different quality levels: otherwise, it may not be possible to properly leverage per-track media handlers for simulcast.

@Sean-Der Sean-Der force-pushed the simulcast branch 2 times, most recently from a6230a5 to fa305ca Compare June 14, 2023 15:04
@Sean-Der
Copy link
Contributor Author

Hey @paullouisageneau can I get another review please, thank you!

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes, this looks better to me, the implementation is way simpler!

@Sean-Der Sean-Der force-pushed the simulcast branch 2 times, most recently from 3f108a1 to ca8bc90 Compare June 16, 2023 15:37
@Sean-Der
Copy link
Contributor Author

Hi @paullouisageneau responded to all the reviews. Mind taking a look again, thank you!

@Sean-Der Sean-Der force-pushed the simulcast branch 2 times, most recently from 6951f05 to 096cbaa Compare June 20, 2023 04:12
@Sean-Der
Copy link
Contributor Author

@paullouisageneau done!

Pulled out the example also. PR is much smaller now :)

@Sean-Der
Copy link
Contributor Author

@paullouisageneau sorry for all the mistakes :/

Pushed mind reviewing again?

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking good, thank you!

@paullouisageneau paullouisageneau merged commit 1ead0ce into paullouisageneau:master Jun 21, 2023
@pkviet
Copy link

pkviet commented Jun 22, 2023

@paullouisageneau could you create a tag for obs ? Unless you plan an imminent new release.
Thanks.
Btw we've bypassed the issue we were having with cross compiled DLLs by switching to a full msvc build.

@paullouisageneau
Copy link
Owner

@paullouisageneau could you create a tag for obs ? Unless you plan an imminent new release.
Thanks.

@pkviet I'll create a tag when a solution to #891 (comment) is merged, so this can be used with OBS.

Btw we've bypassed the issue we were having with cross compiled DLLs by switching to a full msvc build.

Great news!

@pkviet
Copy link

pkviet commented Jun 30, 2023

@paullouisageneau hi, pinging again for a new tag for obs !

@paullouisageneau
Copy link
Owner

@pkviet Sorry for the delay, it's now tagged as v0.19.0-alpha.4.

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

Successfully merging this pull request may close these issues.

4 participants