Make Parser Async - Githubissues

jaruba commented 6 years ago

(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 another try { } 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 top try { } 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 using try { } catch(e) { } so many times
ES6 Promises - oh, the perfect replacement for try { } catch(e) { } to simplify fixing all of this, but vtt.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 code
async.js - the wonders of this library.. people could argue for weeks on how to make this code prettier with async utilities, but it will bloat the hell out of vtt.js so it's not worth it
web workers - i would so totally push this mess straight into a worker and forget about it, but sadly, subtitles are converted to VTTCues, which I imagine are following a spec, and each has it's own get / 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 file
callback hell - well.. it's not gonna handle all possible errors that could arise like a try { } catch(e) { } or promises would, but fuck it, I don't see any serious alternative

So 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: https://github.com/mozilla/vtt.js/issues/343

So 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.

silviapfeiffer commented 6 years ago

@rillian who's responsible for captions at Mozilla these days...?

gkatsev commented 6 years ago

It's also on my todo list to review. Though, we'd need a PR against our fork https://github.com/videojs/vtt.js/

also, as part of video-dev, we're going to try and maintain vtt.js unless mozilla steps up: https://github.com/video-dev/vtt.js

gkatsev commented 4 years ago

I just tried porting this over to videojs/vtt.js but ran into a bunch of issues. The main issue I found is that if I have multiple captions that get parsed, if one of them gets a parser error, none of the other captions end up working.

jaruba commented 4 years ago

@gkatsev This was from more then one year ago.. I've completely fixed this since for my needs. Including this issue: https://github.com/videojs/video.js/issues/5252

But due to the fact that this PR was ignored, and all fixes started from this one terribly inefficient module, I never got to pushing my commits to the rest of the repos so they're now lost to time in an old and highly customised version of videojs that I'm using in a hobby project. (where it works perfectly and has been tested against thousands of subtitles since)

I remember VideoJS itself required quite a few changes to get this working properly, it was not a drop-in change. Redoing all of it might require significant effort on my side, and honestly, I'm not motivated to go down that rabbit's hole again.

Disclaimer: I've worked professionally on (and created) many different video players through the last few years, so I know exactly how they should work and am comfortable with the logic involved.

gkatsev commented 4 years ago

That's totally understandable. I just spent some time refactoring the video.js fork and I'll probably take a look at making changes to make it asynchronous as well. If you do find the commits, they would be super helpful. Appreciate you trying to get this started.

JohannesKuehnel commented 3 years ago

@RickEyre @humphd @rillian Is anyone of you still with Mozilla and able to tell if this project has been completely abandoned or still has a maintainer?

gkatsev commented 3 years ago

I don't think anyone at Mozilla has the bandwidth to maintain this. For Video.js, we maintain our own fork. Our fork is almost ready to ship WebVTT regions!

JohannesKuehnel commented 3 years ago

I don't think anyone at Mozilla has the bandwidth to maintain this. For Video.js, we maintain our own fork. Our fork is almost ready to ship WebVTT regions!

In a different ticket you mentioned you are also low on resources to get the async parsing going, so I thought I would try reaching someone upstream. 😸

gkatsev commented 3 years ago

Yup, they have even less for this 😁

mozilla / vtt.js

Make Parser Async #373