[RTC-435] Add tracks and their metadata by roznawsk · Pull Request #141 · fishjam-dev/fishjam

roznawsk · 2024-01-16T12:09:02Z

Check out PR in protos:
#141

Acknowledging the stipulations set forth:

I hereby confirm that a Pull Request involving updates to the Software Development Kit (SDK) has been smoothly merged, currently awaits processing, or is otherwise deemed unnecessary in this context.
[ x ] I also affirm that another Pull Request, specifically addressing updates to the documentation body (commonly referred to as 'docs'), has either been successfully incorporated, is in the process of review, or is considered superfluous under the prevailing circumstances.

codecov · 2024-01-16T12:12:41Z

Codecov Report

Merging #141 (ff8556a) into main (8f6e1ca) will decrease coverage by 1.13%.
The diff coverage is 76.41%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
- Coverage   87.77%   86.65%   -1.13%     
==========================================
  Files          62       64       +2     
  Lines        1137     1221      +84     
==========================================
+ Hits          998     1058      +60     
- Misses        139      163      +24

Files	Coverage Δ
lib/jellyfish/component.ex	`100.00% <ø> (ø)`
lib/jellyfish/peer.ex	`85.71% <ø> (ø)`
lib/jellyfish/peer/webrtc.ex	`48.27% <ø> (ø)`
lib/jellyfish/room_service.ex	`86.30% <ø> (-0.19%)`	⬇️
lib/jellyfish/track.ex	`100.00% <100.00%> (ø)`
lib/jellyfish/webhook_notifier.ex	`78.94% <ø> (ø)`
lib/jellyfish_web/api_spec/component/file.ex	`100.00% <ø> (ø)`
lib/jellyfish_web/api_spec/component/hls.ex	`100.00% <ø> (ø)`
lib/jellyfish_web/api_spec/component/rtsp.ex	`100.00% <ø> (ø)`
lib/jellyfish_web/api_spec/peer.ex	`100.00% <100.00%> (ø)`
... and 8 more

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 8f6e1ca...ff8556a. Read the comment docs.

Karolk99

I believe there is no necessity to inform the user about the internal state of the engine, as I can't identify any practical use or reason for it. However, when it comes to peers, the situation is different. Tracks represent the connection between the client and Jellyfish; understanding the quantity and types of tracks added by the client is useful.

roznawsk · 2024-01-31T12:10:13Z

 openapi.yaml:
  no-empty-servers:
    - '#/servers'
+  spec:


Note: add lint rule, allowing for nullable field without type specified (metadata can have any type)

roznawsk · 2024-01-31T12:49:27Z

           handshake_opts: handshake_options,
           filter_codecs: filter_codecs,
           log_metadata: [peer_id: options.peer_id],
-           trace_context: nil,


note: option removed in new WebRTC endpoint

roznawsk · 2024-01-31T13:11:48Z

           webhook_url,
-           Jason.encode!(%{notification: notification}),
-           [{"Content-Type", "application/json"}]
+           notification,


per this thread we must set encoding to binary

Remember that it will require changes in SDKs.

Btw the more recent thread informs that some companies use different content-type, e.g.: Cloudflare uses x-protobuf.

So in the end you decided to use protobuf instead of x-protobuf or is it a typo?

Rados13 · 2024-02-01T09:18:58Z

+  defp to_proto_encoding(:H264), do: :ENCODING_H264
+  defp to_proto_encoding(:VP8), do: :ENCODING_VP8
+  defp to_proto_encoding(:OPUS), do: :ENCODING_OPUS
+  defp to_proto_encoding(_encoding), do: :ENCODING_UNSPECIFIED


Shouldn't we raise an error in a situation like this, as we don't support different types of tracks?

This is created from event sent from Rtc Engine, so I guess its better to have track with unknown encoding, than crashing Jellyfish.
Also, as you proposed I will remove the encoding from the Track anyway?

I am not sure about that. I think that if a jellyfish doesn't expect something from the engine, it should raise an error. I think it would be more aligned with the BEAM approach, Let it crash.

Rados13 · 2024-02-01T09:19:13Z

+
+  defp to_proto_track_type(:video), do: :TRACK_TYPE_VIDEO
+  defp to_proto_track_type(:audio), do: :TRACK_TYPE_AUDIO
+  defp to_proto_track_type(_type), do: :TRACK_TYPE_UNSPECIFIED


Rados13 · 2024-02-01T09:24:16Z

+  @impl true
+  def handle_info(%TrackAdded{endpoint_id: endpoint_id} = track_info, state) do
+    Logger.error("Unknown endpoint #{endpoint_id} added track #{inspect(track_info)}")
+    {:noreply, state}
+  end


I am not sure if the best way of handling the inconsistency of the state of the room is to log information about it and then simply ignore it.

I think this makes sense actually. I think we could expect this message if we delete Peer while it adds new track - we would receive TrackAdded and then EndpointRemoved messages.

If this is the case, then it shouldn't be an error because we expect that this can sometimes happen. I think we should modify the logic in room.ex, so we would remove endpoints from the state after we receive EndpointRemoved.

Rados13 · 2024-02-01T09:27:02Z

+      nil ->
+        Logger.warning(
+          "Unable to update track's metadata - track #{inspect(track_info.track_id)} doesn't exist"
+        )
+
+        {:noreply, state}


Doesn't a situation like that mean that we have some error? How could we receive a notification about a track that doesn't exist? I think we should raise an error here or use update_in instead of get_in.

Rados13 · 2024-02-01T09:28:17Z

+      nil ->
+        Logger.warning(
+          "Unable to remove track - track #{inspect(track_info.track_id)} doesn't exist"
+        )


Rados13 · 2024-02-01T09:28:39Z

+
+  @impl true
+  def handle_info(%TrackRemoved{endpoint_id: endpoint_id} = track_info, state) do
+    Logger.error("Unknown endpoint #{endpoint_id} removed track #{inspect(track_info)}")


As mentioned in TrackAdded.

Rados13 · 2024-02-01T09:41:14Z

           webhook_url,
-           Jason.encode!(%{notification: notification}),
-           [{"Content-Type", "application/json"}]
+           notification,


Remember that it will require changes in SDKs.

Rados13 · 2024-02-01T09:51:22Z

           webhook_url,
-           Jason.encode!(%{notification: notification}),
-           [{"Content-Type", "application/json"}]
+           notification,


Btw the more recent thread informs that some companies use different content-type, e.g.: Cloudflare uses x-protobuf.

Rados13 · 2024-02-05T10:21:42Z

      nil ->
        Logger.warning(
-          "Unable to update track's metadata - track #{inspect(track_info.track_id)} doesn't exist"
+          "Metadata updated of an unknown track #{inspect(track_info)}, new track added"
        )

-        {:noreply, state}
+        Track.from_track_message(track_info)


How can we first receive TrackMetadataUpdated before TrackAdded?

Rados13 · 2024-02-05T10:22:15Z

+      {nil, state} ->
        Logger.warning(
          "Unable to remove track - track #{inspect(track_info.track_id)} doesn't exist"
        )


How could we receive TrackRemoved before TrackAdded?

Rados13 · 2024-02-05T10:55:16Z

           webhook_url,
-           Jason.encode!(%{notification: notification}),
-           [{"Content-Type", "application/json"}]
+           notification,


So in the end you decided to use protobuf instead of x-protobuf or is it a typo?

…5-rest-track-metadata

Rados13 · 2024-02-05T12:11:24Z

+      assert %{
+               type: :TRACK_TYPE_AUDIO,
+               metadata: "null"
+             } = track


Couldn't we merge that with the previous assert?

Rados13 · 2024-02-06T14:03:48Z

-            {:track_removed, state.id, {:component_id, component_id}, &1}
-          )
-        )
-


Suggested change

track ->

%Track{}=track ->

Rados13

Only OpenAPI need fixes.

Add tracks to state

4187afa

Add track metadata

e89376e

roznawsk changed the title ~~Add tracks to state~~ [RTC-435] Add tracks and their metadata Jan 16, 2024

roznawsk marked this pull request as ready for review January 16, 2024 13:00

roznawsk requested review from Karolk99 and Rados13 January 16, 2024 13:00

Rados13 reviewed Jan 17, 2024

View reviewed changes

Comment thread lib/jellyfish/room.ex Outdated

Comment thread lib/jellyfish/room.ex Outdated

Comment thread lib/jellyfish/room.ex

Comment thread lib/jellyfish/room.ex

Comment thread lib/jellyfish_web/api_spec/track.ex Outdated

Rados13 assigned roznawsk Jan 17, 2024

Karolk99 reviewed Jan 17, 2024

View reviewed changes

roznawsk added 5 commits January 18, 2024 10:12

Log errors

ba816cf

Fix openapi lint

1f683ce

Add track meta

7a329cc

Add peer metadata

cae69a9

Test for message in FileComponent test

b6c7d95

roznawsk commented Jan 31, 2024

View reviewed changes

Improve handling Engine messages

122192c

roznawsk commented Jan 31, 2024

View reviewed changes

roznawsk force-pushed the RTC-435-rest-track-metadata branch from 3b1e180 to 356a0f5 Compare January 31, 2024 14:47

Improve logging

74081f5

roznawsk force-pushed the RTC-435-rest-track-metadata branch from 356a0f5 to 74081f5 Compare January 31, 2024 14:50

Remove unused deps

b5ce44f

roznawsk requested review from Karolk99 and Rados13 January 31, 2024 14:59

roznawsk mentioned this pull request Jan 31, 2024

[RTC-444] Track notifications fishjam-dev/protos#15

Merged

RTC-engine dependency to master

41c17a0

Rados13 reviewed Feb 1, 2024

View reviewed changes

roznawsk added 2 commits February 2, 2024 14:26

Improve handling unexpected messages

e74f0d3

Remove track encoding

abbddef

roznawsk force-pushed the RTC-435-rest-track-metadata branch from de79057 to abbddef Compare February 2, 2024 13:36

roznawsk requested a review from Rados13 February 2, 2024 13:37

roznawsk mentioned this pull request Feb 2, 2024

[RTC-435] Add Peer and Track metadata fishjam-dev/elixir_server_sdk#51

Merged

Karolk99 approved these changes Feb 5, 2024

View reviewed changes

Rados13 reviewed Feb 5, 2024

View reviewed changes

roznawsk added 4 commits February 6, 2024 11:20

Remove unused cases

6877c49

Fix logging

ba56203

Merge branch 'main' of github.com:jellyfish-dev/jellyfish into RTC-43…

8c4058e

…5-rest-track-metadata

Remove unused deps

02626a4

roznawsk requested a review from Rados13 February 6, 2024 10:47

roznawsk added 2 commits February 6, 2024 11:56

Format

634e213

Fix sending track_removed notifications

76ca6c1

roznawsk force-pushed the RTC-435-rest-track-metadata branch from 2d988ac to 76ca6c1 Compare February 6, 2024 14:34

Rados13 reviewed Feb 7, 2024

View reviewed changes

Rados13 approved these changes Feb 7, 2024

View reviewed changes

roznawsk added 3 commits February 7, 2024 10:42

PR remarks

19f8318

Synchronize protos

466931a

Add lint ignores

ff8556a

roznawsk merged commit 36e1d00 into main Feb 7, 2024

roznawsk deleted the RTC-435-rest-track-metadata branch February 7, 2024 15:20

Conversation

roznawsk commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Acknowledging the stipulations set forth:

Uh oh!

codecov bot commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Karolk99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rados13 Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rados13 Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rados13 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

roznawsk commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

Rados13 Feb 1, 2024 •

edited

Loading

Rados13 Feb 1, 2024 •

edited

Loading