[RTC-477] Allow for muting tracks without renegotiation #392

roznawsk · 2024-05-13T10:49:46Z

This allows for explicitly muting and then unmuting tracks.

The flow is as follows:

Send mute event from frontend AND replace the track with null so no data is sent.

This is how it looks like on the FE:

webrtc.replaceTrack(videoStreamIdRef.current, null, null, "mute");

const mediaEvent = generateMediaEvent("muteTrack", { trackId: trackId });
this.sendMediaEvent(mediaEvent);

The above is POC, and will probably end up with a dedicated function in membrane-webrtc-js.

The mute event is handled by TrackSender which sends %TrackVariantPaused{reason: :muted} events.
Upon receiving this events the TrackReceiver marks the currently active variant as muted_variant
The unmute event is sent from FE as well as the track being replaced with an actual video
The TrackSender sends TrackVariantResumed events to TrackReceiver
The VariantSelector within TrackReceiver requests the last paused variant, ignoring the limitation of the bandwidth, which at this point is probably 0.

I don't think that setting muted variant as inactive is possible. That's because both when switching to muted and inactive state we check whether the variant is in the active state first.

codecov · 2024-05-13T12:36:50Z

Codecov Report

Attention: Patch coverage is 38.46154% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 54.89%. Comparing base (acc80de) to head (d7c54c8).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
- Coverage   55.19%   54.89%   -0.31%     
==========================================
  Files          75       75              
  Lines        3299     3339      +40     
==========================================
+ Hits         1821     1833      +12     
- Misses       1478     1506      +28

Files	Coverage Δ
webrtc/lib/webrtc/variant_selector.ex	`84.72% <100.00%> (+1.38%)`	⬆️
webrtc/lib/webrtc/media_event.ex	`19.27% <0.00%> (-0.48%)`	⬇️
webrtc/lib/webrtc/track_receiver.ex	`0.00% <0.00%> (ø)`
webrtc/lib/webrtc_endpoint.ex	`0.00% <0.00%> (ø)`
webrtc/lib/webrtc/variant_tracker.ex	`70.00% <0.00%> (-25.46%)`	⬇️
webrtc/lib/webrtc/track_sender.ex	`63.49% <5.26%> (-9.66%)`	⬇️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

kamciokodzi · 2024-05-14T10:12:20Z

integration_test/mix.exs

@@ -41,6 +41,7 @@ defmodule Membrane.RTC.Engine.Integration.MixProject do
 {:membrane_rtc_engine_file, path: "../file"},
 {:membrane_rtc_engine_sip, path: "../sip"},
 {:membrane_rtc_engine_recording, path: "../recording"},
+ {:membrane_rtc_engine_webrtc, path: "../webrtc", override: true},


Why do we need override?

The monorepo structure introduces some problems during the process of releasing our packages. Setting override: true here for packages which other packages depend on (engine, webrtc) helps alleviate some of these issues -- namely, CI will be able to run the tests even if other endpoints are locked onto hex versions of engine and webrtc instead of relative paths. This has virtually no downsides here, in the integration test suite.

You can take a look at /release.sh for some insights into the process of releasing the engine and its endpoints

sgfn

Engine changes LGTM

mickel8

Could we add PR description? In particular:

How is a track muted on the FE side?
If track is muted, does it send any data?
What do we do with bandwidth estimation when current track is muted? Do we try to keep it on the same level?
Can it happen that muted track becomes inactive?

mickel8 · 2024-05-15T12:09:14Z

engine/lib/membrane_rtc_engine/events/track_variant_paused.ex

- @type t :: %__MODULE__{variant: Track.variant()}
+ @type t :: %__MODULE__{
+ variant: Track.variant(),
+ reason: :inactive | :muted


Can we describe the difference between those two?

The track variant can be paused due to two reasons.
:inactive means that VariantTracker has detected reduced or no activity of the variant and has set it to this state
:muted means that we have explicitly paused variant, indicating that the lack of data sent is deliberate and that we may resume it in the future

Yeah but let's write this in docs

webrtc/lib/webrtc/track_sender.ex

webrtc/lib/webrtc/variant_selector.ex

mickel8 · 2024-05-22T08:02:38Z

engine/lib/membrane_rtc_engine/events/track_variant_paused.ex

- @type t :: %__MODULE__{variant: Track.variant()}
+ @type t :: %__MODULE__{
+ variant: Track.variant(),
+ reason: :inactive | :muted


Yeah but let's write this in docs

mickel8 · 2024-05-22T08:13:49Z

webrtc/lib/webrtc/variant_selector.ex

- @spec variant_inactive(t(), Track.variant()) :: {t(), selector_action_t()}
- def variant_inactive(selector, variant) do
+ @spec variant_paused(t(), Track.variant(), :inactive | :muted) :: {t(), selector_action_t()}
+ def variant_paused(selector, variant, :inactive) do
 Membrane.Logger.debug("Variant inactive #{variant}")


Suggested change

Membrane.Logger.debug("Variant inactive #{variant}")

Membrane.Logger.debug("Variant paused #{variant}")

webrtc/lib/webrtc/variant_selector.ex

roznawsk force-pushed the RTC-477-mute-tracks branch 3 times, most recently from edd670f to 4df211a Compare May 13, 2024 15:08

roznawsk marked this pull request as ready for review May 13, 2024 15:14

roznawsk requested review from mickel8 and sgfn as code owners May 13, 2024 15:14

kamciokodzi reviewed May 14, 2024

View reviewed changes

sgfn approved these changes May 15, 2024

View reviewed changes

mickel8 reviewed May 15, 2024

View reviewed changes

roznawsk requested review from mickel8 and kamciokodzi May 16, 2024 13:48

mickel8 reviewed May 22, 2024

View reviewed changes

roznawsk requested a review from mickel8 May 23, 2024 09:03

mickel8 approved these changes May 23, 2024

View reviewed changes

roznawsk added 10 commits May 25, 2024 10:11

Track unmute event

1446180

Mute track

ece469e

Fix ci

871acbb

Add test

9e57d4e

Changelog

4f014f7

Improve doc

6da6dfc

Remove unnecessary sorting

40629a7

Revert function

a8dbeb1

Fix test

6d47b65

Improve docs

d7c54c8

roznawsk force-pushed the RTC-477-mute-tracks branch from 86ae2fa to d7c54c8 Compare May 25, 2024 08:12

roznawsk merged commit 79d6aa2 into master May 25, 2024
33 checks passed

roznawsk deleted the RTC-477-mute-tracks branch May 25, 2024 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTC-477] Allow for muting tracks without renegotiation #392

[RTC-477] Allow for muting tracks without renegotiation #392

roznawsk commented May 13, 2024 •

edited

codecov bot commented May 13, 2024 •

edited

kamciokodzi May 14, 2024

sgfn May 15, 2024

sgfn left a comment

mickel8 left a comment

mickel8 May 15, 2024

roznawsk May 16, 2024

mickel8 May 22, 2024

mickel8 May 22, 2024

mickel8 May 22, 2024

	Membrane.Logger.debug("Variant inactive #{variant}")
	Membrane.Logger.debug("Variant paused #{variant}")

[RTC-477] Allow for muting tracks without renegotiation #392

[RTC-477] Allow for muting tracks without renegotiation #392

Conversation

roznawsk commented May 13, 2024 • edited

codecov bot commented May 13, 2024 • edited

Codecov Report

kamciokodzi May 14, 2024

Choose a reason for hiding this comment

sgfn May 15, 2024

Choose a reason for hiding this comment

sgfn left a comment

Choose a reason for hiding this comment

mickel8 left a comment

Choose a reason for hiding this comment

mickel8 May 15, 2024

Choose a reason for hiding this comment

roznawsk May 16, 2024

Choose a reason for hiding this comment

mickel8 May 22, 2024

Choose a reason for hiding this comment

mickel8 May 22, 2024

Choose a reason for hiding this comment

mickel8 May 22, 2024

Choose a reason for hiding this comment

roznawsk commented May 13, 2024 •

edited

codecov bot commented May 13, 2024 •

edited