Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataChannel subchannels #1152

Merged
merged 11 commits into from
Sep 11, 2023
Merged

DataChannel subchannels #1152

merged 11 commits into from
Sep 11, 2023

Conversation

ibc
Copy link
Member

@ibc ibc commented Sep 7, 2023

Fixes #1151

  • Node: transport.consumeData({ subchannels }).
  • Node: dataConsumer.subchannels getter.
  • Node: dataConsumer.setSubchannels() method.
  • Node: Proper test in test-DataConsumer.ts.
  • Node: dataProducer.send(message, ppid, subchannels, requiredSubchannel) (new args).
  • Node: Proper test for dataProducer.send() with DataConsumers subscribed to specific subchannels.
  • Worker: Make subchannels work (hehe).
  • Rust: transport.consumeData({ subchannels }).
  • Rust: dataConsumer.subchannels getter.
  • Rust: dataConsumer.setSubchannels() method.
  • Rust: dataProducer.send(message, ppid, subchannels, requiredSubchannel) (new args).
  • Rust: Replicate Node tests.

@ibc ibc changed the title Datachannel subchannels DataChannel subchannels Sep 7, 2023
@ibc ibc changed the base branch from v3 to flatbuffers September 7, 2023 10:00
@ibc
Copy link
Member Author

ibc commented Sep 7, 2023

NOTE: Fixed, see UPDATE at the end of this comment.

Currently dealing with this error:

node/src/DataConsumer.ts:693:3 - error TS2322: Type 'number | null' is not assignable to type 'number[]'.
  Type 'null' is not assignable to type 'number[]'.

693   subchannels        : data.subchannels()
      ~~~~~~~~~~~

  node/src/DataConsumer.ts:103:2
    103  subchannels: number[];
         ~~~~~~~~~~~
    The expected type comes from property 'subchannels' which is declared here on type 'DataConsumerDump'

node/src/DataConsumer.ts:693:29 - error TS2554: Expected 1 arguments, but got 0.

693   subchannels        : data.subchannels()
                                ~~~~~~~~~~~~~

  node/src/fbs/data-consumer/dump-response.ts:76:13
    76 subchannels(index: number):number|null {
                   ~~~~~~~~~~~~~
    An argument for 'index' was not provided.

node/src/Transport.ts:1735:59 - error TS2345: Argument of type 'number[]' is not assignable to parameter of type 'number'.

1735   FbsTransport.ConsumeDataRequest.addSubchannels(builder, subchannels);
                                                               ~~~~~~~~~~~

I've just added subchannels optional array of numbers in DataConsumerOptions and mandatory subchannels array of numbers in DumpResponse in dataConsumer.fbs, but this is missing: https://github.com/versatica/mediasoup/pull/1152/files#diff-3253c176609c972f1279e2a6c6ca4c52cbfac8423efcc8b2891b9d8dbd92c358R1733

UPDATE: First issue fixed here thanks to @jmillan: 6fca2b3

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far 👍

@ibc
Copy link
Member Author

ibc commented Sep 7, 2023

NOTE: Fixed, see UPDATE at the end of this comment.

In this commit 2f5399d I'm trying to process subchannels optional array in DataConsumer C++ constructor. However test fails:

 FAIL  node/src/tests/test-DataConsumer.ts
  ✕ transport.consumeData() succeeds (2 ms)
  ✕ dataConsumer.dump() succeeds
  ✕ dataConsumer.getStats() succeeds
  ✕ transport.consumeData() on a DirectTransport succeeds (1 ms)
  ✕ dataConsumer.dump() on a DirectTransport succeeds (1 ms)
  ✕ dataConsumer.getStats() on a DirectTransport succeeds
  ✕ dataConsumer.pause() and resume() succeed
  ✕ dataConsumer.close() succeeds (1 ms)
  ✕ Consumer methods reject if closed (1 ms)
  ✕ DataConsumer emits "dataproducerclose" if DataProducer is closed
  ✕ DataConsumer emits "transportclose" if Transport is closed (1 ms)

  ● transport.consumeData() succeeds

    TypeError: FlatBuffers: field 18 must be set

      142 |   builder.requiredField(offset, 6) // data_producer_id
      143 |   builder.requiredField(offset, 8) // type
    > 144 |   builder.requiredField(offset, 18) // subchannels
          |           ^
      145 |   return offset;
      146 | }
      147 |

      at Builder.requiredField (node_modules/flatbuffers/js/builder.js:408:19)
      at Function.endConsumeDataRequest (node/src/fbs/transport/consume-data-request.ts:144:11)
      at createConsumeDataRequest (node/src/Transport.ts:1740:41)
      at PlainTransport.<anonymous> (node/src/Transport.ts:1159:25)
      at node/src/Transport.ts:8:71
      at Object.<anonymous>.__awaiter (node/src/Transport.ts:4:12)
      at PlainTransport.consumeData (node/src/Transport.ts:571:16)
      at node/src/tests/test-DataConsumer.ts:52:35
      at node/src/tests/test-DataConsumer.ts:8:71
      at Object.<anonymous>.__awaiter (node/src/tests/test-DataConsumer.ts:4:12)
      at Object.<anonymous> (node/src/tests/test-DataConsumer.ts:46:53)

And if in the first test in test-DataConsumer.ts I pass an empty subchannels: [] in transport.consumeData() then I get this different error:

  ● transport.consumeData() succeeds

    TypeError: FlatBuffers: object serialization must not be nested.

      126 | static createSubchannelsVector(builder:flatbuffers.Builder, data:number[]|Uint8Array):flatbuffers.Offset;
      127 | static createSubchannelsVector(builder:flatbuffers.Builder, data:number[]|Uint16Array|Uint8Array):flatbuffers.Offset {
    > 128 |   builder.startVector(2, data.length, 2);
          |           ^
      129 |   for (let i = data.length - 1; i >= 0; i--) {
      130 |     builder.addInt16(data[i]!);
      131 |   }

      at Builder.notNested (node_modules/flatbuffers/js/builder.js:244:19)
      at Builder.startVector (node_modules/flatbuffers/js/builder.js:421:14)
      at Function.createSubchannelsVector (node/src/fbs/transport/consume-data-request.ts:128:11)
      at createConsumeDataRequest (node/src/Transport.ts:1733:61)
      at PlainTransport.<anonymous> (node/src/Transport.ts:1159:25)
      at node/src/Transport.ts:8:71
      at Object.<anonymous>.__awaiter (node/src/Transport.ts:4:12)
      at PlainTransport.consumeData (node/src/Transport.ts:571:16)
      at node/src/tests/test-DataConsumer.ts:52:35
      at node/src/tests/test-DataConsumer.ts:8:71
      at Object.<anonymous>.__awaiter (node/src/tests/test-DataConsumer.ts:4:12)
      at Object.<anonymous> (node/src/tests/test-DataConsumer.ts:46:53)

UPDATE: Fixed in 6b83027

  1. Vectors in FBS are required despite (required) is not indicated.
  2. Vectors must be serialized before building the whole FBS request.

@@ -93,24 +93,34 @@ namespace RTC
{
MS_TRACE();

flatbuffers::Offset<FBS::SctpParameters::SctpStreamParameters> sctpStreamParametersOffset;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed Offset suffix to be consistent. For instance, in Producer::FillBuffer() we don't add Offset suffix anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmillan I think we should decide whether Offset suffix must be added or not. I see equivalent places where it's added and others in which it's not. IMHO it's redundant and unsightly and we should remove it from everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree.

@ibc ibc mentioned this pull request Sep 7, 2023
7 tasks
@ibc ibc marked this pull request as ready for review September 8, 2023 11:39
@ibc
Copy link
Member Author

ibc commented Sep 8, 2023

Node and C++ completely done and tests (see test in test-DirectTransport.ts).

So this is ready for review, @jmillan.

@ibc ibc requested a review from jmillan September 8, 2023 11:50
@ibc
Copy link
Member Author

ibc commented Sep 8, 2023

@nazar-pc maybe you know about this unexpected cargo clippy new error?

https://github.com/versatica/mediasoup/actions/runs/6121294807/job/16619834171?pr=1152

error[E0793]: reference to packed field is unaligned
    --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ntapi-0.3.7\src\ntexapi.rs:2783:52
     |
2783 |         *tick_count.QuadPart_mut() = read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad);
     |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
     = note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
     = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers)

error[E0793]: reference to packed field is unaligned
    --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ntapi-0.3.7\src\ntexapi.rs:2807:25
     |
2807 |         ((read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad)
     |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
     = note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
     = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers)

For more information about this error, try `rustc --explain E0793`.
error: could not compile `ntapi` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
Error: The process 'C:\Users\runneradmin\.cargo\bin\cargo.exe' failed with exit code 101

@nazar-pc
Copy link
Collaborator

nazar-pc commented Sep 8, 2023

Looks like it tries to prvent you from doing something bad, is that a new dependency? I don't see any Rust file changes in this PR 😕

@ibc
Copy link
Member Author

ibc commented Sep 8, 2023

Looks like it tries to prvent you from doing something bad, is that a new dependency? I don't see any Rust file changes in this PR 😕

This PR (which doesn't change absolutely anything in Rust side) has flatbuffers branch as target and AFAIS same error happens in flatbuffers branch as well: https://github.com/versatica/mediasoup/actions/runs/6111826437/job/16587913166?pr=1064
Not sure if this is due to some recent Rust change/addition of @jmillan in that branch.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Sep 8, 2023

Hm, didn't check where it comes from, but worth reporting upstream to ntapi crate

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just few considerations noted. 👍

size_t len) override;
size_t len,
uint32_t ppid,
std::vector<uint16_t>& subchannels,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be const, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you mean? Everything? len, ppid, subchannels and requiredSubchannel?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please not open the melon of making all C++ arguments const? We don't do it in 99% of cases and honestly I don't feel like if we are missing something useful.
Screenshot 2023-09-11 at 12 05 04

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant subchannels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why should we make it const and not all the other arguments in this method and tons of others?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But ignore if you want. We can approach the const correctness matter later on.

size_t len) = 0;
size_t len,
uint32_t ppid,
std::vector<uint16_t>& subchannels,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, const.

const uint8_t* msg,
size_t len,
uint32_t ppid,
std::vector<uint16_t>& subchannels,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same.

@@ -93,24 +93,34 @@ namespace RTC
{
MS_TRACE();

flatbuffers::Offset<FBS::SctpParameters::SctpStreamParameters> sctpStreamParametersOffset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree.

@@ -9,11 +9,11 @@ table Boolean {
value:uint8;
}

table Integer {
table Integer32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even Int32 could make it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, please. If so I should also rename Integer32Array to Int32Array. However Int32Array is a JS binary array class so conflicts.

@ibc
Copy link
Member Author

ibc commented Sep 11, 2023

Hm, didn't check where it comes from, but worth reporting upstream to ntapi crate

Honestly I have no idea what ntapi is so no idea about how to report it. I don't even know if this is a bug in ntapi or in our code (due to whatever new version of Rust or deps).

@ibc
Copy link
Member Author

ibc commented Sep 11, 2023

@nazar-pc maybe you know about this unexpected cargo clippy new error?

https://github.com/versatica/mediasoup/actions/runs/6121294807/job/16619834171?pr=1152

error[E0793]: reference to packed field is unaligned
    --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ntapi-0.3.7\src\ntexapi.rs:2783:52
     |
2783 |         *tick_count.QuadPart_mut() = read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad);
     |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
     = note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
     = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use 

So there is a related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1826485

It seems to be fixed in ntapi 0.4.0: https://bugzilla.mozilla.org/show_bug.cgi?id=1826485#c2

However I have no idea which ntapi version we are using (whatever ntapi is). Output of cargo tree doesn't show ntapi at all.

Then people say here that "For some reason (I don't know why) Cargo picked an older version of tokio that used (indirectly) an older version of ntapi. Force Cargo to use new tokio by setting its version to "1.28.0" (the newest version), and it should work.". We have tokio 1.17.0 (whatever tokio is) according to Cargo tree. But of course we don't specify tokio in any Cargo.toml so...

@ibc
Copy link
Member Author

ibc commented Sep 11, 2023

@nazar-pc maybe you know about this unexpected cargo clippy new error?
https://github.com/versatica/mediasoup/actions/runs/6121294807/job/16619834171?pr=1152

error[E0793]: reference to packed field is unaligned
    --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ntapi-0.3.7\src\ntexapi.rs:2783:52
     |
2783 |         *tick_count.QuadPart_mut() = read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad);
     |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
     = note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
     = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use 

So there is a related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1826485

It seems to be fixed in ntapi 0.4.0: https://bugzilla.mozilla.org/show_bug.cgi?id=1826485#c2

However I have no idea which ntapi version we are using (whatever ntapi is). Output of cargo tree doesn't show ntapi at all.

Then people say here that "For some reason (I don't know why) Cargo picked an older version of tokio that used (indirectly) an older version of ntapi. Force Cargo to use new tokio by setting its version to "1.28.0" (the newest version), and it should work.". We have tokio 1.17.0 (whatever tokio is) according to Cargo tree. But of course we don't specify tokio in any Cargo.toml so...

@nazar-pc can we do this in mediasoup/rust/Cargo.toml?:
Screenshot 2023-09-11 at 12 27 04

@ibc ibc merged commit 2798355 into flatbuffers Sep 11, 2023
22 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add subchannels in DataChannels (dataProducer.send() method)
3 participants