Skip to content

Commit

Permalink
fix: Suppress Infinity duration on Android Chrome before playback (#3476
Browse files Browse the repository at this point in the history
)

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.

Fixes #3079.
  • Loading branch information
mister-ben authored and gkatsev committed Nov 3, 2016
1 parent 2988f6a commit ed59531
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 1 deletion.
25 changes: 24 additions & 1 deletion src/js/tech/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,30 @@ class Html5 extends Tech {
* @return {Number}
*/
duration() {
return this.el_.duration || 0;
// Android Chrome will report duration as Infinity for VOD HLS until after
// playback has started, which triggers the live display erroneously.
// Return NaN if playback has not started and trigger a durationupdate once
// the duration can be reliably known.
if (this.el_.duration === Infinity &&
browser.IS_ANDROID && browser.IS_CHROME) {
if (this.el_.currentTime === 0) {
// Wait for the first `timeupdate` with currentTime > 0 - there may be
// several with 0
const checkProgress = () => {
if (this.el_.currentTime > 0) {
// Trigger durationchange for genuinely live video
if (this.el_.duration === Infinity) {
this.trigger('durationchange');
}
this.off(this.player_, 'timeupdate', checkProgress);

This comment has been minimized.

Copy link
@boushley

boushley Nov 17, 2016

Contributor

The this.player_ reference here is likely misleading.

This is equivalent to this.off('timeupdate', checkProgress) since this.player_ will always be this for a Tech. Based on slack conversations we've had the lack of a reference to the player is on purpose inside of Techs. So the confusing this.player_ should be removed from this call.

This comment has been minimized.

Copy link
@boushley

boushley Nov 17, 2016

Contributor

I opened a PR to resolve these: #3790

}
};

this.on(this.player_, 'timeupdate', checkProgress);

This comment has been minimized.

Copy link
@boushley

boushley Nov 17, 2016

Contributor

Same as my comment on the this.off call above. We should remove this.player_ since it is just this inside of a Tech.

return NaN;
}
}
return this.el_.duration || NaN;
}

/**
Expand Down
19 changes: 19 additions & 0 deletions test/unit/tech/html5.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -562,3 +562,22 @@ QUnit.test('Exception in play promise should be caught', function() {

tech.el_ = oldEl;
});

test('When Android Chrome reports Infinity duration with currentTime 0, return NaN', function() {
const oldIsAndroid = browser.IS_ANDROID;
const oldIsChrome = browser.IS_CHROME;
const oldEl = tech.el_;

browser.IS_ANDROID = true;
browser.IS_CHROME = true;

tech.el_ = {
duration: Infinity,
currentTime: 0
};
ok(Number.isNaN(tech.duration()), 'returned NaN with currentTime 0');

browser.IS_ANDROID = oldIsAndroid;
browser.IS_CHROME = oldIsChrome;
tech.el_ = oldEl;
});

0 comments on commit ed59531

Please sign in to comment.