Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to:
#340
videojs/video.js#1913
video-dev#4
videojs/video.js#5252
(probably many more that I don't know about too)
Short Explanation of the Issue:
This method: https://github.com/mozilla/vtt.js/blob/master/lib/vtt.js#L1097 is fully synchronous, and relies on being synchronous throughout the entire logic. Tested against 15 subtitles of 1600-2000 lines, it took around 700-900ms in my tests to parse each subtitle.
More specifically these lines: https://github.com/mozilla/vtt.js/blob/master/lib/vtt.js#L1199-L1315 in which the iteration of the entire file is wrapped in a
try { } catch(e) { }
and also every iteration of subtitle time lines (CUE
state) is wrapped in anothertry { } catch(e) { }
, all this while iterating synchronously through what may possibly be very large subtitle files efficiently block the entire page for some time.Possible solutions:
setTimeout(function() {},0)
- wrapping the toptry { } catch(e) { }
in a setTimeout takes less then 5 seconds and would still improve this madness a lot, but that doesn't fix the inefficiency of usingtry { } catch(e) { }
so many timesES6 Promises
- oh, the perfect replacement fortry { } catch(e) { }
to simplify fixing all of this, butvtt.js
is a shim, and as such it needs increased compatibility with browsers, and adding a polyfill for promises is also out of the question as it would bloat the codeasync.js
- the wonders of this library.. people could argue for weeks on how to make this code prettier withasync
utilities, but it will bloat the hell out ofvtt.js
so it's not worth itweb workers
- i would so totally push this mess straight into a worker and forget about it, but sadly, subtitles are converted toVTTCue
s, which I imagine are following a spec, and each has it's ownget
/set
methods which would be lost on serialization from the worker, not to mention the inefficiency of serializing / de-serializing responses from the worker on a large filecallback hell
- well.. it's not gonna handle all possible errors that could arise like atry { } catch(e) { }
or promises would, but fuck it, I don't see any serious alternativeSo callback hell it is.. this dropped the loading time of
WebVTT.Parser.parse()
to 20-50ms from 700-900ms. I'm sure it can be improved a lot more, but it's a starting point.I tested this PR with the same 15 subtitles, they all went through and worked like a charm with VideoJS, I tried to test with
vtt.js
's tests too, but most of those fail anyway as described in: #343So I'm just leaving this here for y'all to babble about. Gonna go pour a good glass of wine 'cause my mind's spinning from the
try
,catch
,throw
,continue
,return
hurricane I just got out of.