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

Suppress Infinity duration on Android Chrome before playback #3476

Merged
merged 8 commits into from
Nov 3, 2016

Conversation

mister-ben
Copy link
Contributor

@mister-ben mister-ben commented Jul 29, 2016

Description

Re issue #3079 and the discussion in #3083. Android Chrome returns Infinity for VOD HLS until after playback has started. This causes issues such as an unwanted flash of the live display control.

Specific Changes proposed

HTML5 tech will return NaN instead of Infinity if playback has not started. Fires a durationupdate event once the reported duration can be believed if the duration is still Infinity, so controls can update.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Unit test updated
  • Reviewed by Two Core Contributors

}
};
this.on(this.player_, 'timeupdate', checkProgress);
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

I believe duration should be NaN when the true value is unknown.

@dmlap
Copy link
Member

dmlap commented Jul 29, 2016

Would you mind adding a unit test for this?

if (this.el_.duration === Infinity &&
browser.IS_ANDROID && browser.IS_CHROME) {
if (this.el_.currentTime === 0) {
let checkProgress = () => {
Copy link
Member

Choose a reason for hiding this comment

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

The newly released version of videojs-standard will complain about the use of let for names that are never reassigned (const is preferred).

@mister-ben
Copy link
Contributor Author

@misteroneill re the question I see on email notifications but not showing up here on why I've used on()/off() rather than one(), I get a few timeupdate events before currentTime and duration update.

return NaN;
}
}
return this.el_.duration || NaN;
Copy link
Member

Choose a reason for hiding this comment

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

do the time components handle NaN correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They behave correctly and formatTime deals with it and Infinity.

@gkatsev
Copy link
Member

gkatsev commented Aug 8, 2016

LGTM.

Needs a rebase against master. Would you be able to do it? Otherwise, I can take care of it as part of the merge.

@gkatsev gkatsev added confirmed minor This PR can be added to a minor release. It should not be added to a patch release. labels Aug 8, 2016
@gkatsev gkatsev added this to the 5.13 milestone Sep 27, 2016
@gkatsev
Copy link
Member

gkatsev commented Oct 3, 2016

This needs to be rebased against master.

@incompl
Copy link

incompl commented Oct 7, 2016

@gkatsev Looks like this branch has no conflicts with base branch, what needs to happen next?

@vdeshpande
Copy link
Contributor

@gkatsev , @mister-ben What needs to happen next here ?

@mister-ben
Copy link
Contributor Author

It's ready to go and will be pulled into the 5.13 release.

@incompl
Copy link

incompl commented Oct 26, 2016

Awesome

@gkatsev gkatsev merged commit ed59531 into videojs:master Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants