[RTC-32] Fetch actual bitrate of a variant #229

LVala · 2023-01-25T12:46:53Z

TODO:

fix integrations tests (js sdk from this pr required)
update media event docs

…ndwidth through it

codecov · 2023-01-25T13:27:22Z

Codecov Report

Merging #229 (c20f387) into master (5136fd8) will increase coverage by 0.46%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   57.73%   58.20%   +0.46%     
==========================================
  Files          40       40              
  Lines        1791     1823      +32     
==========================================
+ Hits         1034     1061      +27     
- Misses        757      762       +5

Impacted Files	Coverage Δ
...rane_rtc_engine/endpoints/webrtc/track_receiver.ex	`49.45% <0.00%> (-1.12%)`	⬇️
...b/membrane_rtc_engine/endpoints/webrtc_endpoint.ex	`0.00% <0.00%> (ø)`
...embrane_rtc_engine/endpoints/webrtc/media_event.ex	`24.24% <90.00%> (+18.88%)`	⬆️
...mbrane_rtc_engine/endpoints/webrtc/track_sender.ex	`73.83% <100.00%> (+2.40%)`	⬆️
...ne_rtc_engine/endpoints/webrtc/variant_selector.ex	`84.84% <100.00%> (+0.23%)`	⬆️
lib/membrane_rtc_engine/tees/tee.ex	`87.95% <100.00%> (+0.45%)`	⬆️

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 5136fd8...c20f387. Read the comment docs.

LVala · 2023-01-25T13:44:33Z

I think this PR is ready for review except for the thing I mentioned above, also I was wondering whether I should add some more tests (webrtc_endpoint.ex specifically, even tho there's no tests for this file at the moment).

lib/membrane_rtc_engine/endpoints/webrtc/media_event.ex

lib/membrane_rtc_engine/endpoints/webrtc_endpoint.ex

daniel-jodlos · 2023-02-24T07:31:41Z

lib/membrane_rtc_engine/endpoints/webrtc/media_event.ex

@@ -266,22 +289,45 @@ defmodule Membrane.RTC.Engine.Endpoint.WebRTC.MediaEvent do
            "type" => "offer",
            "sdp" => sdp
          },
-          "trackIdToTrackMetadata" => track_id_to_track_metadata,
-          "midToTrackId" => mid_to_track_id
+          "trackIdToTrackInfo" => track_id_to_track_info


We have to maintain backward compatibility. So, unfortunately, this change can't go in like that. I would recommend just going with the flow and adding another key to that object - if it's missing, I would simply generate a sensible default value, so that older clients can still connect.

Regarding the refactor, I think I would skip it for now, as we will be moving to protocol buffers and we'll fix it then - it's just not worth it to mess with it right now

daniel-jodlos · 2023-02-24T07:57:42Z

lib/membrane_rtc_engine/endpoints/webrtc/variant_selector.ex

+  """
+  @spec update_variant_bitrate(t(), Track.variant(), non_neg_integer()) :: t()
+  def update_variant_bitrate(%__MODULE__{} = selector, variant, bitrate) do
+    variant_bitrates = %{selector.variant_bitrates | variant => bitrate * 1024}


On the FE you divide bitrate by 1024 - these operations cancel each other out

daniel-jodlos · 2023-02-24T09:51:08Z

lib/membrane_rtc_engine/endpoints/webrtc/variant_selector.ex

+  @spec update_variant_bitrate(t(), Track.variant(), non_neg_integer()) :: t()
+  def update_variant_bitrate(%__MODULE__{} = selector, variant, bitrate) do
+    variant_bitrates = %{selector.variant_bitrates | variant => bitrate * 1024}
+    %{selector | variant_bitrates: variant_bitrates}


This is not enough. You need to update an allocation, if you received an update about the currently selected or queued variant, or update requested bitrate if this affects your next_desired_variant

daniel-jodlos · 2023-03-20T10:32:13Z

lib/membrane_rtc_engine/endpoints/webrtc/variant_selector.ex

@@ -142,6 +142,19 @@ defmodule Membrane.RTC.Engine.Endpoint.WebRTC.VariantSelector do
    }
  end

+  # TODO bytes per second or bits per second


bits per second

daniel-jodlos · 2023-03-21T06:09:25Z

lib/membrane_rtc_engine/endpoints/webrtc_endpoint.ex

+        |> Enum.filter(&Map.has_key?(variant_bitrates, to_rid(&1)))
+        |> Enum.map(&{&1, Map.get(variant_bitrates, to_rid(&1))})
+        |> Map.new()


Wouldn't it be easier to go the other way around with rids?

Suggested change

|> Enum.filter(&Map.has_key?(variant_bitrates, to_rid(&1)))

|> Enum.map(&{&1, Map.get(variant_bitrates, to_rid(&1))})

|> Map.new()

Map.new(variant_bitrates, fn {rid_str, bitrate} -> {to_track_variant(rid_str), bitrate} end)

daniel-jodlos · 2023-03-21T06:14:35Z

lib/membrane_rtc_engine/endpoints/webrtc_endpoint.ex

@@ -703,6 +703,7 @@ defmodule Membrane.RTC.Engine.Endpoint.WebRTC do
  def handle_pad_added(Pad.ref(:output, {track_id, variant}) = pad, _ctx, state) do
    %Track{encoding: encoding} = track = Map.get(state.inbound_tracks, track_id)
    extensions = Map.get(state.extensions, encoding, []) ++ Map.get(state.extensions, :any, [])
+    variant_bitrates = to_track_variants(track, Map.get(state.track_id_to_bitrates, track_id))


I think I would use Map.fetch! to get a better error if we happen to not have this information in the state for whatever reason.

lib/membrane_rtc_engine/endpoints/webrtc/variant_selector.ex

lib/membrane_rtc_engine/endpoints/webrtc_endpoint.ex

daniel-jodlos · 2023-03-23T08:15:24Z

lib/membrane_rtc_engine/endpoints/webrtc/media_event.ex

+               },
+               track_id_to_track_metadata: track_id_to_track_metadata,
+               # use default values in VariantSelector if "trackIdToTrackBitrates" is not present
+               track_id_to_track_bitrates: Map.merge(default_bitrates, bitrates),


Because of the merge, the server could in theory accept an invalid media event, with non exhaustive track_id_to_track_bitrates mapping

internal_docs/webrtc_media_events.md

Co-authored-by: Daniel Jodłoś <daniel.jodlos@swmansion.com>

LVala added 10 commits January 25, 2023 13:13

Modify sdpOffer Media Event handling

ee56567

Add TrackVariantEvent and modify some elements to exchange variant ba…

c4ead07

…ndwidth through it

Change names from bandwidth to bitrate accordingly

bfa6b6d

Add handling of new variant bitrate message

5a9d25c

Fix wrong bitrate units

c64d938

Minor code refactor

4d8666a

Fix trackSender tests

c244a13

Minor bug fix and adjusting to changes in js sdk

d0a2bef

Fix bug in tee

c18be7e

Add tests

cdadb97

LVala self-assigned this Jan 25, 2023

Add media event tests

184fb6b

Add event more tests

620ba14

LVala marked this pull request as ready for review January 25, 2023 13:44

LVala requested a review from mickel8 as a code owner January 25, 2023 13:44

LVala commented Jan 25, 2023

View reviewed changes

lib/membrane_rtc_engine/endpoints/webrtc/media_event.ex Outdated Show resolved Hide resolved

LVala commented Jan 25, 2023

View reviewed changes

lib/membrane_rtc_engine/endpoints/webrtc_endpoint.ex Outdated Show resolved Hide resolved

daniel-jodlos self-requested a review February 23, 2023 10:37

mickel8 force-pushed the master branch from 6e8ee02 to 17ec55f Compare February 23, 2023 17:48

daniel-jodlos suggested changes Feb 24, 2023

View reviewed changes

LVala added 8 commits March 15, 2023 13:23

Merge branch 'master' into fetch-bitrate-of-simulcast-variant

f59d2bb

Chagne sdpOffer medai event to be backwards compatible

d11d27e

Change unit of variant bitrates in VariantSelector

f46e6ba

Fix tests

bd76bc8

Changes to VariantSelector

da36698

Fix sending some unnecessary VariantBitrate events

207027c

Not sent unnecessary media events from track_sender

42cdf8f

Merge branch 'master' into fetch-bitrate-of-simulcast-variant

6172b10

Fix tests

82f55ab

LVala requested a review from daniel-jodlos March 20, 2023 09:54

LVala mentioned this pull request Mar 20, 2023

[RTC-32] Fetch actual bitrate of a variant fishjam-dev/membrane-webrtc-js#21

Merged

daniel-jodlos suggested changes Mar 21, 2023

View reviewed changes

LVala added 5 commits March 21, 2023 17:56

Apply requested changes

3262c90

Change default variant limits to chrome defaults

d806dad

Code style improvements

4b5e337

Remove unnecessary comment

212cb05

Fix tests

2f88d97

LVala requested a review from daniel-jodlos March 22, 2023 09:32

daniel-jodlos reviewed Mar 23, 2023

View reviewed changes

LVala added 4 commits March 23, 2023 17:29

Add validation to media events with track variant bitrates

138fccb

Remove validation of media events with track variant bitrates, add docs

a797a12

Merge branch 'master' into fetch-bitrate-of-simulcast-variant

98dd709

Add docs

95c8ceb

LVala requested a review from daniel-jodlos March 24, 2023 12:51

daniel-jodlos approved these changes Mar 29, 2023

View reviewed changes

internal_docs/webrtc_media_events.md Outdated Show resolved Hide resolved

Update internal_docs/webrtc_media_events.md

c20f387

Co-authored-by: Daniel Jodłoś <daniel.jodlos@swmansion.com>

LVala merged commit 976e456 into master Mar 29, 2023

LVala deleted the fetch-bitrate-of-simulcast-variant branch March 29, 2023 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTC-32] Fetch actual bitrate of a variant #229

[RTC-32] Fetch actual bitrate of a variant #229

LVala commented Jan 25, 2023

codecov bot commented Jan 25, 2023 •

edited

Loading

LVala commented Jan 25, 2023 •

edited

Loading

daniel-jodlos Feb 24, 2023

daniel-jodlos Feb 24, 2023

daniel-jodlos Feb 24, 2023

daniel-jodlos Feb 24, 2023

daniel-jodlos Mar 20, 2023

daniel-jodlos Mar 21, 2023

daniel-jodlos Mar 21, 2023

daniel-jodlos Mar 23, 2023

[RTC-32] Fetch actual bitrate of a variant #229

[RTC-32] Fetch actual bitrate of a variant #229

Conversation

LVala commented Jan 25, 2023

codecov bot commented Jan 25, 2023 • edited Loading

Codecov Report

LVala commented Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 25, 2023 •

edited

Loading

LVala commented Jan 25, 2023 •

edited

Loading