Multiple Sources not refreshing

[narkowicz]

When specifying multiple local sources in v1.0.0-beta.7 video does not refresh when new sources are provided (i.e. the browser keeps playing the first video sources passed in props.url).

This seems to be expected behaviour:

Dynamically modifying a source element and its attribute when the element is already inserted in a video or audio element will have no effect. To change what is playing, just use the src attribute on the media element directly, possibly making use of the canPlayType() method to pick from amongst available resources. Generally, manipulating source elements manually after the document has been parsed is an unnecessarily complicated approach.

https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element:the-source-element-11

I'm currently using a quick-fix where applying one of the source url's as a key prop to <ReactPlayer> forces a refresh of the child <video> element when the url changes, as suggested in https://stackoverflow.com/a/47382850/1823021

If reproducible, it would be great to handle this in the player - where a key could be applied to the <video> element, or approach changed such that a non-server render uses canPlayType() to modify src directly on video.

I don't have experience with canPlayType() in the wild but Modernizr hints at unreliability on mobile.

[cookpete]

Good spot. I think the problem lies here:

https://github.com/CookPete/react-player/blob/c1220540f69b72ae80b3cfce88eb037b75a9e25d/src/ReactPlayer.js#L31-L39

I'm using a very crude prop comparison function for shouldComponentUpdate. It will return false for any changes to url if an array is used.

The solution is probably to use a better comparison util between props and nextProps, perhaps fast-deep-equal. Originally I wanted to avoid to much processing in shouldComponentUpdate, and I didn't want to bloat the package with an unnecessary dependency.

[narkowicz]

Nice catch - I've checked my use of the component and you're right, where there are no other prop changes (other than the url sources array) the component does not re-render.

However, I also have instances where other props change (i.e. loop) and in these instances the <source> elements are being redrawn with the new and correct src but the video being played will not update in the browser. I've observed this behaviour in Chrome 65.0.3316.0 canary and Safari 11.0.2 (12604.4.7.1.6) on macOS Sierra 10.12.6.

Looks like there may be two separate issues exhibiting similar behaviour?

Just a thought - have you measured performance without shouldComponentUpdate? The trend seems to be abandoning the check altogether wherever possible - especially where deep equality checks are required.

[cookpete]

Looks like there may be two separate issues exhibiting similar behaviour?

Yeah you're right. There are two things going on: shouldComponentUpdate returning false for array URLs, and browsers deliberately ignoring any change to source elements.

The problem with using a key on the <video> element is that it would create a new element, and lose all the event bindings for onPlay, onPause, etc. We already have to re-apply the event bindings when switching between audio and video elements, but this is much more of an edge case.

If you were to do what the spec suggests, you could override the src attribute using config.file.attributes, although this seems a bit hacky:

<ReactPlayer
  url={[...]}
  config={{
    file: { 
      attributes: { 
        src: 'file.mp4' 
      } 
    } 
  }}
/>

Just a thought - have you measured performance without shouldComponentUpdate? The trend seems to be abandoning the check altogether wherever possible - especially where deep equality checks are required.

I honestly haven't given it that much thought. I assumed it would be wise not to re-render the player unnecessarily, especially if your app updates a progress bar or something on an interval and <ReactPlayer /> is constantly being re-rendered. I also wanted to avoid potential bugs with the external scripts and iframes that are generated by the various player SDKs. I think it's best to just update the prop comparison logic to allow for arrays and objects.

[narkowicz]

Thanks @cookpete - really appreciate all your good work.

I'll implement a fix for updating multiple sources in my wrapper component for now - may revisit if I have something to contribute.