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

fix: performance on node #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ThaUnknown
Copy link
Contributor

@ThaUnknown ThaUnknown commented Mar 17, 2022

This PR fixes a performance issue when using Transform streams, which caused some video players to get stalled, this was because the transform would wait for the parser to finish parsing before it could continue, we don't care about that as we don't mutate data, just parse it

Additionally it fixes an issue where subtitlestream would stop parsing it metadata wasn't fully according to spec, for example some encoders would embed CR32 as the first tag in the cluster, not the timescale

Adds chapters

@mathiasvr
Copy link
Owner

How do you detect data loss? Do you get any exceptions? I'm not sure how to reproduce.
I'm seeing that the emit event can delay data but it's still the same chunks and in the same order as the transform method.

@ThaUnknown
Copy link
Contributor Author

ThaUnknown commented Mar 21, 2022

No errors get thrown, but 1/2 of the chunks pushed to the stream are ignored by the transform for some reason, tends to happen on chunks of constant sizes which come in very slowly.

The way I detected data loss is I summed the data being pushed and data being received and it was exactly half, however this might have been an issue caused by electron's node integration, rather than node itself, this definitely fixed it however, reproducing is a bitch however.

This happened with webtorrent's file.createReadStream which seems compliant with node's readable streams.

@mathiasvr
Copy link
Owner

The only thing I can think of is that there could be an issue with the way the previous instance is closed (when seeking to new offset), however since the transform method gets called before the data event it doesn't make sense that it could fix data loss?

It would be great if you could come up with some not too complicated reproduction to better understand what the issue is, but otherwise I will just do some testing later to check that the 'data' event works for other cases.

@ThaUnknown
Copy link
Contributor Author

@mathiasvr I've updated this PR

@ThaUnknown ThaUnknown force-pushed the master branch 3 times, most recently from 5fea2fc to b7d913c Compare December 27, 2022 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants