This repository was archived by the owner on Sep 19, 2024. It is now read-only.
Enhance API for configuring simulcast. Fix switching between available simulcast encodings #109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
mickel8
commented
Apr 6, 2022
•
mickel8
commented
- enhance API for configuring simulcast both on the client and server sides. Now user can choose, which simulcast encodings should be by default enabled on the client side i.e. sent to the server. Besides this, user can choose which encoding should be by default passed to the client from the server. This can be configured per track.
- fix switching between encodings when one of them becomes active or inactive
- refactor a little internal linking mechanism
- now endpoint can subscribe only for one track i.e. it cannot pass list of tracks it subscribes for
mickel8
force-pushed
the
simulcast-frontend-api
branch
5 times, most recently
from
9b49751 to
8279c8a
Compare
mickel8
force-pushed
the
simulcast-frontend-api
branch
from
49d3642 to
e8165d5
Compare
mickel8
force-pushed
the
simulcast-frontend-api
branch
2 times, most recently
from
59271b1 to
a43e56c
Compare
mickel8
force-pushed
the
simulcast-frontend-api
branch
3 times, most recently
from
a4f5c8a to
4dda8fa
Compare
mickel8
force-pushed
the
simulcast-frontend-api
branch
2 times, most recently
from
4f7e44d to
2fba296
Compare
mickel8
changed the title
Rados13
suggested changes
Apr 11, 2022
Comment on lines
87
to
90
| Enum.each(tracks, fn track -> | ||
| case Engine.subscribe(state.rtc_engine, endpoint_id, track.id, :RTP) do | ||
| :ok -> | ||
| {:ok, Map.update!(state, :tracks, &Map.merge(&1, new_tracks))} |
There was a problem hiding this comment.
Are you sure that HLS_sink can handle RTP packets?
There was a problem hiding this comment.
Yeah, I think we need to write some integration tests for this
Rados13
approved these changes
Apr 13, 2022
mat-hek
reviewed
Apr 14, 2022
lib/membrane_rtc_engine/engine.ex
Outdated
| defp do_fulfill_subscription(s, tee_kind, state) do | ||
| links = prepare_track_to_endpoint_links(s, tee_kind, state) | ||
| s = %Subscription{s | status: :active} | ||
| state = update_in(state, [:subscriptions, s.endpoint_id], &Map.put(&1, s.track_id, s)) |
There was a problem hiding this comment.
Suggested change
| state = update_in(state, [:subscriptions, s.endpoint_id], &Map.put(&1, s.track_id, s)) | |
| state = put_in(state, [:subscriptions, s.endpoint_id, s.track_id], s) |
lib/membrane_rtc_engine/engine.ex
Outdated
|
|
||
| defp fulfill_subscription(%Subscription{format: _remote_format} = s, _ctx, state) do |
There was a problem hiding this comment.
Suggested change
| defp fulfill_subscription(%Subscription{format: _remote_format} = s, _ctx, state) do | |
| defp fulfill_subscription(%Subscription{format: _remote_format} = subscription, _ctx, state) do |
I'd avoid shortcuts and one-letter names
mickel8
force-pushed
the
simulcast-frontend-api
branch
from
61b1caf to
54614ca
Compare
mat-hek
approved these changes
Apr 19, 2022
lib/membrane_rtc_engine/engine.ex
Outdated
| @@ -925,7 +937,7 @@ defmodule Membrane.RTC.Engine do | |||
| {:ok, state} | |||
| end | |||
|
|
|||
| defp link_inbound_track(track_id, rid, track, endpoint_id, ctx, state) do | |||
| defp create_tee_and_link(track_id, rid, track, endpoint_id, ctx, state) do | |||
There was a problem hiding this comment.
Suggested change
| defp create_tee_and_link(track_id, rid, track, endpoint_id, ctx, state) do | |
| defp create_and_link_tee(track_id, rid, track, endpoint_id, ctx, state) do |
lib/membrane_rtc_engine/engine.ex
Outdated
| defp fulfill_subscription(%Subscription{format: _remote_format} = s, _ctx, state) do | ||
| do_fulfill_subscription(s, :tee, state) | ||
| defp fulfill_subscription(%Subscription{format: _remote_format} = subscription, _ctx, state) do | ||
| do_fulfill_subscription(subscription, :tee, state) | ||
| end | ||
|
|
||
| defp do_fulfill_subscription(s, tee_kind, state) do |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.