Improved data tracks depacketizer to support mutliple in flight packets#1923
Improved data tracks depacketizer to support mutliple in flight packets#1923
Conversation
…Start" packets being stored (fixes the last failing test)
- Use map insertion order instead of a timestamp for sorting - Add comments
Also rename some fields which came up during writing the tests.
…tial frame map auto resize itself
…es into the Depacketizer from the RemoteDataTrack
|
size-limit report 📦
|
| * The value applies to all current and future subscribers of this track. May be called | ||
| * before or after {@link RemoteDataTrack.subscribe}; live subscriptions pick up the new value | ||
| * immediately. Defaults to 1. */ | ||
| setMaxPartialFrames(n: number): void { |
There was a problem hiding this comment.
question: What are your thoughts on adding this to DataTrackSubscribeOptions instead? This would still have the caveat that "the value applies to all current and future subscribers of this track," but this is the case for bufferSize anyway. Also keeps API surface cleaner.
There was a problem hiding this comment.
I had brought up in our 1:1 that this needs to be configured at the track level, and not the subscription level, since there is one IncomingDataTrackPipeline per RemoteDataTrack. Given this, I'm curious what you have in mind here - are you suggesting this be changed so there's aIncomingDataTrackPipeline per subscription instead?
I will say that just generally, I am not really a big fan of this method approach, and would like to find a better place to put this, so if there's something I'm missing here I'm definitely open to changing this.
There was a problem hiding this comment.
I'm proposing putting it on DataTrackSubscribeOptions but this updates the track-level value under-the-hood. One disadvantage I can think of is there is no way to change max partial frames once you have a subscription (without obtaining a second one)—but not sure if this is something you would want to do anyway.
There was a problem hiding this comment.
I'm confused - if it was done this way, wouldn't that mean that if a user did .subscribe({ maxPartialFrames: 5 }) first, then .subscribe({ maxPartialFrames: 10 }) second, the last value would "win" and apply to both subscriptions since there is one pipeline for all subscriptions of a given track? The only way to work around this would be one pipeline per subscription rather than one pipeline per track?
There was a problem hiding this comment.
Yes, the latest value would win. This behavior would have to be documented at the option level similar to this disclaimer in Rust for buffer size.
| ); | ||
| }); | ||
|
|
||
| it('should assemble multiple partial frames concurrently when maxPartialFrames is set', () => { |
ladvoc
left a comment
There was a problem hiding this comment.
Just the one question about exposing the new option, but looks good otherwise! I will port this to Rust so these can be merged at the same time.
maxPartialFrameswhich controls how many frames are being stored at once.RemoteDataTracklevel setting - there is one pipeline per track, not one pipeline per subscription, so this is where it makes most sense (also given that you won't really need to configure it differently by subscription since this is fully dependant on the data being received from the sender).Code example:
Todo