Flatbuffers #927

jmillan · 2022-10-19T13:30:53Z

NOTE: THIS PR IS DEPRECATED IN FAVOUR OF #1063

WIP to remove JSON in favour of flatbuffers for as the worker communication format.

JSON parsing is CPU intensive.
JSON is not type safe.
flatbuffers does not parse the buffer. Reading the message takes 0ms.
flatbuffers is type safe since code (C++, Typescript, Rust, etc) is autogenerated out of the schema definitions.

Extras that come along with this changes:

There is now a single Channel for worker communication.
- Previously there were 2 (Channel and PayloadChannel)
Each message sent Node->Worker requires a single write() call to Channel.
- Previously 2 calls per message were needed for Channel and 4 for PayloadChannel.
Each message send Worker->Node previously executed two extra calls to memcpy that are now executed now.

Feature request for making optional field getters return undefined instead of null if the value is not set.

update flatbuffers meson wrapdb in order to use latest version PR ready
Instead of creating a vector for uint8*, make a memcpy in C++ and the correponding Buffer copy in TS.
Consider whether it's worth it avoiding to create a new Uint8Array out of the flatbuffers builder each time we send data to worker.
Remove temporal checks in TS once this issue is fixed.
Planus not supporting same table name in different namespaces.
flatbuffers (TS) should throw TypeError instead of Error when there is a missing required field. PR

ibc

Looks amazing. Just a few comments or questions basically to understand things.

fbs/consumer.fbs

fbs/rtpParameters.fbs

node/src/Channel.ts

node/src/Transport.ts

ibc · 2022-10-19T14:55:59Z

Why such a double /fbs/fbs/ path? I know why and wonder whether we should rename the root one to flatbuffers.

jmillan · 2022-10-19T15:58:04Z

For some reason I don't understand libwebrtc dependency is not being compiled with -DMS_LOG_STD and -DMS_TEST flags, hence the worker tests are failing.

Any idea why those flags are suddenly not being passed to this dependency? I've just added a new dependency (flatbuffers) within this PR and I can't see how it could affect it.

cc @nazar-pc if you can please comment.

NOTE: It is not a blocker, I can still go on with this PR.

nazar-pc · 2022-10-21T11:00:50Z

Any idea why those flags are suddenly not being passed to this dependency? I've just added a new dependency (flatbuffers) within this PR and I can't see how it could affect it.

Are you sure it worked before then? I see nothing that would have affected it.

jmillan · 2022-10-21T11:46:15Z

Are you sure it worked before then? I see nothing that would have affected it.

It works in v3 indeed. Otherwise the GH actions would fail the same way.

nazar-pc · 2022-10-21T11:51:13Z

I see CI fails due to segfault when running tests, why do you think it is related to those constants?

jmillan · 2022-10-21T12:59:36Z

why do you think it is related to those constants?

I'm certain.

if you add '-v' option to meson compile statement in test: setup target of worker/Makefile you'll see that libwebrtc is compiled without such flags.

Then when running the test executable mediasoup initializes DepLibwebrtc, this generates a log that instead of stdout goes to a non existent Channel , hence the core dump.

I've verified it compiling the debug symbols and running the test binary with lldb

jmillan · 2022-10-21T14:06:48Z

I'm checking the rest of warnings too, which are probably related to the compiler versions used in GH actions.

nazar-pc · 2022-10-23T14:56:45Z

I have no idea why it stops adding constants, but should be possible to debug by incrementally modifying meson.build files in v3 branch and see at which point it breaks.

jmillan · 2022-10-23T14:58:38Z

I have no idea why it stops adding constants, but should be possible to debug by incrementally modifying meson.build files in v3 branch and see at which point it breaks.

Yes, I need to debug it before continuing pushing further commits here.

jmillan · 2022-10-24T10:57:19Z

3eb3813 creates a new libwebrtc_test dependency with the required 'cpp_flags'. 'mediasoup_worker_test' makes use of such dependency instead of the non test 'libwebrtc' dependency.

As indicated in the commit comment, I don't know how it worked before, but as I understand 'cpp_args' are not passed to subprojects.

nazar-pc · 2022-10-24T22:44:08Z

As indicated in the commit comment, I don't know how it worked before, but as I understand 'cpp_args' are not passed to subprojects.

Talks to Meson folks on Matrix, they're pretty responsive there and might help

ibc

Some preliminary comments but I need a call to discuss in a call

fbs/common.fbs

fbs/request.fbs

fbs/response.fbs

This way we make sure no old and unused files are kept

jmillan · 2023-04-13T17:42:51Z

IMHO the whole generation of the flatbuffers request body (before sending it) should be wrapped with try/catch and the catch block should generate a

IMHO it would be better to provide a patch to flatbuffers so it throws optionally TypeError instead of Error here. I'm adding this task in the issue description. Otherwise we will fill everything with these try/catch blocks.

PR: google/flatbuffers#7910

jmillan · 2023-04-13T18:46:32Z

@nazar-pc, could you please check the warning given by clippy? Following the compiler suggestions leads me to an endless loop.

ibc · 2023-04-13T19:54:07Z

PR: google/flatbuffers#7910

Once that PR is merged and released and we upgrade flatbuffers dep, there is a test in mediasoup that will fail. Easy fix. It's has a note about this.

nazar-pc · 2023-04-14T01:53:53Z

@nazar-pc, could you please check the warning given by clippy? Following the compiler suggestions leads me to an endless loop.

Please push what you have before Saturday (I'll be mostly unavailable today) and I'll try to make some progress here. Not sure how much, but it'll be helpful for me to look at changes as a whole rather than patching tiny things here and there.

jmillan · 2023-04-14T06:37:40Z

@nazar-pc, could you please check the warning given by clippy? Following the compiler suggestions leads me to an endless loop.

Please push what you have before Saturday (I'll be mostly unavailable today) and I'll try to make some progress here. Not sure how much, but it'll be helpful for me to look at changes as a whole rather than patching tiny things here and there.

This branch is up to date.

Workaround until planus-org/planus#180 is fixed. This change keeps compatibility with C++ and TS.

jmillan · 2023-04-14T10:41:57Z

I'll try to make some progress here

I'll be out for the weekend but I'll be available during the week. We can sync anytime.

nazar-pc · 2023-04-15T21:39:47Z

I am reading through changes now and trying to understand what is going on and I see things that appear to be unrelated to this branch.

For example ConsumerOptions::enable_rtx was added, was it added because it was missing in one of the PRs in v3? If so it needs to be added to v3 and merged into here instead.

There are also a bunch of things edited under .github/workflows, none of which seem necessary (concurrency should have been added to v3 instead).

libwebrtc_test_dep is added for some reason, why not using libwebrtc_dep the way it was before?

BUILD_DIR ?= $(INSTALL_DIR)/build is interesting, it looks fine on surface, but what if you don't want to build in the same directory as you install stuff into? MEDIASOUP_OUT_DIR was specifically for build output, it is still where pip is stored.

General comments on the whole branch: 319 commits is completely insane and unmanageable at all, try to use rebase and squash features of git. You develop incrementally locally, but you don't need to push every though and every experiment to GitHub. Squash things into locally structured and named commits that can be reviewed individually. When something changes in v3, rebase on it, so we have up to date starting point for the whole branch and clean changes on top. Right now it looks more or less like a stream of consciousness and the only way to deal with it is to look at the whole thing, but then I don't have the context of why many of the things changed.

UPD: Looks like IDEA can't even compare the branches properly anymore, it essentially shows the wrong diff, so some of above changes are not really the changes that are present 😕

…includes (#1062)

jmillan · 2023-04-17T08:50:18Z

rust/src/fbs.rs

+//! Flatbuffers data structures compiled from `.fbs` files
+pub use root::fbs::*;

 const _: () = ::planus::check_version_compatibility("planus-0.3.1");

 #[no_implicit_prelude]
 mod root {
+    #[allow(missing_docs)]
+    #[allow(clippy::all)]


This file is auto-generated. Do we need to add this changes manually when the flatbuffers schema changes?

I believe the way you had it before could suppress the whole project. This will is currently committed into the repo, but it will be replaced with actually auto-generated during compile time in mediasoup-sys in the future, so these changes are temporary either way.

jmillan · 2023-04-17T08:51:22Z

rust/src/fbs.rs

@@ -1,9 +1,12 @@
-pub use root::*;
+//! Flatbuffers data structures compiled from `.fbs` files
+pub use root::fbs::*;


ibc · 2023-04-17T09:01:31Z

For example ConsumerOptions::enable_rtx was added, was it added because it was missing in one of the PRs in v3? If so it needs to be added to v3 and merged into here instead.

Probably we forgot to add that field in ConsumerOptions in Rust in the past. Yes, I agree that this should have been done in v3 first. But we must be pragmatic. Those non urgent/critical changes in v3 may later generate conflicts when it comes to merge flatbuffers into v3.

General comments on the whole branch: 319 commits is completely insane and unmanageable at all, try to use rebase and squash features of git. You develop incrementally locally, but you don't need to push every though and every experiment to GitHub. Squash things into locally structured and named commits that can be reviewed individually. When something changes in v3, rebase on it, so we have up to date starting point for the whole branch and clean changes on top. Right now it looks more or less like a stream of consciousness and the only way to deal with it is to look at the whole thing, but then I don't have the context of why many of the things changed.

Would it help if we rebase flatbuffers into a few commits with descriptive messages and force push it?

jmillan · 2023-04-17T09:23:50Z

I'm trying to reduce the history of this branch by squashing commits...

nazar-pc · 2023-04-17T09:28:49Z

Would it help if we rebase flatbuffers into a few commits with descriptive messages and force push it?

Absolutely, but not sure how much time that'd require at this point

jmillan · 2023-04-17T10:03:08Z

It's certainly impossible in terms of spent time to do squash commits at this point.

My only sane proposal is:

Create a new branch flatbuffers-new out of v3.
merge flatbuffers branch into it.
reset the last commit (the whole difference between flatbuffers and v3)
commit all diff into a single commit "flatbuffers".
remove flatbuffers branch.
rename flatbuffers-new to flatbuffers.

We would have a single commit with all the changes performed in this branch. IMHO, up to this point is the safest and cleanest option.

Do we agree?

nazar-pc · 2023-04-17T10:05:25Z

I don't think it would be so much better, but it would likely fix my IDE showing correct diff.

Please don't delete old flatbuffers branch though, I have created bug report into IntelliJ IDEA, hopefully they'll try to reproduce it and having branch around would be helpful for them.

jmillan · 2023-04-17T10:28:22Z

NOTE: THIS PR IS DEPRECATED IN FAVOUR OF #1063

Please, don't push anything in flatbuffers branch but in flatbuffers-new.

I'm closing this PR.

@nazar-pc, I'm renaming this branch flatbuffers-old so the new branch is called flatbuffers. I'm telling this so you can update the issue you opened in IntelliJ IDEA.

jmillan · 2023-04-17T10:29:54Z

@nazar-pc, I'm renaming this branch flatbuffers-old so the new branch is called flatbuffers. I'm telling this so you can update the issue you opened in IntelliJ IDEA.

here:
https://github.com/versatica/mediasoup/tree/flatbuffers-old

ibc · 2023-04-17T10:31:42Z

I'm renaming this branch flatbuffers-old so the new branch is called flatbuffers

Please notify when done so I can retarget my PR.

jmillan · 2023-04-17T10:54:41Z

Please notify when done so I can retarget my PR.

It's done.

ibc reviewed Oct 19, 2022

View reviewed changes

restore debug modification

0ead186

ibc reviewed Oct 28, 2022

View reviewed changes

fbs/common.fbs Outdated Show resolved Hide resolved

fbs/request.fbs Outdated Show resolved Hide resolved

fbs/request.fbs Outdated Show resolved Hide resolved

fbs/response.fbs Outdated Show resolved Hide resolved

flatbuffers: Make all response body types contain the 'Response' sufix

c98f8a1

ibc reviewed Oct 28, 2022

View reviewed changes

fbs/response.fbs Outdated Show resolved Hide resolved

jmillan added 14 commits November 2, 2022 10:43

flatbuffers schemas cleanup

271fa93

transport.dump() maps are now arrays

873b91a

Router: check numSctpStreams param

6a3db2f

worker: format

34a3cc7

remove fbs/utils.ts file

32129fd

remove flatbuffers auto generated folder before re-generating them

0324d9f

This way we make sure no old and unused files are kept

Transport.dump(), traceEventTypes contains a string array

1bfa2c6

WebRtcTransport tests pass

8fc6000

flatbuffers: allow same type name in different namespace

b5d4a7e

lint

a98a2c7

schema 'required' label consistency

8cfc859

utils: parserVector optional argument to parse each instance

0c3ece3

PlainTransport: Creation and Dump

3ae27fb

Worker: remove old code

14d6778

jmillan added 3 commits April 13, 2023 19:49

Fix response type

03e5575

worker: create_router()

de9f705

clippy

f2c4160

jmillan added 2 commits April 13, 2023 20:46

clippy

50c34bb

remove unused import

111cdec

jmillan added 2 commits April 14, 2023 10:44

set lint attribute for fbs out of the autogenerated file

363b162

fbs: use alias on response body variants

4e247e3

Workaround until planus-org/planus#180 is fixed. This change keeps compatibility with C++ and TS.

run cargo fmt

68b52a0

nazar-pc added 2 commits April 16, 2023 01:18

Minimal cleanup, clippy is happy now

11c8663

Replace static variables with regular class variables, remove unused …

e858e5c

…includes (#1062)

jmillan commented Apr 17, 2023

View reviewed changes

jmillan closed this Apr 17, 2023

jmillan deleted the flatbuffers branch April 17, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatbuffers #927

Flatbuffers #927

jmillan commented Oct 19, 2022 •

edited

Loading

ibc left a comment

ibc commented Oct 19, 2022

jmillan commented Oct 19, 2022

nazar-pc commented Oct 21, 2022

jmillan commented Oct 21, 2022

nazar-pc commented Oct 21, 2022 •

edited

Loading

jmillan commented Oct 21, 2022

jmillan commented Oct 21, 2022

nazar-pc commented Oct 23, 2022

jmillan commented Oct 23, 2022

jmillan commented Oct 24, 2022

nazar-pc commented Oct 24, 2022

ibc left a comment

jmillan commented Apr 13, 2023

jmillan commented Apr 13, 2023

ibc commented Apr 13, 2023

nazar-pc commented Apr 14, 2023

jmillan commented Apr 14, 2023

jmillan commented Apr 14, 2023

nazar-pc commented Apr 15, 2023 •

edited

Loading

jmillan Apr 17, 2023

nazar-pc Apr 17, 2023

jmillan Apr 17, 2023

jmillan Apr 17, 2023

ibc commented Apr 17, 2023

jmillan commented Apr 17, 2023

nazar-pc commented Apr 17, 2023

jmillan commented Apr 17, 2023 •

edited

Loading

nazar-pc commented Apr 17, 2023

jmillan commented Apr 17, 2023

jmillan commented Apr 17, 2023

ibc commented Apr 17, 2023

jmillan commented Apr 17, 2023

Flatbuffers #927

Flatbuffers #927

Conversation

jmillan commented Oct 19, 2022 • edited Loading

ibc left a comment

Choose a reason for hiding this comment

ibc commented Oct 19, 2022

jmillan commented Oct 19, 2022

nazar-pc commented Oct 21, 2022

jmillan commented Oct 21, 2022

nazar-pc commented Oct 21, 2022 • edited Loading

jmillan commented Oct 21, 2022

jmillan commented Oct 21, 2022

nazar-pc commented Oct 23, 2022

jmillan commented Oct 23, 2022

jmillan commented Oct 24, 2022

nazar-pc commented Oct 24, 2022

ibc left a comment

Choose a reason for hiding this comment

jmillan commented Apr 13, 2023

jmillan commented Apr 13, 2023

ibc commented Apr 13, 2023

nazar-pc commented Apr 14, 2023

jmillan commented Apr 14, 2023

jmillan commented Apr 14, 2023

nazar-pc commented Apr 15, 2023 • edited Loading

jmillan Apr 17, 2023

Choose a reason for hiding this comment

nazar-pc Apr 17, 2023

Choose a reason for hiding this comment

jmillan Apr 17, 2023

Choose a reason for hiding this comment

jmillan Apr 17, 2023

Choose a reason for hiding this comment

ibc commented Apr 17, 2023

jmillan commented Apr 17, 2023

nazar-pc commented Apr 17, 2023

jmillan commented Apr 17, 2023 • edited Loading

nazar-pc commented Apr 17, 2023

jmillan commented Apr 17, 2023

jmillan commented Apr 17, 2023

ibc commented Apr 17, 2023

jmillan commented Apr 17, 2023

jmillan commented Oct 19, 2022 •

edited

Loading

nazar-pc commented Oct 21, 2022 •

edited

Loading

nazar-pc commented Apr 15, 2023 •

edited

Loading

jmillan commented Apr 17, 2023 •

edited

Loading