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

Tech 2.0 additional fixes #2166

Closed
wants to merge 11 commits into from
34 changes: 8 additions & 26 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as browser from './utils/browser.js';
import log from './utils/log.js';
import toTitleCase from './utils/to-title-case.js';
import { createTimeRange } from './utils/time-ranges.js';
import { bufferedPercent } from './utils/buffer.js';
import FullscreenApi from './fullscreen-api.js';
import MediaError from './media-error.js';
import Options from './options.js';
Expand Down Expand Up @@ -415,7 +416,12 @@ class Player extends Component {
var techOptions = assign({
'source': source,
'playerId': this.id(),
'textTracks': this.textTracks_
'techId': `${this.id()}_${techName}_api`,
'textTracks': this.textTracks_,
'autoplay': this.options_.autoplay,
'preload': this.options_.preload,
'loop': this.options_.loop,
'muted': this.options_.muted
Copy link
Member

Choose a reason for hiding this comment

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

Very nice. For #2033 I want to try to move away from using options like this and instead use the player properties (e.g. this.autoplay()), but there's still more work to do to make that even possible. Not something for this PR but just wanted to call it out as something that may change in the near future.

}, this.options_[techName.toLowerCase()]);

if (this.tag) {
Expand Down Expand Up @@ -510,9 +516,6 @@ class Player extends Component {
this.on(this.tech, 'touchmove', this.handleTechTouchMove);
this.on(this.tech, 'touchend', this.handleTechTouchEnd);

// Turn on component tap events
this.tech.emitTapEvents();

// The tap listener needs to come after the touchend listener because the tap
// listener cancels out any reportedUserActivity when setting userActive(false)
this.on(this.tech, 'tap', this.handleTechTap);
Expand Down Expand Up @@ -1148,28 +1151,7 @@ class Player extends Component {
* @return {Number} A decimal between 0 and 1 representing the percent
*/
bufferedPercent() {
var duration = this.duration(),
buffered = this.buffered(),
bufferedDuration = 0,
start, end;

if (!duration) {
return 0;
}

for (var i=0; i<buffered.length; i++){
start = buffered.start(i);
end = buffered.end(i);

// buffered end can be bigger than duration by a very small fraction
if (end > duration) {
end = duration;
}

bufferedDuration += end - start;
}

return bufferedDuration / duration;
return bufferedPercent(this.buffered(), this.duration());
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/js/tech/flash.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class Flash extends Tech {
let { source, parentEl } = options;

// Generate ID for swf object
let objId = options.playerId+'_flash_api';
let objId = options.techId;

// Merge default flashvars with ones passed in to init
let flashVars = assign({
Expand Down
2 changes: 1 addition & 1 deletion src/js/tech/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class Html5 extends Tech {

Dom.setElementAttributes(el,
assign(attributes, {
id: this.options_.playerId + '_html5_api',
id: this.options_.techId,
class: 'vjs-tech'
})
);
Expand Down
60 changes: 13 additions & 47 deletions src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import TextTrack from '../tracks/text-track';
import TextTrackList from '../tracks/text-track-list';
import * as Fn from '../utils/fn.js';
import log from '../utils/log.js';
import { createTimeRange } from '../utils/time-ranges.js';
import { bufferedPercent } from '../utils/buffer.js';
import window from 'global/window';
import document from 'global/document';

Expand All @@ -21,7 +21,6 @@ import document from 'global/document';
class Tech extends Component {

constructor(options={}, ready=function(){}){
options = options || {};
// we don't want the tech to report user activity automatically.
// This is done manually in addControlsListeners
options.reportTouchActivity = false;
Expand Down Expand Up @@ -50,6 +49,9 @@ class Tech extends Component {
}

this.initTextTrackListeners();

// Turn on component tap events
this.emitTapEvents();
}

/**
Expand Down Expand Up @@ -109,15 +111,15 @@ class Tech extends Component {
this.progressInterval = this.setInterval(Fn.bind(this, function(){
// Don't trigger unless buffered amount is greater than last time

let bufferedPercent = this.bufferedPercent();
let numBufferedPercent = this.bufferedPercent();

if (this.bufferedPercent_ !== bufferedPercent) {
if (this.bufferedPercent_ !== numBufferedPercent) {
this.trigger('progress');
}

this.bufferedPercent_ = bufferedPercent;
this.bufferedPercent_ = numBufferedPercent;

if (bufferedPercent === 1) {
if (numBufferedPercent === 1) {
this.stopTrackingProgress();
}
}), 500);
Expand All @@ -127,33 +129,12 @@ class Tech extends Component {
this.duration_ = this.duration();
}

bufferedPercent() {
let bufferedDuration = 0,
start, end;

if (!this.duration_) {
return 0;
}

let buffered = this.buffered();

if (!buffered || !buffered.length) {
buffered = createTimeRange(0,0);
}

for (var i=0; i<buffered.length; i++){
start = buffered.start(i);
end = buffered.end(i);

// buffered end can be bigger than duration by a very small fraction
if (end > this.duration_) {
end = this.duration_;
}

bufferedDuration += end - start;
}
buffered() {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

should this return null? Or should it just return a 0, 0 time range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is never called outside the unit tests. null to me seems more appropriate because their is no buffer at all.

Copy link
Member

Choose a reason for hiding this comment

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

If this isn't used outside of tests do we need to define it here? Should we change something about the test or whatever relies on buffered?

Otherwise I think we should follow the spec.

The buffered attribute must return a new static normalised TimeRanges object that represents the ranges of the media resource, if any, that the user agent has buffered, at the time the attribute is evaluated. Users agents must accurately determine the ranges available, even for media streams where this can only be determined by tedious inspection.

http://www.w3.org/html/wg/drafts/html/master/semantics.html#dom-media-buffered

}

return bufferedDuration / this.duration_;
bufferedPercent() {
return bufferedPercent(this.buffered(), this.duration_);
}

stopTrackingProgress() {
Expand All @@ -166,21 +147,6 @@ class Tech extends Component {

this.on('play', this.trackCurrentTime);
this.on('pause', this.stopTrackingCurrentTime);
// timeupdate is also called by .currentTime whenever current time is set

// Watch for native timeupdate event only
var onTimeUpdate = function(e){
if (e.manuallyTriggered) return;

this.off('timeupdate', onTimeUpdate);

// Update known progress support for this playback technology
this.featuresTimeupdateEvents = true;
// Turn off manual progress tracking
this.manualTimeUpdatesOff();
};

this.on('timeupdate', onTimeUpdate);
}

manualTimeUpdatesOff() {
Expand Down
35 changes: 35 additions & 0 deletions src/js/utils/buffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { createTimeRange } from './time-ranges.js';

/**
* Compute how much your video has been buffered
* @param {Object} Buffered object
* @param {Number} Total duration
* @return {Number} Percent buffered of the total duration
* @private
*/
export function bufferedPercent(buffered, duration) {
var bufferedDuration = 0,
start, end;

if (!duration) {
return 0;
}

if (!buffered || !buffered.length) {
buffered = createTimeRange(0,0);
Copy link
Member

Choose a reason for hiding this comment

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

whitespace after comma

}

for (let i=0; i<buffered.length; i++){
Copy link
Member

Choose a reason for hiding this comment

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

whitespace around operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those missing whitespaces were actually original cut/paste of the old code but I'll fix it anyway :)

Copy link
Member

Choose a reason for hiding this comment

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

You only notice these things when someone touches something around them :P

start = buffered.start(i);
end = buffered.end(i);

// buffered end can be bigger than duration by a very small fraction
if (end > duration) {
end = duration;
}

bufferedDuration += end - start;
}

return bufferedDuration / duration;
}
17 changes: 0 additions & 17 deletions test/unit/tech/tech.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,6 @@ test('should synthesize timeupdate events by default', function() {
equal(timeupdates, 1, 'triggered at least one timeupdate');
});

test('stops timeupdates if the tech produces them natively', function() {
var timeupdates = 0, tech, expected;
tech = new Tech();
tech.on('timeupdate', function() {
timeupdates++;
});

tech.trigger('play');

// simulate a native timeupdate event
tech.trigger('timeupdate');

expected = timeupdates;
this.clock.tick(10 * 1000);
equal(timeupdates, expected, 'did not simulate timeupdates');
});

test('stops manual timeupdates while paused', function() {
var timeupdates = 0, tech, expected;
tech = new Tech();
Expand Down