Data streams v2#1985
Conversation
🦋 Changeset detectedLatest commit: c3fd1ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
| /** The data-streams-v2 wire signals carried directly on the header: the compression flag and the | ||
| * inline single-packet payload. Both used to live in reserved header attributes; they are now | ||
| * first-class protobuf fields on `DataStream.Header`. */ | ||
| export interface StreamHeaderV2Fields { | ||
| /** Compression applied to the inline/chunked payload. Defaults to `NONE` when omitted. */ | ||
| compression?: DataStream_CompressionType; | ||
| /** The full payload smuggled into the header for single-packet (inline) sends. */ | ||
| inlineContent?: Uint8Array; | ||
| } |
There was a problem hiding this comment.
Note to self: get rid of this and inline these two fields directly into the relevant function calls. Or name this something more generic if I decide to keep it.
| // set text part of progress to 1 | ||
| handleProgress(1, 0); |
There was a problem hiding this comment.
Note to self: add some tests for the onProgress stuff. I'm not sure I've fully exercised it to make sure I didn't break anything here. Though worth noting I did find some bugs where in some paths onProgress wasn't being called before so I'm actually not convinced it was working properly before either.
There was a problem hiding this comment.
Fixed this (sort of) in 4a6e2bb. It definitely works better than how it did before but I think there's still some work that could be done to improve it. Right now it measures bytes before the compression stage, which means that if the compressor were to take a long time it could potentially skew the progress.
| // FIXME: make this a global event to ensure "max listener" warning won't get logged for lots of | ||
| // in flight data streams. | ||
| engine.once(EngineEvent.Closing, onEngineClose); |
There was a problem hiding this comment.
Note to self: fix this pre-existing bug as part of this larger change, I think right now there will me a "max listener" warning if you try to send too many data streams concurrently.
There was a problem hiding this comment.
alternatively if the pattern is expected we can also bump the maxListener count.
There was a problem hiding this comment.
I'm not a fan of just bumping the maxListener count, since you're just increasing the high water mark threshold. So IMO it either should be disabled (I forget if that's possible in the node EventEmitter implementation) or will address using a similar pattern as I did in waitForBufferStatusLow here.
There was a problem hiding this comment.
yeah, the latter option sounds nice!
size-limit report 📦
|
lukasIO
left a comment
There was a problem hiding this comment.
this is looking great already.
Really like that you consolidated a bunch of spread out things into helpers. Makes it a lot nicer to reason about.
| // FIXME: make this a global event to ensure "max listener" warning won't get logged for lots of | ||
| // in flight data streams. | ||
| engine.once(EngineEvent.Closing, onEngineClose); |
There was a problem hiding this comment.
alternatively if the pattern is expected we can also bump the maxListener count.
lukasIO
left a comment
There was a problem hiding this comment.
thanks for addressing all my comments!
|
Nice work! |
Using sendText means that future optimizations to send a single data stream packet can be enabled.
…t / split into 15k MTU Doing this makes data arrive a bit faster since it's not waiting for compressed bytes to accumulate before sending out a "fully filled" packet
…am materialization
…ial stream materialization" This reverts commit 25af9bc.
…ntexts It's still not perfect - the progress events are reported pre-compressor right now so the compressor lag isn't taken into account directly (other than the side effect of it on future data "pulls" through the stream). But I think it's an improvement over the previous state of just 0 -> 1.
This is set fairly high and will prevent any malicious decompression bomb style payloads
lukasIO
left a comment
There was a problem hiding this comment.
great work!
just some questions around the NonSharedUint8Array usage, there are some internal Uint8Arrays declared directly in some places
…8Array in all data stream utility functions

Warning
This change relies on some new SFU updates to be able to function. Checkout
main(really, livekit/livekit@86a79f8 or later), build + run the SFU locally, and point any test app at this local SFU for testing.Overview
Data streams v2 consists of some updates to data streams to add three things:
This pull request contains the initial web implementation and initial data streams spec which will be used to propagate this across all other client sdks.
Performance
Both 1 and 2 together at least doubles⚠️ data stream throughput across the spectrum of network conenctions and payload sizes.
Before:

After:

TODO