Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

loadstart event should always come before loadedmetadata event #310

Closed
vasklund opened this issue Jun 16, 2015 · 5 comments
Closed

loadstart event should always come before loadedmetadata event #310

vasklund opened this issue Jun 16, 2015 · 5 comments
Labels

Comments

@vasklund
Copy link

The loadstart event does not always happen before the loadedmetadata event, which is the expected behavior. This causes problems for example when trying to override the playlist selection, as discussed in #277. You can see the problem in action at http://output.jsbin.com/haheco/1/ (I can reproduce the problem around 50% of the time).

Today loadstart is triggered in Flash (HTTPVideoProvider:initNetConnection), and loadedmetadata is triggered in both HLS JS (playlists.on('loadedmetadata') handler) and Flash (HTTPVideoProvider:onMetaData). My understanding is that we want to trigger loadstart right before we start downloading the playlists at videojs-hls.js#L108, but that would mean moving that logic from Flash to JS. It would make sense to only trigger loadedmetadata from JS too, as the event triggers twice today.

I'm not sure what other implications/issues apart from #277 there are, but nonetheless this seems like it could cause more problems down the road. I'm not sure how this problem should be approached (it seems code changes are needed both in Flash and contrib HLS), but I wanted to highlight the issue and also ask for feedback in case I'm misunderstanding something.

@vasklund
Copy link
Author

PR #311 shows a naive approach to solving the issue described above - it's most certainly not a good solution.

@dmlap dmlap added the bug label Jun 17, 2015
@dcrockwell
Copy link

+1 this is blocking #277, which in turn is blocking our team.

@vasklund
Copy link
Author

@dcrockwell: There's a very crude monkey-patch which works as a short-term workaround if you really need it. Notice line 63. Maybe there are better workarounds, but this is what we are forced to used at the moment.

@dcrockwell
Copy link

@vasklund excellent! Thank you very much.

@dmlap
Copy link
Member

dmlap commented Aug 11, 2015

Fixed in #356

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants