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

fix: let the tech preload auto on its own #4861

Merged
merged 3 commits into from
Apr 2, 2018
Merged

Conversation

brandonocasey
Copy link
Contributor

Description

This was found due to the work done in #4660. Basically we reload the video element twice on every source with preload set to auto. This can potentially cause the same data to be downloaded twice.

Specific Changes proposed

  • remove players call to load, as tech should handle preload on its own, and this is harmful for the html5 tech.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@brandonocasey
Copy link
Contributor Author

brandonocasey commented Mar 16, 2018

This currently fires two sourcesets:

QUnit.skip('player.src({...}) preload auto', function(assert) {
const done = assert.async();
this.mediaEl.setAttribute('preload', 'auto');
this.player = videojs(this.mediaEl);
this.player.one('sourceset', () => {
validateSource(assert, this.player, [this.testSrc]);
done();
});
this.player.src(this.testSrc);
});

@brandonocasey
Copy link
Contributor Author

Since sourceset is now in master I made the skipped tests run again.

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.

I'm still a bit concerned about potential side-effects of this, though, since sourceset is feature-flagged currently, we can limit its affect.

@@ -2471,11 +2471,7 @@ class Player extends Component {
this.techCall_('src', source.src);
}

if (this.options_.preload === 'auto') {
Copy link
Member

Choose a reason for hiding this comment

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

what about skipping this is enableSourceset is set?

src/js/player.js Outdated
}

// Set the source synchronously if possible (#2326)
// Set the source synchronously if possible (#2326)
Copy link
Member

Choose a reason for hiding this comment

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

this indent is interesting. I think it was dedented so that it lines up with the this.ready call, not sure if it's necessary or not. Maybe we want to make a new line for the arg and have the comment directly above the arg?

@brandonocasey
Copy link
Contributor Author

brandonocasey commented Mar 23, 2018

I think we may be better off testing if this causes breakage with some QA, but if we just want to do it later we could put it behind the enableSourceset flag.

@pagarwal123
Copy link

QA-LGTM

@gkatsev gkatsev merged commit fdcae1b into master Apr 2, 2018
@gkatsev gkatsev deleted the fix/let-tech-preload branch April 2, 2018 18:30
brandonocasey added a commit that referenced this pull request Apr 2, 2018
This was found due to the work done in #4660. Basically we reload the video element twice on every source with preload set to auto. This can potentially cause the same data to be downloaded twice.
brandonocasey added a commit that referenced this pull request Apr 3, 2018
This was found due to the work done in #4660. Basically we reload the video element twice on every source with preload set to auto. This can potentially cause the same data to be downloaded twice.
@gkatsev gkatsev mentioned this pull request Apr 23, 2018
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.

4 participants