Fix: signal multiple sources change to the browser

[belgattitude]

Hey @cookpete

I'm building a PWA where I need to be able to load videos from a list. I'm using multiple sources mode (webm/mp4 and multilingual tracks to set videos).

Unfortunately, if the initial loading works, subsequent updates of the sources does not trigger a reload (although the DOM shows updated <source> and <track>). See my initial ticket : #481. To illustrate the problem switch multiple sources on this demo https://soluble.io/react-player/demo/

I thought initially to change the react keys of the sources in FilePlayer.js:

 renderSourceElement = (source, index) => {
    /* This was a first attempt to fix https://github.com/CookPete/react-player/issues/481
     * Unfortunately it's not working
    */
    if (typeof source === 'string') {
      return <source key={`${source}-${index}`} src={source} />
    }
    return <source key={`${source.src}-${index}`} {...source} />
    

But it's not working.

My solution was to signal / trigger the change by setting srcObject = null when there's a source change. Both Firefox 61 and Chrome 68 seems to work (using ubuntu 18.04). To be sure it does not produce errors on others browsers, it's encloded in a try/catch...

Please let me know if it looks good to you.

Thanks for the great work.

[cookpete]

It looks like the "proper" fix for this would be to update the key of the <video> to indicate a new element is required, but this breaks the current behaviour for adding/removing event listeners to the DOM nodes – it creates a new DOM node without the event listeners set when the component mounts.

Is it possible that just calling this.player.load() would do the trick instead of srcObject = null? If so, that seems a bit less hacky and probably wouldn't need a try/catch.

Either way – thanks so much for the amount of effort you've put into this. It's really nice to get such a well-explained issue, complete with a proposed solution.

[belgattitude]

Is it possible that just calling this.player.load() would do the trick instead of srcObject = null?

Seems to work as expected with Chrome 69 and Firefox 62, and yes this looks better, so I've updated the P/R with the load() method. Thanks.

Either way – thanks so much for the amount of effort you've put into this. It's really nice to get such a well-explained issue, complete with a proposed solution.

Ahah... thank you in return ;)

BTW, you can test with the latest changes here, I guess it won't really be easy to add proper testing at that level:

https://soluble.io/react-player/demo-fixed/

Previous version
https://soluble.io/react-player/demo/

[belgattitude]

For information, this P/R does not yet fix issues with subtitles changes (Firefox)... I was unsure of how to do it in the library, so I did it in my code, see the shouldComponentUpdate() and XXXSubtitle() methods as a example workaround:

shouldComponentUpdate(nextProps: VideoPlayerProps, nextState: VideoPlayerState): boolean { 
  if (nextProps.video.videoId !== this.props.video.videoId) {
      if (this.playerRef.current !== null) {
	  const videoEl = this.playerRef.current!.getInternalPlayer() as HTMLVideoElement;
	  // Ohoh Firefox, you keep the old subtitles... let's fix that ;)
	  hideAllSubtitles(videoEl);
          // Let's explicitly load the new video :) Till #482 is merged
	  videoEl.load();
      }
      return true;
  }
  // To be tested, a better solution must be found
  if (nextProps.activeSubtitleLang !== this.props.activeSubtitleLang) {
      const videoEl = this.playerRef.current!.getInternalPlayer() as HTMLVideoElement;
      showSubtitle(videoEl, nextProps.activeSubtitleLang || 'en');
      return true;
  }
  return false;
}

const hideAllSubtitles = (video: HTMLVideoElement): void => {
  for (let i = 0; i < video.textTracks.length; i++) {
      console.log('Hidding', video.textTracks[i]);
      video.textTracks[i].mode = 'hidden';
      console.log('New value', video.textTracks[i]);
  }
}
const showSubtitle = (video: HTMLVideoElement, lang: string): void => {
  for (let i = 0; i < video.textTracks.length; i++) {
      if (video.textTracks[i].language === lang) {
	  console.log('SHOWING', video.textTracks[i]);
	  video.textTracks[i].mode = 'showing';
      } else {
	  video.textTracks[i].mode = 'hidden';
      }
  }
}

I keep it for information only.... once I have more time to look at it, I'll make a suggestion.

[cookpete]

Nice work @belgattitude. I'm assuming that the key solution would fix subtitles (and most other problems around this) but we just need to solve the problem with event listeners being lost.

[belgattitude]

Hi @cookpete,

Equality for url's seems to not take arrays into consideration, see the proposal fix: eaaea27. Does it make sense to you ? I've runned into little issue with that. Do you want me to add in a separate P/R or included here (I would love to have it merged before) ?

Secondly:

I'm assuming that the key solution would fix subtitles (and most other problems around this) but we just need to solve the problem with event listeners being lost.

I'm not sure to understand this one... Do you mean you'd prefer to (or we must) implement the "proper" fix for this would be to update the key of the <video> to indicate a new element is required way ?

Thanks for your time !

[cookpete]

I'm happy to merge this along with the isEqual fix and tackle the alternative fix separately. Thanks for your hard work!