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

Tech 2.0 additional fixes #2166

Closed
wants to merge 11 commits into from
Closed

Tech 2.0 additional fixes #2166

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
815518e
Remove the old options default arg in Tech
eXon May 18, 2015
7a08b45
this.tech.emitTapEvents(); should be handled by the tech
eXon May 18, 2015
8040884
De-dupe the bufferedPercent code in both Tech and Player
eXon May 18, 2015
39a6d7f
Have the player generate the tech ID
eXon May 18, 2015
152717e
Added autoplay/preload/loop/muted to tech option
eXon May 18, 2015
1766e2c
Remove the watch for native timeupdates
eXon May 18, 2015
e3c0285
Fixed the JSDoc for bufferedPercent
eXon May 18, 2015
5d19240
Removed the unit test for native timeupdate
eXon May 18, 2015
6f455f4
Added cute whitespaces
eXon May 18, 2015
aa998c9
Merge remote-tracking branch 'upstream/master'
eXon May 21, 2015
956d77a
buffer should always return a TimeRange
eXon May 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 8 additions & 26 deletions src/js/player.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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);
}

for (let 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;
}
17 changes: 0 additions & 17 deletions test/unit/tech/tech.test.js
View file
Open in desktop
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