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.

Sign up for GitHub

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

Jump to bottom

Simulcast: Allow layer selection for publisher #477

Merged
merged 2 commits into from
May 12, 2021
Merged

Simulcast: Allow layer selection for publisher #477

merged 2 commits into from
May 12, 2021

Conversation

dreamerns
Copy link
Contributor

Description

  • Allow publisher to select which layer(s) should be enabled.
  • Publisher can activate/deactivate layer with encoding params of the sender, however, there is no easy way to detect if a layer is enabled or not on sfu side. I added another message to middleware that will allow publishers to select which layer(s) are currently active on the sender side. It updates all the down tracks accordingly.
  • Message is then fanned out to all subscribers, so they are aware which layer is active and which ones are available.
  • In order to send messages on 'ion-sfu' channel (besides audio levels), I added a little bit more robust messaging type (similar to json-rpc). This is a breaking change for the client, unfortunately, but it gives us more flexibility if any new message type needs to be added in the future.

Reference issue

Fixes #...

@codecov
Copy link

codecov bot commented Mar 18, 2021 •
edited
Loading

Codecov Report

Merging #477 (91a809b) into master (7add00c) will decrease coverage by 0.52%.
The diff coverage is 11.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #477      +/-   ##
==========================================
- Coverage   40.72%   40.20%   -0.53%     
==========================================
  Files          23       23              
  Lines        2345     2378      +33     
==========================================
+ Hits          955      956       +1     
- Misses       1263     1295      +32     
  Partials      127      127
Impacted Files Coverage Δ
pkg/sfu/downtrack.go 31.17% <0.00%> (-2.28%) ⬇️
pkg/sfu/peer.go 54.70% <0.00%> (-4.02%) ⬇️
pkg/sfu/session.go 44.44% <100.00%> (+1.26%) ⬆️
pkg/sfu/router.go 47.64% <0.00%> (-1.18%) ⬇️

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 7add00c...91a809b. Read the comment docs.

@dreamerns dreamerns mentioned this pull request Mar 18, 2021
Merged
billylindeman
billylindeman reviewed Mar 18, 2021
View reviewed changes
Copy link
Contributor

@billylindeman billylindeman left a comment

Choose a reason for hiding this comment

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

awesome work

pkg/sfu/session.go Outdated
@@ -204,6 +204,15 @@ func (s *Session) OnClose(f func()) {
s.onCloseHandler = f
}

func (s *Session) SendMessage(method, params, origin string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better named BroadcastAPIChannelMessage for the whole Session

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I wasn't too happy with my naming :-)

pkg/sfu/peer.go Outdated
@@ -215,6 +226,25 @@ func (p *Peer) Trickle(candidate webrtc.ICECandidateInit, target int) error {
return nil
}

func (p *Peer) SendMessage(method, params string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe name SendAPIChannelMessage?

pkg/sfu/peer.go Outdated
@@ -30,6 +31,16 @@ type SessionProvider interface {
GetSession(sid string) (*Session, WebRTCTransportConfig)
}

const (
Copy link
Contributor

Choose a reason for hiding this comment

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

These might be better defined in the subscriberapi.go, not a strong opinion on this though just something to think about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been debating the same. Since the method name can be anything, we could put constants in the appropriate location (session and subscribeapi.go), that way middleware would be on one place, but on the other hand this way all available methods are in one place. Not a strong opinion either.

@adwpc
Copy link
Contributor

adwpc commented Mar 21, 2021

This is useful.

@dreamerns dreamerns marked this pull request as ready for review March 26, 2021 21:23
@dreamerns dreamerns force-pushed the simulcast-publisher-layer-selection branch from 4915ed0 to 5b20944 Compare March 30, 2021 05:17
@dreamerns dreamerns force-pushed the simulcast-publisher-layer-selection branch from 5b20944 to 8de0e7b Compare April 19, 2021 20:16
OrlandoCo
OrlandoCo reviewed Apr 30, 2021
View reviewed changes
pkg/sfu/session.go Outdated Show resolved Hide resolved
@dreamerns dreamerns force-pushed the simulcast-publisher-layer-selection branch from 8de0e7b to ddec8b0 Compare April 30, 2021 23:42
@dreamerns dreamerns force-pushed the simulcast-publisher-layer-selection branch 2 times, most recently from d8b3da2 to 2ebbebf Compare May 12, 2021 00:12
@dreamerns
Simulcast: Allow layer selection for publisher
3ac9111 
Changed ion-sfu channel messaging to be peer based and added aditional types

Addressing PR comments

Rename IonSfuMessage to ChannelAPIMessage. Use interface{} instead of string, fixes double marshalling issue
@dreamerns
Rebase on new interface changes
91a809b
@dreamerns dreamerns force-pushed the simulcast-publisher-layer-selection branch from 2ebbebf to 91a809b Compare May 12, 2021 20:10
@billylindeman billylindeman merged commit 6386736 into ionorg:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrlandoCo OrlandoCo OrlandoCo left review comments

@billylindeman billylindeman billylindeman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dreamerns @adwpc @billylindeman @OrlandoCo