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

Proposed solution for issue #656 - "Default flag not working on tracks" #1153

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Chris-DL
Contributor

Chris-DL commented Apr 16, 2014

Proposed solution for issue #656 - "Default flag not working on tracks"

Added additional check to look for auto preload option to avoid loading external captions, subtitles, etc. file on default

Thanks @Korotkiewicz for finding solution.

Proposed solution for issue #656 - "Default flag not working on track…
…s (subtitles, captions)"

Added additional check to look for auto preload option to avoid loading external captions, subtitles, etc. file on default
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 17, 2014

Member

Nice, thanks!

So the breaking tests is actually that we have another tests that sets auto play to true and a track with default true.
https://github.com/videojs/video.js/blob/master/test/unit/player.js#L83

The way I would actually fix that one is to change default in both places of that test to something like attrtest. All we're doing there is making sure values get passed through, so not using default specifically isn't a big issue.

The way this is being implemented isn't really ideal. It requires one of the control elements to handle the default track behavior. If controls were disabled or removed, this would stop working.

The preferred way to fix this would be to uncomment this line.
https://github.com/videojs/video.js/blob/v4.5.2/src/js/tracks.js#L61
And then fix the error that comes up when it's uncommented. The error happens because with the html5 tech, the player is 'ready' before it's child components (including the textTrackDisplay) have finished loading.

The dirty hack way to fix the issue would be to put a settimeout around the track.show call.

  if (track.dflt()) {
    this.ready(function(){
      setTimeout(function(){
        track.show();
      }, 0);
    });
  }

I think we could accept that solution for now, along with a comment pointing to this note. This issue has been outstanding for awhile.

Would you be interest in updating your PR to do that instead?

The long term way to fix this I think would be to make the textTrackDisplay an actual part of a tech, so that it's ready when the tech is. @mmcc and I have already talked about doing that, and relying more on the browser's captions support at the same time.

Another way to fix this might be to wait for all child elements to be loaded before triggering ready on the player. I think that could be done, and probably should be anyway.

Member

heff commented Apr 17, 2014

Nice, thanks!

So the breaking tests is actually that we have another tests that sets auto play to true and a track with default true.
https://github.com/videojs/video.js/blob/master/test/unit/player.js#L83

The way I would actually fix that one is to change default in both places of that test to something like attrtest. All we're doing there is making sure values get passed through, so not using default specifically isn't a big issue.

The way this is being implemented isn't really ideal. It requires one of the control elements to handle the default track behavior. If controls were disabled or removed, this would stop working.

The preferred way to fix this would be to uncomment this line.
https://github.com/videojs/video.js/blob/v4.5.2/src/js/tracks.js#L61
And then fix the error that comes up when it's uncommented. The error happens because with the html5 tech, the player is 'ready' before it's child components (including the textTrackDisplay) have finished loading.

The dirty hack way to fix the issue would be to put a settimeout around the track.show call.

  if (track.dflt()) {
    this.ready(function(){
      setTimeout(function(){
        track.show();
      }, 0);
    });
  }

I think we could accept that solution for now, along with a comment pointing to this note. This issue has been outstanding for awhile.

Would you be interest in updating your PR to do that instead?

The long term way to fix this I think would be to make the textTrackDisplay an actual part of a tech, so that it's ready when the tech is. @mmcc and I have already talked about doing that, and relying more on the browser's captions support at the same time.

Another way to fix this might be to wait for all child elements to be loaded before triggering ready on the player. I think that could be done, and probably should be anyway.

@heff heff added confirmed labels Apr 17, 2014

Chris-DL added some commits Apr 17, 2014

Revert "Proposed solution for issue #656 - "Default flag not working …
…on tracks (subtitles, captions)""

This reverts commit 8735935.
Uncommented existing default track logic in addTextTrack function to …
…allow default tracks to be shown appropriately.

Added setTimeout workaround so that exception is not thrown because of html5 tech issue.
Removed "default" attribute from player unit test so that track does not try to load extern during validation.
@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL Apr 17, 2014

Contributor

@heff Thanks for your quick response. I have updated my PR with your suggested changes.

I modified the player unit test to not specifically pass the "default" option but rather "attrtest" to make sure it was passed through.

I also reverted the original fix and then uncommented the existing default track code in the addTextTrack function (found here https://github.com/videojs/video.js/blob/v4.5.2/src/js/tracks.js#L61) as suggested. Essentially, adding the quick and dirty hack to get it all working.

For the long term, if you or @mmcc will point me in the right direction for implementing the textTrackDisplay as part of the tech then I'd be happy to give it a go.

Contributor

Chris-DL commented Apr 17, 2014

@heff Thanks for your quick response. I have updated my PR with your suggested changes.

I modified the player unit test to not specifically pass the "default" option but rather "attrtest" to make sure it was passed through.

I also reverted the original fix and then uncommented the existing default track code in the addTextTrack function (found here https://github.com/videojs/video.js/blob/v4.5.2/src/js/tracks.js#L61) as suggested. Essentially, adding the quick and dirty hack to get it all working.

For the long term, if you or @mmcc will point me in the right direction for implementing the textTrackDisplay as part of the tech then I'd be happy to give it a go.

@kutothe

This comment has been minimized.

Show comment
Hide comment
@kutothe

kutothe Apr 22, 2014

@Chris-DL would you happen to have a single video.js file with your change implemented so that I could test it? Or, could you point me in the right direction as to how I could create one myself? I'm sure there's a script that combines all the js files, but this is all new to me.

kutothe commented on 49e75db Apr 22, 2014

@Chris-DL would you happen to have a single video.js file with your change implemented so that I could test it? Or, could you point me in the right direction as to how I could create one myself? I'm sure there's a script that combines all the js files, but this is all new to me.

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL Apr 22, 2014

Owner

Hey @kutothe. I've created a github gist with the contents of the video.js file with the fix included. You should be able to copy and paste the contents to overwrite your video.js file. Let me know if you need help with anything or have any questions.

https://gist.github.com/Chris-DL/1e93360ae29b2c629b6a

Owner

Chris-DL replied Apr 22, 2014

Hey @kutothe. I've created a github gist with the contents of the video.js file with the fix included. You should be able to copy and paste the contents to overwrite your video.js file. Let me know if you need help with anything or have any questions.

https://gist.github.com/Chris-DL/1e93360ae29b2c629b6a

This comment has been minimized.

Show comment
Hide comment
@kutothe

kutothe Apr 22, 2014

That worked perfectly for me, and is very helpful. I hope it get's merged in soon. Thank you, and nice job!

kutothe replied Apr 22, 2014

That worked perfectly for me, and is very helpful. I hope it get's merged in soon. Thank you, and nice job!

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL Apr 23, 2014

Owner

@kutothe I'm glad that I could help.

Owner

Chris-DL replied Apr 23, 2014

@kutothe I'm glad that I could help.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 7, 2014

Member

This looks great, thanks!! Pulling this in.

Moving captions handling to the tech I think is going to be difficult, but you're welcome to take a stab at it. A first step might be to make it possible to determine if captions are supported by the current video element, and if so, allow the track info to be passed through.

It'd probably be easier to address the waiting for children to load issue. Right now we just trigger ready on the player whenever a new tech is loaded and ready, but we could add in a step to wait for children as well.
https://github.com/videojs/video.js/blob/v4.5.2/src/js/player.js#L268

Member

heff commented May 7, 2014

This looks great, thanks!! Pulling this in.

Moving captions handling to the tech I think is going to be difficult, but you're welcome to take a stab at it. A first step might be to make it possible to determine if captions are supported by the current video element, and if so, allow the track info to be passed through.

It'd probably be easier to address the waiting for children to load issue. Right now we just trigger ready on the player whenever a new tech is loaded and ready, but we could add in a step to wait for children as well.
https://github.com/videojs/video.js/blob/v4.5.2/src/js/player.js#L268

@heff heff closed this in 1df28ff May 7, 2014

@Chris-DL Chris-DL deleted the Chris-DL:feature/fix-default-subtitles branch May 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment