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.

Sign up for GitHub

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

Jump to bottom

Load text tracks on demand #6043

Merged
merged 1 commit into from
Nov 4, 2019
Merged

Load text tracks on demand #6043

merged 1 commit into from
Nov 4, 2019

Conversation

bvibber
Copy link
Contributor

@bvibber bvibber commented Jun 11, 2019

Description

Reimplementation of #2192
on current code. Seems to work but has not been carefully tested,
especially on conditions such as slow networks or default tracks.
Would appreciate testing and any advice on ways this may go
wrong and need additional work.

For #5252

This delays loading of the VTT cue files until they are selected.
For sites like Wikipedia that tend to have large numbers of
crowdsourced subtitles and can show many files together on one
page, this saves a lot of unnecessary network transfer and API
hits.

Specific Changes proposed

  • does not load text tracks immediately unless they are default or non-subtitles
  • does load them on demand

Requirements Checklist

  • Feature implemented / Bug fixed (seems to work!)
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE) (tested in Firefox)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Jun 11, 2019

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-TimedMediaHandler that referenced this pull request Jun 12, 2019
@bvibber @jdforrester
video.js: Local patch for subtitle loading behavior
c6390e0 
Upstream PR in progress, will add more adjustments to it
and later will be able to remove this patch here.
videojs/video.js#6043

Bug: T222763
Change-Id: I4d2eeb44dacdcd12c7717d208b21bed2775a8360
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this pull request Jun 12, 2019
@bvibber
Update git submodules
2d99e83 
* Update TimedMediaHandler from branch 'master'
  to c6390e0b29d05093f31d5b22b4840fe7fdae884a
  - video.js: Local patch for subtitle loading behavior
    
    Upstream PR in progress, will add more adjustments to it
    and later will be able to remove this patch here.
    videojs/video.js#6043
    
    Bug: T222763
    Change-Id: I4d2eeb44dacdcd12c7717d208b21bed2775a8360
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-TimedMediaHandler that referenced this pull request Jun 20, 2019
@bvibber @jdforrester
video.js: Local patch for subtitle loading behavior
a43b42a 
Upstream PR in progress, will add more adjustments to it
and later will be able to remove this patch here.
videojs/video.js#6043

Bug: T222763
Change-Id: I4d2eeb44dacdcd12c7717d208b21bed2775a8360
(cherry picked from commit c6390e0)
wmfgerrit pushed a commit to wikimedia/mediawiki that referenced this pull request Jun 20, 2019
@bvibber
Update git submodules
09c67b4 
* Update extensions/TimedMediaHandler from branch 'wmf/1.34.0-wmf.8'
  to a43b42a4e3438769ec53d679a4ad9b14cd310f13
  - video.js: Local patch for subtitle loading behavior
    
    Upstream PR in progress, will add more adjustments to it
    and later will be able to remove this patch here.
    videojs/video.js#6043
    
    Bug: T222763
    Change-Id: I4d2eeb44dacdcd12c7717d208b21bed2775a8360
    (cherry picked from commit c6390e0b29d05093f31d5b22b4840fe7fdae884a)
@gkatsev gkatsev added the minor This PR can be added to a minor release. It should not be added to a patch release. label Jul 29, 2019
@stale
Copy link

stale bot commented Sep 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the outdated Things closed automatically by stalebot label Sep 28, 2019
@bvibber
Copy link
Contributor Author

bvibber commented Oct 1, 2019

I'll fix this up shortly...

@stale stale bot removed the outdated Things closed automatically by stalebot label Oct 1, 2019
@bvibber bvibber force-pushed the subtitle-ondemand branch from 57a43a5 to 55367a6 Compare October 1, 2019 18:36
@bvibber
Copy link
Contributor Author

bvibber commented Oct 1, 2019

New version of this patch retains previous default behavior, adding a preloadTextTracks option on techs set to true by default. It can now be disabled, leading to on-demand loading for the non-default tracks.

Should pass tests, but I'm not 100% sure this is the right way to set up such a config option. Comments & recommendations welcome. :)

@bvibber
Copy link
Contributor Author

bvibber commented Oct 1, 2019

Not sure why the tests are failing, but there's a lot of 404s reported? Not sure if something's wrong with the test runner or if the code's broken in a way I don't understand.

@gkatsev
Copy link
Member

gkatsev commented Oct 1, 2019

Yeah, not sure what's up with the test. Restarted but I'll check it out tomorrow.

gkatsev
gkatsev reviewed Oct 7, 2019
View reviewed changes
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should the behavior be when we have default attribute set? It looks like it'll preload all the tracks if any have default enabled. Also, it looks like it'll always preload the descriptions track, it probably shouldn't be preloaded as well.

src/js/tech/tech.js Outdated Show resolved Hide resolved
src/js/tracks/text-track.js Outdated Show resolved Hide resolved
@bvibber
Copy link
Contributor Author

bvibber commented Nov 4, 2019

What should the behavior be when we have default attribute set? It looks like it'll preload all the tracks if any have default enabled. Also, it looks like it'll always preload the descriptions track, it probably shouldn't be preloaded as well.

It should only load the individual track labeled with default if I'm reading it correctly. The check for kinds was inherited from the previous patch I based this on, so might not be 100% correct. I think the idea is that metadata tracks should be passed through and loaded? Can adjust that bit if necessary.

@bvibber
Option to load text tracks on demand vs preload
9ab12f8 
Reimplementation of videojs#2192
on current code. Seems to work but has not been carefully tested,
especially on conditions such as slow networks and complex tracks.

For videojs#5252

A `preloadTextTracks` tech option is added, set to true by default,
to keep current behavior intact. Alternate behavior can be enabled
by setting this to false.

This delays loading of the VTT cue files until they are selected.
For sites like Wikipedia that tend to have large numbers of
crowdsourced subtitles and can show many files together on one
page, this saves a lot of unnecessary network transfer and API
hits.

Does mean there may be dropped cues while switching to a track
that requires on-demand loading.

Example usage:

videojs(element, {
  html5: {
    preloadTextTracks: false
  }
};
@gkatsev gkatsev merged commit 0e37fbf into videojs:master Nov 4, 2019
@welcome
Copy link

welcome bot commented Nov 4, 2019

Congrats on merging your first pull request! 🎉🎉🎉

This was referenced Mar 19, 2020
Closed
Closed
@isabelleingato isabelleingato mentioned this pull request Dec 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkatsev gkatsev gkatsev approved these changes

@misteroneill misteroneill misteroneill approved these changes

Assignees
No one assigned
Labels
minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bvibber @gkatsev @misteroneill