FBS: WIP, scope notifications #1162

jmillan · 2023-09-20T07:59:06Z

Scope notifications to the class to which they are being received.

This adds a level of indirection to the overall message but enhances the design of the flatbuffer types and allows exhausing the 'switch' statements in the processing blocks.

# Conflicts: # worker/Makefile

# Conflicts: # worker/src/RTC/RtpStream.cpp

…ize options (#1084)

# Conflicts: # .github/workflows/mediasoup-node.yaml # .gitignore # CHANGELOG.md # npm-scripts.js # worker/src/lib.cpp

# Conflicts: # CHANGELOG.md # npm-scripts.mjs # package.json

# Conflicts: # npm-scripts.mjs

Already thown by flatbuffers.

…er (#1104)

# Conflicts: # CHANGELOG.md

# Conflicts: # worker/src/RTC/RtpStreamRecv.cpp

…st-wip

And enable 'cargo test' workflow in GH actions.

As explained in #1064 (comment) and messages below it

# Conflicts: # CHANGELOG.md

As Nazar said, for now the process is manual: 1. In `rust` directory run this: `planus rust -o src/fbs.rs ../worker/fbs/*` 2. Then revert some of the changes at the top of the file to make sure it re-exports things the same way and suppresses clippy warnings in there. 3. For ^ to work you might need to install it first with `cargo install planus-cli` (this will download, compile and install binary in Cargo's directory).

fbs: FBS WebRtcTransport --------- Co-authored-by: Iñaki Baz Castillo <[email protected]>

Rust: port transport methods to FBS

Rust: port transport notifications to FBS

* FBS: normalize FBS types * FBS: format schemas * FBS: type for RtpHeaderExtensionUri (#1161)

Scope notifications to the class to which they are being received. This adds a level of indirection to the overall message but enhances the design of the flatbuffer types and allows exhausing the 'switch' statements in the processing blocks.

jmillan

@ibc, @nazar-pc, this is a WIP where we are scoping the notifications to each corresponding class. It adds a level of indirection to the overall message but IMHO it's worth it.

Similarly will be done with Request/Responses. Please Let me know your opinions.

jmillan · 2023-09-20T08:00:26Z

rust/src/router/plain_transport.rs

-                let tuple_fbs = transport::Tuple::try_from(body.tuple().unwrap()).unwrap();
-                let tuple = TransportTuple::from_fbs(&tuple_fbs);
+        match notification {
+            plain_transport::Notification::Tuple(notification) => {


Switch cases only contain the specific PlainTransport Notification.

jmillan · 2023-09-20T08:00:48Z

rust/src/router/plain_transport.rs

+                };
+
+                // Get the Notification instance.
+                let notification = notification_wrapper


Get the PlainTransport Notification out of the wrapper.

jmillan · 2023-09-20T08:01:45Z

worker/fbs/notification.fbs

@@ -55,6 +55,7 @@ union Body {
    DataProducer_SendNotification: FBS.DataProducer.SendNotification,

    // Notifications from worker.
+    PlainTransport: FBS.PlainTransport.NotificationWrapper,


This type (Notification::Body) will only have the respective namespaces (PlainTransport, DirectTransport, Producer, Consumer, etc), not the specific notifications anymore.

jmillan · 2023-09-20T08:02:00Z

worker/fbs/plainTransport.fbs

@@ -46,6 +46,17 @@ table GetStatsResponse {

 // Notifications from Worker.

+union Notification {


PlainTransort Notification type.

jmillan · 2023-09-20T08:02:50Z

worker/fbs/plainTransport.fbs

+   Trace: FBS.Transport.TraceNotification,
+}
+
+table NotificationWrapper {


This wrapper is needed since Notification, which is a union cannot be used by itself, it needs to be contained within a table.

jmillan · 2023-09-20T08:03:05Z

worker/src/RTC/PlainTransport.cpp

@@ -1188,11 +1188,17 @@ namespace RTC
 		auto notification = FBS::PlainTransport::CreateTupleNotification(
 		  this->shared->channelNotifier->GetBufferBuilder(), tuple);

+		auto notificationWrapper = FBS::PlainTransport::CreateNotificationWrapper(


Wrap Notification into NotificationWrapper.

ibc · 2023-09-20T10:36:10Z

I like it.

nazar-pc

Overall makes sense

nazar-pc · 2023-09-30T04:45:47Z

rust/src/router/plain_transport.rs

+                    .try_into()
+                    .unwrap();


Why is try_into needed here? Reading just on GitHub this looks a bit confusing.

This is getting a Notification object out a NotificationRef. Otherwise each field in NotificationRef is accessed via a method with the field name, which returns a result with the value. IMHO it's too cluttered.

NOTE: We will finally use NotificationRef instead as noted here

jmillan · 2024-10-15T08:42:12Z

This draft is definitely not usable. We would have to create a new one if we want to accomplish this. I'm closing as the corresponding issue is already created.

jmillan and others added 30 commits April 18, 2023 12:38

flatbuffers

0a88526

Merge branch 'v3' into flatbuffers

655c601

# Conflicts: # worker/Makefile

Update NPM flatbuffers dep

add71dc

Merge branch 'v3' into flatbuffers

0428b72

# Conflicts: # worker/src/RTC/RtpStream.cpp

[flatbuffers] Add ListenInfo in all transports and send/recv buffer s…

148090a

…ize options (#1084)

Merge branch 'v3' into flatbuffers

7f2be00

Merge branch 'v3' into flatbuffers

e30f8a7

# Conflicts: # .github/workflows/mediasoup-node.yaml # .gitignore # CHANGELOG.md # npm-scripts.js # worker/src/lib.cpp

Merge branch 'v3' into flatbuffers

ea75df2

# Conflicts: # CHANGELOG.md # npm-scripts.mjs # package.json

Merge branch 'v3' into flatbuffers

64b4705

# Conflicts: # npm-scripts.mjs

PlainTransportOptions: add optional rtcpListenInfo (#1099)

6594f07

Remove explicit TypeError exceptions

25ff0f0

Already thown by flatbuffers.

Rust: WIP

7a92969

Merge branch 'v3' into flatbuffers

ab1dfac

[flatbuffers] Add pause() and resume() in DataProducer and DataConsum…

4b104f6

…er (#1104)

Merge branch 'v3' into flatbuffers

f300103

# Conflicts: # CHANGELOG.md

Merge branch 'v3' into flatbuffers

7510950

Merge branch 'v3' into flatbuffers

878c9d2

Merge branch 'v3' into flatbuffers

ae4c6cc

# Conflicts: # CHANGELOG.md

Merge branch 'v3' into flatbuffers

9eccfa9

# Conflicts: # CHANGELOG.md

Merge branch 'v3' into flatbuffers

ad3d6b0

Merge branch 'v3' into flatbuffers

1892a88

Merge branch 'v3' into flatbuffers

6f0adba

Merge branch 'v3' into flatbuffers

7beff59

# Conflicts: # worker/src/RTC/RtpStreamRecv.cpp

Merge remote-tracking branch 'origin/flatbuffers' into flatbuffers-ru…

253371f

…st-wip

rust: remove unneeded file

14f736d

rust: disable tests for unimplemented code

da152f7

And enable 'cargo test' workflow in GH actions.

Merge branch 'v3' into flatbuffers

f28d922

cosmetic

8b23595

Merge branch 'v3' into flatbuffers

6cc984c

Fix DirectTransport test when using pause/resume()

4037935

As explained in #1064 (comment) and messages below it

ibc and others added 21 commits August 11, 2023 11:34

Merge branch 'v3' into flatbuffers

ab0bd70

# Conflicts: # CHANGELOG.md

Merge branch 'v3' into flatbuffers

a1a81fd

Merge branch 'v3' into flatbuffers

8d5d4b9

# Conflicts: # CHANGELOG.md

rust: move PlainTransport to flatbuffers (#1135)

2dc44f9

Use FBS Transport::Tuple everywhere (#1148)

a23fd0e

rust: move PipeTransport to flatbuffers (#1149)

258c17c

Merge remote-tracking branch 'origin/v3' into flatbuffers

1c5cf53

Merge branch 'v3' into flatbuffers

008ac96

# Conflicts: # CHANGELOG.md

merge

7651f97

DataChannel subchannels (#1152)

2798355

Update CHANGELOG

1bebb87

[flatbuffers] Update rust deps (#1155)

8435d1c

cosmetic: add blank line

6f12821

fbs: FBS WebRtcTransport (#1153)

0f137a7

fbs: FBS WebRtcTransport --------- Co-authored-by: Iñaki Baz Castillo <[email protected]>

Make DTLS fragment stay within MTU size range (#1156)

8472b90

Rust: port transport methods to FBS (#1157)

80e81a0

Rust: port transport methods to FBS

Jest coverage: ignore tests/ and fbs/ folders

949bae3

Rust: port transport notifications to FBS (#1158)

67dd174

Rust: port transport notifications to FBS

FBS: normalize FBS types (#1159)

d57e3b0

* FBS: normalize FBS types * FBS: format schemas * FBS: type for RtpHeaderExtensionUri (#1161)

FBS: WIP, scope notifications

62615a4

Scope notifications to the class to which they are being received. This adds a level of indirection to the overall message but enhances the design of the flatbuffer types and allows exhausing the 'switch' statements in the processing blocks.

jmillan force-pushed the wip-notification-scoping branch from 07b43d5 to 62615a4 Compare September 20, 2023 07:59

jmillan commented Sep 20, 2023

View reviewed changes

nazar-pc approved these changes Sep 30, 2023

View reviewed changes

jmillan mentioned this pull request Oct 25, 2023

flatbuffers #1064

Merged

7 tasks

Base automatically changed from flatbuffers to v3 November 8, 2023 10:00

jmillan mentioned this pull request Nov 8, 2023

Flatbuffers: Scope notifications/request per instance #1213

Open

jmillan closed this Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FBS: WIP, scope notifications #1162

FBS: WIP, scope notifications #1162

jmillan commented Sep 20, 2023

jmillan left a comment

jmillan Sep 20, 2023

jmillan Sep 20, 2023

jmillan Sep 20, 2023

jmillan Sep 20, 2023

jmillan Sep 20, 2023 •

edited

Loading

jmillan Sep 20, 2023

ibc commented Sep 20, 2023

nazar-pc left a comment

nazar-pc Sep 30, 2023

jmillan Oct 2, 2023

jmillan Oct 25, 2023

jmillan commented Oct 15, 2024

		@@ -46,6 +46,17 @@ table GetStatsResponse {

		// Notifications from Worker.

		union Notification {

FBS: WIP, scope notifications #1162

FBS: WIP, scope notifications #1162

Conversation

jmillan commented Sep 20, 2023

jmillan left a 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

jmillan Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibc commented Sep 20, 2023

nazar-pc left a 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

jmillan commented Oct 15, 2024

jmillan Sep 20, 2023 •

edited

Loading