-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactor data consumer SCTP stream parameter handling #1111
Refactor data consumer SCTP stream parameter handling #1111
Conversation
This required to bump the minimum NodeJS version to 17.0.0
The API of `DataConsumer` and `DataProducer` have been unified to use the same kind of parameters object when providing SCTP stream parameters and those parameters are always validated. Previously, creating a `DataConsumer` from a `DataProducer` that has been created via `DirectTransport` was brittle since SCTP stream parameters cannot be inferred from the `DirectTransport`'s `DataProducer`. It is now possible to provide the SCTP stream id for the consumer, too.
There are many concerns in this PR:
Regarding this:
If you create a BTW the reliability of |
No worries, I totally get it if a major API change is not acceptable for you at the current point in time. My primary concern was the ability to add the SCTP stream id. Aligning the options objects with the one for the producer was secondary. So, would you accept another PR instead that leaves the the current API as-is but adds the SCTP stream id as an optional parameter to the
Thanks for the clarification! Then I'm assuming I misinterpreted that a consumer can consume unreliably from a reliable producer (because technically that should be possible - not that I would have a use case for this).
Thanks for that insight. This made me curious. Can you elaborate where the "cannot handle" part could happen in the mediasoup code? |
Yes. But please don't do it in v3 branch but in flatbuffers branch to avoid conflicts when we merge its corresponding PR #1064 into v3 branch. Also, changes must also be done in Rust, and tests must be added in both.
If the network of the consuming device is poor and usrsctp lib cannot deliver all messages to it, then it will discard some of them and mediasoup will report the corresponding warning/error notification to the app. |
Sure, in that case I think I might wait until you have merged #1064.
Ah, is that the case where |
usrsctp lib decides whether it can send the message or not, number of re-attempts, etc based on the quality of the current transmission. We, in mediasoup code, have no control about that. Yes, doing data buffering in Node layer is an approach. We use it as well. |
The API of
DataConsumer
andDataProducer
have been unified to use the same kind of parameters object when providing SCTP stream parameters and those parameters are always validated.Previously, creating a
DataConsumer
from aDataProducer
that has been created viaDirectTransport
was brittle since SCTP stream parameters cannot be inferred from theDirectTransport
'sDataProducer
.It is now possible to provide the SCTP stream id for the consumer, too.
My primary motivation was to be able to set the SCTP stream id for the consumer and then I noticed that the current API is a bit brittle when used in combination with a
DirectTransport
, leaving me with an SCTPDataConsumer
whose parameters were stuck at default and didn't match the originalDataProducer
.I'm not sure how controversial this API change is but I'm certain you will tell me. 🙂
(This builds on #1110, so I'm marking it as a draft for now.)