Implement source-first ordering in "selectSource" as an optional over… #2847

imbcmdth · 2015-11-23T20:00:17Z

…ride to the currently implemented default "tech-first" ordering

Video.js currently uses a tech-first slection algorithm (ie. find the first source that can be decoded by the first tech. if none, try the next tech and repeat.) but the video element does something more like source-first. This commit implements the semantics of an optional source-first selection system in video.js

pam · 2015-11-23T20:07:33Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 0b7ca4129f4f3953d9697e11cfaf11745051ed9d
Build details: https://travis-ci.org/pam/video.js/builds/92788871

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-23T20:10:25Z

src/js/player.js

+    };
+
+    let foundSourceAndTech;
+    let flipFn = (fn) => (a, b) => fn(b, a);


why not just flip and finder?

pam · 2015-11-23T20:14:30Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 71bfad903a78fdec6f2b9e3c00fc3fb824122877
Build details: https://travis-ci.org/pam/video.js/builds/92790585

(Please note that this is a fully automated comment.)

pam · 2015-11-23T20:32:02Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 9a79e9eca6e279b92156639656b87ca42257fda6
Build details: https://travis-ci.org/pam/video.js/builds/92793794

(Please note that this is a fully automated comment.)

pam · 2015-11-23T20:33:44Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 8aec518ef57c217aed7078a8ebca358970386822
Build details: https://travis-ci.org/pam/video.js/builds/92794152

(Please note that this is a fully automated comment.)

pam · 2015-11-23T20:47:13Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 423dd27f6cf95424f505211974c5ae870011543f
Build details: https://travis-ci.org/pam/video.js/builds/92797548

(Please note that this is a fully automated comment.)

dmlap · 2015-11-23T21:20:32Z

test/unit/player.test.js

+    // Extend TechFaker to create a tech that plays the only mime-type that TechFaker
+    // will not play
+    import Tech from '../../src/js/tech/tech.js';
+    import TechFaker from './tech/tech-faker.js';


I don't know if we were ever explicit about this but I thought the style-consensus among the @videojs/core-committers was for imports to live at the top of the file only.

They are required to be at the top of the file. Only reason it hasn't complained is because we're putting babel into loose mode.

dmlap · 2015-11-23T21:26:08Z

We probably should add some doc for the new option.

gkatsev · 2015-11-23T21:28:29Z

test/unit/player.test.js

+    fixture.innerHTML += html;
+    var tag = document.getElementById('example_1');
+
+    var player = new Player(tag, { techOrder: ['techFaker', 'playsUnsupported'], sourceOrder: true });


Almost every other tests in that file is using var seems like a silly thing to change for one test.

We kind of skimped on the migration of tests to full es6.
However, new code should definitely be with lets and what not.

gkatsev · 2015-11-23T21:38:23Z

Hm... I'm not sure we really have a place to document these options.

gkatsev · 2015-11-23T21:42:02Z

Probably best place to document the option, besides the function's jsdoc might be in another section on this page: https://github.com/videojs/docs/blob/master/docs/guides/options.html
Currently, the options there are only for options that are also video element attributes.

imbcmdth · 2015-11-23T21:52:44Z

@gkatsev Hmm. I added documentation to the tech.md guide since it is there where we mention techOrder.

gkatsev · 2015-11-23T21:53:04Z

@rcrooks do we use the docs/ directory in this repo anymore?

gkatsev · 2015-11-23T21:54:09Z

Yeah, I guess that location is fine. Now we need to figure out whether it should be in this repo or in the docs repo. Also, the jsdoc for the function should be updated.

gkatsev · 2015-11-23T21:55:51Z

src/js/player.js

+
+    let foundSourceAndTech;
+    let flip = (fn) => (a, b) => fn(b, a);
+    let finder = (techArr, source) => {


if you want to be fancy:

let finder = ([techName, tech], source) => { if (tech.canPlaySource(source)) { return {source: source, tech: techName}; } };

Let's be fancy.

pam · 2015-11-23T21:58:03Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: a1de5ca4d779ddfc7b0a27cda1052d555e2a1153
Build details: https://travis-ci.org/pam/video.js/builds/92813318

(Please note that this is a fully automated comment.)

pam · 2015-11-23T22:03:41Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: c37f28b06029e8dcc42421e45ab4552dbab4f4ab
Build details: https://travis-ci.org/pam/video.js/builds/92814381

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-23T22:06:27Z

src/js/player.js

+    let techs =
+      this.options_.techOrder
+        .map(toTitleCase)
+        .map((techName) => {


// `Component.getComponent(...)` is for support of old behavior of techs // being registered as components. // Remove once that deprecated behavior is removed. .map((techName) => [techName, Tech.getTech(techName) || Component.getCompnent(techName)])

I don't like putting that much distance between chained property accesses. The reason the comment is inside the function in the first place is that it doesn't "break the chain" and indentation makes things even more clear.

I'm ok either way. If we update getTech to automatically try calling getComponent, then, we could probably just remove the comment altogether and have the nice oneliner here.

pam · 2015-11-23T22:09:10Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: aed2ad0174b0b9158ddfacb5182777836af1c9ae
Build details: https://travis-ci.org/pam/video.js/builds/92815408

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-24T06:29:06Z

docs/guides/tech.md

+
+With a player setup as follows:
+
+  <video data-setup='{"techOrder": ["html5", "flash"]}, "sourceOrder": true'>


sourceOrder should be moved to the data-setup json.

gkatsev · 2015-11-24T06:29:37Z

Small comment on the docs.
LGTM.

pam · 2015-11-24T17:05:57Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 82df17e41cea6d94b160c0ff6624dd9a5d712b9a
Build details: https://travis-ci.org/pam/video.js/builds/92977110

(Please note that this is a fully automated comment.)

…ride to the currently implemented default "tech-first" ordering Video.js currently uses a tech-first slection algorithm (ie. find the first source that can be decoded by the first tech. if none, try the next tech and repeat.) but the video element does something more like source-first. This commit implements the semantics of an optional source-first selection system in video.js

…h-order anymore

pam · 2015-11-25T22:05:02Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 4aef350
Build details: https://travis-ci.org/pam/video.js/builds/93253360

(Please note that this is a fully automated comment.)

@gkatsev

* upstream/stable: (479 commits) v5.4.4 @gkatsev switched to use custom vtt.js from npm. closes videojs#2905 v5.4.3 @gkatsev updated options customizer and github-release options. closes videojs#2903 v5.4.2 @gkatsev updated grunt-release config. closes videojs#2900 v5.4.1 @gkatsev added chg- and github- release for next releases. closes videojs#2899 v5.4.0 @gkatsev added ability to release next tag from master. closes videojs#2894 @gkatsev added nullcheck for cues in updateForTrack. Fixes videojs#2870. closes videojs#2896 @chemoish emulated HTMLTrackElement to enable track load events. closes videojs#2804 @gkatsev added a Player#reset method. Fixes videojs#2852. closes videojs#2880 @nick11703 changed multiline comments in sass with single-line comments. closes videojs#2827 @gkatsev added Player#tech. Fixes videojs#2617. closes videojs#2883 @misteroneill updated videojs-ie8 to 1.1.1. closes videojs#2869 v5.3.0 @imbcmdth added sourceOrder option for source-first ordering in selectSource. closes videojs#2847 @forbesjo updated formatTime to not go negative. closes videojs#2821 v5.2.4 ...

imbcmdth force-pushed the source-first-selection branch from 0b7ca41 to 71bfad9 Compare November 23, 2015 20:08

gkatsev reviewed Nov 23, 2015
View reviewed changes

src/js/player.js

};

let foundSourceAndTech;

let flipFn = (fn) => (a, b) => fn(b, a);

Copy link

Member

gkatsev Nov 23, 2015

Choose a reason for hiding this comment

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

why not just flip and finder?

imbcmdth force-pushed the source-first-selection branch from 9a79e9e to 8aec518 Compare November 23, 2015 20:26

imbcmdth force-pushed the source-first-selection branch from 8aec518 to 423dd27 Compare November 23, 2015 20:41

gkatsev mentioned this pull request Nov 23, 2015

Loop through sources then techs #2844

Closed

dmlap reviewed Nov 23, 2015
View reviewed changes

gkatsev reviewed Nov 23, 2015
View reviewed changes

gkatsev mentioned this pull request Nov 24, 2015

We have docs/guides in two places videojs/docs#14

Closed

3 tasks

gkatsev reviewed Nov 24, 2015
View reviewed changes

gkatsev added the minor This PR can be added to a minor release. It should not be added to a patch release. label Nov 24, 2015

jrivera added 7 commits November 25, 2015 16:56

Changed some function names and added back some of the removed comments

03cbcba

Moved import to top of test and added documentation

45369d6

Updated function comments to not that selectSource isn't strictly tec…

182d28f

…h-order anymore

Let's be fancy.

5f20b16

Convert a single test to let in a sea of tests with var because dogma

0b91f38

Fix to docs

4aef350

imbcmdth force-pushed the source-first-selection branch from 82df17e to 4aef350 Compare November 25, 2015 21:59

gkatsev mentioned this pull request Nov 25, 2015

Couple suggestions for better fallback behavior (HLS vs MP4) #1384

Closed

gkatsev closed this in 69b89e5 Nov 25, 2015

gkatsev mentioned this pull request Nov 25, 2015

5.3.0 release proposal #2861

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement source-first ordering in "selectSource" as an optional over… #2847

Implement source-first ordering in "selectSource" as an optional over… #2847

imbcmdth commented Nov 23, 2015

pam commented Nov 23, 2015

gkatsev Nov 23, 2015

pam commented Nov 23, 2015

pam commented Nov 23, 2015

pam commented Nov 23, 2015

pam commented Nov 23, 2015

dmlap Nov 23, 2015

gkatsev Nov 23, 2015

dmlap commented Nov 23, 2015

gkatsev Nov 23, 2015

imbcmdth Nov 23, 2015

gkatsev Nov 23, 2015

gkatsev commented Nov 23, 2015

gkatsev commented Nov 23, 2015

imbcmdth commented Nov 23, 2015

gkatsev commented Nov 23, 2015

gkatsev commented Nov 23, 2015

gkatsev Nov 23, 2015

imbcmdth Nov 23, 2015

pam commented Nov 23, 2015

pam commented Nov 23, 2015

gkatsev Nov 23, 2015

imbcmdth Nov 23, 2015

gkatsev Nov 23, 2015

pam commented Nov 23, 2015

gkatsev Nov 24, 2015

gkatsev commented Nov 24, 2015

pam commented Nov 24, 2015

pam commented Nov 25, 2015


		With a player setup as follows:

		<video data-setup='{"techOrder": ["html5", "flash"]}, "sourceOrder": true'>

Implement source-first ordering in "selectSource" as an optional over… #2847

Implement source-first ordering in "selectSource" as an optional over… #2847

Conversation

imbcmdth commented Nov 23, 2015

pam commented Nov 23, 2015

Choose a reason for hiding this comment

pam commented Nov 23, 2015

pam commented Nov 23, 2015

pam commented Nov 23, 2015

pam commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlap commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Nov 23, 2015

gkatsev commented Nov 23, 2015

imbcmdth commented Nov 23, 2015

gkatsev commented Nov 23, 2015

gkatsev commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Nov 23, 2015

pam commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Nov 23, 2015

Choose a reason for hiding this comment

gkatsev commented Nov 24, 2015

pam commented Nov 24, 2015

pam commented Nov 25, 2015