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

Add optional sctpStreamId to SCTP-based data consumers #1378

Draft
wants to merge 2 commits into
base: v3
Choose a base branch
from

Conversation

threema-lenny
Copy link

@threema-lenny threema-lenny commented Apr 19, 2024

Second try (after #1111). This makes it possible to use a specific SCTP stream ID / data channel ID for forwarding data.

Could you give me feedback on whether this looks good for the Node API? Then I'll see if I can add the change to the Rust API, too.

TODO

  • Rust side and test.
  • Document new sctpStreamId in DataConsumerOptions in the website. This task is for mediasoup authors once this PR is merged.

@threema-lenny threema-lenny marked this pull request as draft April 19, 2024 15:40
Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

What is the real purpose of this PR?

this.#sctpStreamIds![sctpStreamId] = 1;
sctpStreamParameters.streamId = sctpStreamId;
sctpStreamParameters.streamId = this.getNextSctpStreamId(sctpStreamId);
ortc.validateSctpStreamParameters(sctpStreamParameters);
Copy link
Member

Choose a reason for hiding this comment

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

Why this call to ortc.validateSctpStreamParameters()?

Copy link
Author

@threema-lenny threema-lenny Apr 25, 2024

Choose a reason for hiding this comment

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

It seemed like an oversight as this call is also present for validating the SCTP stream parameters for the producer (https://github.com/versatica/mediasoup/pull/1378/files#diff-3253c176609c972f1279e2a6c6ca4c52cbfac8423efcc8b2891b9d8dbd92c358R1016).

Copy link
Member

Choose a reason for hiding this comment

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

Please remove it. There is no need for this. These sctpStreamParameters were already validated in transport.produceData() and they are literally a copy of them.

Copy link
Author

@threema-lenny threema-lenny Apr 29, 2024

Choose a reason for hiding this comment

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

You're right, only streamId is now user input but its value (besides type) is not validated in validateSctpStreamParameters anyways. I'll remove it.

@jmillan
Copy link
Member

jmillan commented Apr 25, 2024

@threema-lenny, how are you taking benefit of choosing the steam ID for a DataConsumer? Can you share a use case?

@threema-lenny
Copy link
Author

threema-lenny commented Apr 25, 2024

@threema-lenny, how are you taking benefit of choosing the steam ID for a DataConsumer? Can you share a use case?

Sure.

We're using OOB negotiated data channel IDs (using pc.createDataChannel('foo', {negotiated: true, id: 0})). This simplifies client code by making it possible to have a single bidirectional RTCDataChannel at creation of the peer connection without having to listen to an ondatachannel event and having to map data channels there (although I'm not even sure if the mediasoup SFU implements the data channel establishment protocol, haven't checked) nor is it necessary to signal it in another way because the ID is already known beforehand.

In our case, data is relayed via the SFU using this fixed SCTP stream ID which is why we set the corresponding SCTP stream ID for both producer and consumer.

@ibc ibc marked this pull request as ready for review April 28, 2024 11:24
@ibc ibc marked this pull request as draft April 28, 2024 11:25
@ibc
Copy link
Member

ibc commented Apr 28, 2024

I get the point of this feature but need some time to review it.

Comment on lines +111 to +114
await ctx.webRtcTransport2!.consumeData({
dataProducerId: ctx.dataProducer!.id,
sctpStreamId: 123,
});
Copy link
Member

Choose a reason for hiding this comment

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

Please, create const dataConsumer here and assert that its sctpStreamParameters.streamId matches the given sctpStreamId.

Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Other than the requested changes I think this PR goes in the correct direction, so please, make these minimal changes and write the Rust side as well.

@ibc
Copy link
Member

ibc commented Jun 28, 2024

@threema-lenny are you working on this PR?

@threema-lenny
Copy link
Author

@ibc Not right now but I'll definitely need to come back to getting this done.

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.

3 participants