Filter the source given to `Player#src` before use #4030

brandonocasey · 2017-02-03T17:11:39Z

Description

Add a util that filters all possible Tech~SourceObjects into a valid Tech~SourceObject (see tests)
Remove Player#sourceList_ as it is no longer in use

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Unit Tests updated or fixed
Reviewed by Two Core Contributors

gkatsev · 2017-02-03T18:23:34Z

src/js/player.js

@@ -2218,27 +2219,23 @@ class Player extends Component {
   *         The current video source when getting
   */
  src(source) {
-    if (source === undefined) {


why was this part removed?

Ah, I see. I think it's better to keep it. Why do extra work if we know for sure we can exit early here?

yeah I guess we can keep it to do less work, I just though it would be better to have less code. I don't think that filterSource will do anything with it other than turn it into an empty array. I guess it depends on wether or not we want to log an error if an invalid source array is passed to us vs being used as a getter.

video.js throws the following errors:

player.src([]); // No compatible source was found for this media player.src({}); // The media could not be loaded, either because the server or network failed or because the format is not supported.

It looks like vjs 5 special cased empty arrays to have the custom object and with an empty object it basically tried to set an empty string and had it raise the error from the video element.

I think we should maintain this functionality. Though, having player.src({}) produce the same error as player.src([]) sounds fine too.

Do we want to wrap the this.error call in a setTimeout like what was done in sourceList_? and also done below

Probably, yes.

gkatsev · 2017-02-03T18:26:18Z

src/js/player.js


-      this.cache_.sources = [src];
+    // no valid sources, return the current source
+    if (!src.length) {


Should we treat src([]) the same as src()?

I think we want to exit early in the case where an empty source is passed, I could see logging an error, but not much else would be different.

gkatsev · 2017-02-03T18:27:59Z

src/js/utils/filter-source.js

+import {isObject} from './obj';
+
+/**
+ * Filter out single bad source objects or multiple source objects in an


I don't think it's worth handling nested arrays of sources.

I will remove that from the doc comments/unit tests if you like, but the code will probably stay the same either way.

gkatsev · 2017-02-03T18:35:11Z

src/js/player.js

    }

-    this.cache_.source = src;
+    this.cache_.sources = src;
+    src = src[0];


with this, we'd only ever be able to try running through the first source. See line 2247.

Or at least, we don't have the benefit of filterSource for subsequent iterations and the two arrays aren't necessary in sync.

I think 2247 uses the original source object and not src

Yes, but previously source was the source of truth for a list of sources. With the filterSource functionality, the output of filterSource became the source of truth.

gkatsev · 2017-02-03T18:35:55Z

src/js/player.js


+    this.cache_.source = src;
    this.currentType_ = src.type;


just realized that we need to move/copy this down into line 2264

is there any reason not to use the cache_ object here?

probably no reason not to ad that in. This was just how it used to be.

I am wondering two things now:

Do we need to change this.cache_.source, this.cache_.src, and this.cache_.type in the middleware.setSource callback and also above it? or an we just do it inside?

Should we even cache this.cache_.src and this.cache_.type or should we just cache this.cache_.source and use that when we need to get the type or the src url

If you have a source like this:

{ src: 'foo', type: 'video/foo' }

With a middleware that returns an m3u8 for it, we want the following behavior:

player.src(); // 'http://example.com/video.m3u8' player.currentSrc(); // 'foo' player.currentSource(); // {src: 'foo', type: 'video/foo'}

Let me double check what we talked about for currentType previously.

Oh, maybe we don't need to move currentType because we want it to match the type of the video returned by player.currentSrc(). So, in the above scenario, it'll be video/foo.

brandonocasey · 2017-02-06T16:51:42Z

@gkatsev this has been updated with a lot of the things that you asked about

brandonocasey · 2017-02-06T16:54:23Z

src/js/player.js

@@ -2385,14 +2357,7 @@ class Player extends Component {
   *         The current source object
   */
  currentSource() {
-    const source = {};
-    const src = this.currentSrc();


This code didn't make sense because this.currentSrc was looking at the same variable (this.cache_.source).

gkatsev · 2017-02-06T17:04:41Z

src/js/player.js

@@ -2218,37 +2219,39 @@ class Player extends Component {
   *         The current video source when getting
   */
  src(source) {
-    if (source === undefined) {


I really think this should be separate and at the top here. It makes a clear delineation between when it's being called as a getter vs when it's called as a setter.

added back in in the most recent commit

gkatsev · 2017-02-06T17:06:35Z

src/js/player.js

@@ -2415,7 +2380,7 @@ class Player extends Component {
   *         The source MIME type
   */
  currentType() {
-    return this.currentType_ || '';
+    return this.currentSource() && this.currentSource().type || '';


any other references to this.currentType_ that need to be removed?

using ack and ripgrep returns no results for this.currentType_ now, so I don't think so.

gkatsev · 2017-02-06T18:58:03Z

src/js/player.js

@@ -2264,11 +2266,26 @@ class Player extends Component {
      }

      this.changingSrc_ = false;
-      this.cache_.src = src_.src;
+      this.cache_.source = src[0];
+      this.cache_.src = src_;


this should be src_.src.

gkatsev · 2017-02-06T19:01:59Z

Also, should we clear out the source caches if we error out? So, if everything worked but then you do player.src([]), player.currentSources() still returns the previously loaded sources.

brandonocasey · 2017-02-06T19:06:01Z

Maybe we should reset the source cache at the top of the function if it is not being used as a getter?

Ex:

src(source) {
  if (!source) {
   return this.cache_.src;
  }

  this.cache_.src = null;
  this.cache_.source = null;
  this.cache_.sources = [];
}

gkatsev

Follow up from this PR is here: #4036

gkatsev · 2017-02-06T20:47:33Z

Making the executive decision to merge even though travis is being silly. Hopefully master will be fine.

better source function through filtering

41fb1db

brandonocasey force-pushed the fix/better-src-function branch from 6805695 to 41fb1db Compare February 3, 2017 17:21

brandonocasey added 2 commits February 3, 2017 12:57

handle nested arrays

a1fd850

always return an array, add jsdocs

7b28bb5

brandonocasey force-pushed the fix/better-src-function branch 2 times, most recently from a0721c5 to 602e6ce Compare February 3, 2017 18:18

better doc comments and import

0fe92a8

brandonocasey force-pushed the fix/better-src-function branch from 602e6ce to 0fe92a8 Compare February 3, 2017 18:19

gkatsev requested changes Feb 3, 2017

View reviewed changes

code review changes

827d7ba

brandonocasey commented Feb 6, 2017

View reviewed changes

gkatsev reviewed Feb 6, 2017

View reviewed changes

brandonocasey added 3 commits February 6, 2017 12:14

bring back specific getter usage

f834b96

added a doc comment for src_

e1e0f1d

whoops need to test for undefined not against

e8bb74d

gkatsev requested changes Feb 6, 2017

View reviewed changes

brandonocasey added 2 commits February 6, 2017 14:07

code review fix

eb989fd

better naming, and fix src mixup

93776a3

gkatsev approved these changes Feb 6, 2017

View reviewed changes

gkatsev merged commit 6541467 into master Feb 6, 2017

gkatsev deleted the fix/better-src-function branch February 6, 2017 20:48

misteroneill mentioned this pull request Nov 14, 2018

Cannot use non-string sources, which may conflict with middleware #5585

Open

Filter the source given to Player#src before use #4030

Filter the source given to Player#src before use #4030

Conversation

brandonocasey commented Feb 3, 2017

Description

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Feb 6, 2017

brandonocasey commented Feb 6, 2017 • edited Loading

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev commented Feb 6, 2017

Filter the source given to `Player#src` before use #4030

Filter the source given to `Player#src` before use #4030

brandonocasey Feb 3, 2017 •

edited

Loading

brandonocasey commented Feb 6, 2017 •

edited

Loading