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 #189

Merged
merged 23 commits into from
Sep 29, 2020
Merged

Add simulcast support #189

merged 23 commits into from
Sep 29, 2020

Conversation

OrlandoCo
Copy link
Collaborator

Description

Look simulcast project.

Reference issue

Fixes #57

@OrlandoCo OrlandoCo requested a review from tarrencev September 22, 2020 01:46
Copy link
Collaborator

@tarrencev tarrencev left a comment

Choose a reason for hiding this comment

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

some minor comments otherwise looks really good!

pkg/receiver.go Outdated Show resolved Hide resolved
pkg/simulcastsender.go Outdated Show resolved Hide resolved
pkg/simulcastsender.go Outdated Show resolved Hide resolved
"encoding/binary"
)

// VP8Helper is a helper to get temporal data from VP8 packet header
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@OrlandoCo OrlandoCo Sep 25, 2020

Choose a reason for hiding this comment

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

This just take care of the temporal data, it doesn't care of other flags or make a copy a of the vp8 payload for performance. So it's out of scope of the rtp/codecs. I can make a pull request to add temporal flags on vp8 codec, but we should not use it on sfu for temporal info.

@codecov
Copy link

codecov bot commented Sep 28, 2020

Codecov Report

Merging #189 into master will decrease coverage by 48.24%.
The diff coverage is 29.22%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #189       +/-   ##
===========================================
- Coverage   69.57%   21.33%   -48.25%     
===========================================
  Files           8        9        +1     
  Lines         677      872      +195     
===========================================
- Hits          471      186      -285     
- Misses        178      670      +492     
+ Partials       28       16       -12     
Impacted Files Coverage Δ
pkg/receiver.go 0.00% <0.00%> (-53.71%) ⬇️
pkg/router.go 0.00% <0.00%> (-80.77%) ⬇️
pkg/sender.go 0.00% <0.00%> (-93.48%) ⬇️
pkg/session.go 0.00% <0.00%> (-90.00%) ⬇️
pkg/sfu.go 0.00% <0.00%> (-62.67%) ⬇️
pkg/webrtctransport.go 0.00% <0.00%> (-66.40%) ⬇️
pkg/simulcastsender.go 46.96% <46.96%> (ø)
pkg/helpers.go 88.67% <88.67%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d21a6bc...a7e2d02. Read the comment docs.

@tarrencev tarrencev marked this pull request as ready for review September 28, 2020 20:51
pkg/sender.go Outdated
ctx, cancel := context.WithCancel(ctx)
sender.Track()
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, a typo, will fixt it.

if track.RID() != "" {
router = newRouter(p.id, cfg.router, SimulcastRouter)
go func() {
// Send 3 big remb msgs to fwd all the tracks
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we dont do any congestion control on the pub tracks right now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, currently a remb loop is called per track, to calculate the bandwidth and later downgrade senders, but we can omit fwding the remb packet.

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.

simulcast support
2 participants