No video playback with 1.0.5 when embedded in custom application

[murezzda]

Describe the bug
Currently, our web-application won't play back any videos with 1.0.5. The timestamp of the video is 00:00:00/00:00:00, and it seems the media file is not loaded. I've managed to pinpoint this to the shouldComponentUpdate function of the video player:

shouldComponentUpdate(nextProps) {
  if (nextProps.previewIsDisplayed !== this.props.previewIsDisplayed) {
    return true;
  }
  return false;
}

By Uncommenting this, the playback of the video is back to normal.

We use a .Net Core backend. The Application is started via Visual Studio in an IIS.

To Reproduce

I'm still investigating why exactly this happens, we call the compontent with

<TranscriptEditor
   transcriptData={this.state.currentTranscript}
   fileName={this.state.fileName}
   mediaUrl={this.state.mediaUrl}
   isEditable={this.state.isEditable}
   sttJsonType={this.state.sttJsonType}
   ref={this.transcriptEditorRef}
/>

where the mediaUrl is a api call to our backend which returns the media file.

I was not able to reproduce this behaviour in the Demo App, I've even managed to play back the same api call with the shouldComponentUpdate function enabled.

Question
What is exactly the behaviour of previewIsDisplayed?

[pietrop]

Thanks for flagging this @murezzda and apologies for inadvertently introducing a bug

---

shouldComponentUpdate(nextProps) {
  if (nextProps.previewIsDisplayed !== this.props.previewIsDisplayed) {
    return true;
  }
  return false;
}

just to confirm, the code above, is from the VideoPlayer?

previewIsDisplayed

previewIsDisplayed is the "Display Preview" toggle in the settings, it hides the video preview in the player for users that find it distracting, eg they might want to concentrate on the audio only.

possible problem

Looks like there's a number of props that are not taken into account in shouldComponentUpdate in videoPlayer.

• this.props.videoRef
• this.props.mediaUrl
• this.props.previewIsDisplayed

My hunch is that if any of these change after component initialisation shouldComponentUpdate in it's current setup might stop the update as it's not taking them into account.

commenting out shouldComponentUpdate

I looked into whether the check against all the props in shouldComponentUpdate the check is redundant and if you comment it out, when looking at the react inspector tool, Highlight updates, the videoPlayer keeps flashing. While with the shouldComponentUpdate not commented out it doesn't

see the colour Highlights around the VideoPlayer component

possible solution

  // to avoid unnecessary re-renders
  shouldComponentUpdate(nextProps) {
    if (nextProps.previewIsDisplayed !== this.props.previewIsDisplayed) {
      return true;
    }

    if (nextProps.mediaUrl !== this.props.mediaUrl) {
      return true;
    }

    return false;
  }

or

  // to avoid unnecessary re-renders
  shouldComponentUpdate(nextProps) {
    if (nextProps.previewIsDisplayed !== this.props.previewIsDisplayed) {
      return true;
    }

    // not sure if this is needed?
    if (nextProps.videoRef.current !== this.props.videoRef.current) {
       return true;
     }

    if (nextProps.mediaUrl !== this.props.mediaUrl) {
      return true;
    }

    return false;
  }

with these two options it doesn't re-render unnecessarily and 🤞 should work in your setup

reproduce

As you mentioned, I wasn't able to reproduce

would you able to try these two options out on your end?

Otherwise let me know and I can do an alpha release on npm with the changes for you to test out.

[murezzda]

Thanks a lot for the help @pietrop! Regarding your first question, it is indeed the code from VideoPlayer.

I've added the snipped

if (nextProps.mediaUrl !== this.props.mediaUrl) {
      return true;
    }

to the shouldComponentUpdate function and it indeed works now as intended. Thus I'm not sure if checking nextProps.videoRef.current is also needed.

Would it be possible to release this fix on npm? I would like to update our application in the near future.

Thanks again 😃

[pietrop]

fixed in #186

[pietrop]

@murezzda @bbc/[email protected] contains the change