Skip to content

Fixed some bugs#2

Open
olee wants to merge 5 commits intodirkjanm:masterfrom
247GradLabs:master
Open

Fixed some bugs#2
olee wants to merge 5 commits intodirkjanm:masterfrom
247GradLabs:master

Conversation

@olee
Copy link

@olee olee commented Oct 6, 2016

  • Fixed loading issue where player was not ready.
  • Fixed loading of thumbnails to be relative to the video file.

olee added 2 commits October 6, 2016 10:09
Fixed loading of thumbnails to be relative to the video file.
@olee
Copy link
Author

olee commented Oct 13, 2016

@dirkjanm did you get a chance to take a look at the changes?

@dirkjanm
Copy link
Owner

Thanks for the pull request! The changes look good, just the one with the relative thumbnails is a bit odd, since I would normally expect an URL starting with a / to be relative to the domain instead of the video source. Adding this as default behaviour can be confusing for people. Would it maybe be better to add an option instead called for example relativeUrls, which will cause URLs to be loaded relative to the video, and if this option is not set (default) to leave the URL parsing up to the browser?

@olee
Copy link
Author

olee commented Oct 16, 2016

If you read this article it will actually tell you, that urls in vtt files should always be relative to the video file. That's why I used a kind of solution that allows both.

But actually it should always be relative except if the url is absolute.
For me the current solution is enough and I think it's at least better than the state of it before, but maybe you could try adding a correct path resolution after merging those first fixes (checking for absolute http://... paths).

@dirkjanm
Copy link
Owner

Actually the article you are linking to says that the URLs are relative to the VTT file, not to the video file. And an URL starting with an / would point to the root of the domain and not to the current folder in the case of JW player.

Note that there is already a setting that allows you to set a basepath which is used as a prefix for all thumbnails, wouldn't that suit your needs already?

@olee
Copy link
Author

olee commented Oct 16, 2016

Oh yeah it was relative to the vtt file not the video file.
Then the code should be changed to reflect that.
I will try to fix the PR the next days.

A basepath setting sounds good, but it would still not meet the specs which say it should always be relative (except if absolute url)

@richardbushell
Copy link

@olee @dirkjanm
Did either of you have videojs-vtt-thumbnails working with videojs 5?
We have videojs 5.16.0 but we always get an exception stating:
Uncaught TypeError: Cannot read property 'progressControl' of undefined
progressControl = player.controlBar.progressControl;
Please advise...

@olee
Copy link
Author

olee commented Oct 19, 2017

I just fixed our branch at https://github.com/247GradLabs/videojs-vtt-thumbnails to work with videojs 5

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.

3 participants