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

Make Parser Async #373

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Make Parser Async #373

wants to merge 4 commits into from

Conversation

jaruba
Copy link

@jaruba jaruba commented Jun 17, 2018

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 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: #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
Copy link

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

This was specific to VideoJS's `vtt.js`
@gkatsev
Copy link
Contributor

gkatsev commented Jun 22, 2018

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
Copy link
Contributor

gkatsev commented Nov 26, 2019

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
Copy link
Author

jaruba commented Nov 28, 2019

@gkatsev This was from more then one year ago.. I've completely fixed this since for my needs. Including this issue: videojs/video.js#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
Copy link
Contributor

gkatsev commented Nov 28, 2019

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
Copy link

@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
Copy link
Contributor

gkatsev commented Jul 29, 2021

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
Copy link

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
Copy link
Contributor

gkatsev commented Jul 29, 2021

Yup, they have even less for this 😁

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.

None yet

4 participants