fix: add live stream error screen & auto refresh #819
Conversation
✅ Deploy Preview for cld-video-player ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for cld-vp-esm-pages ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/video-player.js
Outdated
| _setRetryForLiveStream(delaySeconds = 10) { | ||
| const tempVideo = document.createElement('video'); | ||
| tempVideo.setAttribute('crossorigin', 'anonymous'); | ||
| const liveStreamUrl = this.currentSourceUrl(); | ||
|
|
||
| const retryLiveStreamLoad = () => { | ||
| setTimeout(() => { | ||
| tempVideo.src = liveStreamUrl; | ||
| tempVideo.load(); | ||
| }, delaySeconds * 1000); | ||
| }; | ||
|
|
||
| tempVideo.onprogress = () => { | ||
| tempVideo.onerror = null; | ||
| tempVideo.oncanplaythrough = null; | ||
| this.source(liveStreamUrl); | ||
| }; | ||
| tempVideo.onerror = retryLiveStreamLoad; | ||
|
|
||
| retryLiveStreamLoad(); | ||
| } |
There was a problem hiding this comment.
@jakub-roch we already have a reTryVideo handler.
can we re-use that instead?
https://github.com/cloudinary/cloudinary-video-player/blob/master/src/video-player.js#L418
| width: playerOptions.width, | ||
| withCredentials: playerOptions.withCredentials, | ||
| debug: playerOptions.debug, | ||
| type: playerOptions.type, |
tsi
left a comment
tsi
left a comment
There was a problem hiding this comment.
@jakub-roch looks much better.
I'm approving to unblock you but please see my comments
| const maxTries = this.videojs.options_.maxTries || 3; | ||
| const videoReadyTimeout = this.videojs.options_.videoTimeout || 55000; |
There was a problem hiding this comment.
Shouldn't these also use the RETRY_DEFAULT_TIMEOUT const?
There was a problem hiding this comment.
Im not sure abut changing it - will check it to make sure we can replace it with RETRY_DEFAULT_TIMEOUT
docs/live-streaming.html
Outdated
| videoPlayerInstance = cloudinary.videoPlayer('player', { | ||
| cloudName, | ||
| publicId: liveStreamingUrl, | ||
| type: 'live', | ||
| controls: true, | ||
| sourceTypes: ['hls'], | ||
| }); |
There was a problem hiding this comment.
Instead of an empty player, can we show something like this example by default? better if it comes from demo/prod cloud
| if (s >= (/iPad|iPhone|iPod/.test(navigator.userAgent) ? 1 : 4)) { | ||
| this.nbCalls = 0; | ||
| return true; | ||
| reloadVideoUntilAvailable(maxNumberOfCalls = Number.POSITIVE_INFINITY, timeout = RETRY_DEFAULT_TIMEOUT) { |
There was a problem hiding this comment.
Do we really want to try this until infinity by default? can't this create issues?
There was a problem hiding this comment.
I dont see a reason why not - it makes sure that until user close tab / browser it will still re-try to load video
it just repeats fetch calls for video to see if its available
| } | ||
|
|
||
| return false; | ||
| if (this.reloadVideoRetriesCount < maxNumberOfCalls) { |
There was a problem hiding this comment.
What is this.reloadVideoRetriesCount? is there supposed to be a this.reloadVideoRetriesCount++ somewhere?
There was a problem hiding this comment.
nice catch, I missed it 👍
src/video-player.js
Outdated
| this.videoElement = getResolveVideoElement(elem); | ||
|
|
||
| this.options = extractOptions(this.videoElement, initOptions); | ||
| this.isLiveStream = this.options.videojsOptions.type === 'live'; |
There was a problem hiding this comment.
Should we set this as part of player constructor? Imean, the initial source can be live but the same player can switch to other sources that are not.
Here we do source.resourceConfig().type === 'live', would that be better?
There was a problem hiding this comment.
good point, I added isLiveStream fn that is called every time to check current type 👍
There was a problem hiding this comment.
anyway its edge case that type of live will be changed but anyway good to handle it
Add live stream error screen when
type === 'live'& auto refresh every 10 seconds