Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mediaCannotPlay and onTrackPlaybackFailure #356

Merged
merged 6 commits into from
Feb 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/core/docs/playerContext.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
| muted | A boolean showing whether the track volume is currently muted |
| shuffle | A boolean showing whether the track is currently playing tracks in shuffle order |
| stalled | A boolean showing whether the track playback is currently stalled due to network issues |
| mediaCannotPlay | A boolean showing whether the track playback failed while fetching the media data or if the type of the resource is not supported media format. |
| playbackRate | A number showing the current rate of playback (1 is normal) |
| setVolumeInProgress | A boolean showing whether the volume is currently being adjusted |
| repeatStrategy | A value that is either "track", "playlist" or "none". Tells whether the playlist repeats at the playlist level, the track level, or none (playback stops at the end of the playlist). |
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/PlayerContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export default createSingleGlobalContext({
'stalled',
'playbackRate',
'setVolumeInProgress',
'repeatStrategy'
'repeatStrategy',
'mediaCannotPlay'
]
});
77 changes: 54 additions & 23 deletions packages/core/src/PlayerContextProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ const defaultState = {
// true if the media is currently stalled pending data buffering
stalled: false,
// true if the active track should play on the next componentDidUpdate
awaitingPlay: false
awaitingPlay: false,
/* true if an error occurs while fetching the active track media data
* or if its type is not a supported media format
*/
mediaCannotPlay: false
};

// assumes playlist is valid
Expand All @@ -81,6 +85,8 @@ function getGoToTrackState({
return {
activeTrackIndex: index,
trackLoading: isNewTrack,
mediaCannotPlay:
prevState.mediaCannotPlay && !shouldForceLoad && !isNewTrack,
currentTime: 0,
loop: isNewTrack || shouldForceLoad ? false : prevState.loop,
awaitingPlay: Boolean(shouldPlay),
Expand All @@ -89,25 +95,6 @@ function getGoToTrackState({
};
}

function setMediaElementSources(mediaElement, sources) {
// remove current sources
let firstChild;
while ((firstChild = mediaElement.firstChild)) {
mediaElement.removeChild(firstChild);
}
// add new sources
for (const source of sources) {
const sourceElement = document.createElement('source');
sourceElement.src = source.src;
if (source.type) {
sourceElement.type = source.type;
}
mediaElement.appendChild(sourceElement);
}
// cancel playback and re-scan new sources
mediaElement.load();
}

/**
* Wraps an area which shares a common [`playerContext`](#playercontext)
*/
Expand Down Expand Up @@ -166,6 +153,9 @@ export class PlayerContextProvider extends Component {
this.videoHostOccupiedCallbacks = new Map();
this.videoHostVacatedCallbacks = new Map();

// bind internal methods
this.onTrackPlaybackFailure = this.onTrackPlaybackFailure.bind(this);

// bind callback methods to pass to descendant elements
this.togglePause = this.togglePause.bind(this);
this.selectTrackIndex = this.selectTrackIndex.bind(this);
Expand Down Expand Up @@ -266,7 +256,7 @@ export class PlayerContextProvider extends Component {
media.addEventListener('loopchange', this.handleMediaLoopchange);

// set source elements for current track
setMediaElementSources(media, getTrackSources(playlist, activeTrackIndex));
this.setMediaElementSources(getTrackSources(playlist, activeTrackIndex));

// initially mount media element in the hidden container (this may change)
this.mediaContainer.appendChild(media);
Expand Down Expand Up @@ -345,7 +335,8 @@ export class PlayerContextProvider extends Component {
// if not, then load the first track in the new playlist, and pause.
return {
...baseNewState,
...getGoToTrackState({ prevState, index: 0, shouldPlay: false })
...getGoToTrackState({ prevState, index: 0, shouldPlay: false }),
mediaCannotPlay: false
danielr18 marked this conversation as resolved.
Show resolved Hide resolved
};
}

Expand All @@ -372,7 +363,7 @@ export class PlayerContextProvider extends Component {
this.state.awaitingForceLoad ||
prevSources[0].src !== newSources[0].src
) {
setMediaElementSources(this.media, newSources);
this.setMediaElementSources(newSources);
this.media.setAttribute(
'poster',
this.props.getPosterImageForTrack(newTrack)
Expand Down Expand Up @@ -452,6 +443,11 @@ export class PlayerContextProvider extends Component {
// remove special event listeners on the media element
media.removeEventListener('srcrequest', this.handleMediaSrcrequest);
media.removeEventListener('loopchange', this.handleMediaLoopchange);

const sourceElements = media.querySelectorAll('source');
for (const sourceElement of sourceElements) {
sourceElement.removeEventListener('error', this.onTrackPlaybackFailure);
}
}
clearTimeout(this.gapLengthTimeout);
clearTimeout(this.delayTimeout);
Expand Down Expand Up @@ -499,6 +495,39 @@ export class PlayerContextProvider extends Component {
});
}

setMediaElementSources(sources) {
// remove current sources
let firstChild;
while ((firstChild = this.media.firstChild)) {
this.media.removeChild(firstChild);
}
// add new sources
for (const source of sources) {
const sourceElement = document.createElement('source');
sourceElement.src = source.src;
if (source.type) {
sourceElement.type = source.type;
}
sourceElement.addEventListener('error', this.onTrackPlaybackFailure);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove the event listeners on the source elements on component unmount, in case someone is holding onto a reference to the media element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be garbage collected as you said in the issue?

https://stackoverflow.com/questions/12528049/if-a-dom-element-is-removed-are-its-listeners-also-removed-from-memory

Except in old browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, doesn't hurt to remove them, check out the last commit.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally yes, but if someone retained an outside ref to the media element then I want to make sure we don't have callbacks belonging to an unmounted component getting called.

this.media.appendChild(sourceElement);
}
// cancel playback and re-scan new sources
this.media.load();
}

onTrackPlaybackFailure(event) {
this.setState({
mediaCannotPlay: true
});
if (this.props.onTrackPlaybackFailure) {
this.props.onTrackPlaybackFailure(
this.props.playlist[this.state.activeTrackIndex],
this.state.activeTrackIndex,
event
);
}
}

registerVideoHostElement(hostElement, { onHostOccupied, onHostVacated }) {
this.videoHostElementList = this.videoHostElementList.concat(hostElement);
this.videoHostOccupiedCallbacks.set(hostElement, onHostOccupied);
Expand Down Expand Up @@ -942,6 +971,7 @@ export class PlayerContextProvider extends Component {
shuffle: state.shuffle,
stalled: state.stalled,
playbackRate: state.playbackRate,
mediaCannotPlay: state.mediaCannotPlay,
setVolumeInProgress: state.setVolumeInProgress,
repeatStrategy: getRepeatStrategy(state.loop, state.cycle),
registerVideoHostElement: this.registerVideoHostElement,
Expand Down Expand Up @@ -1019,6 +1049,7 @@ PlayerContextProvider.propTypes = {
}),
onStateSnapshot: PropTypes.func,
onActiveTrackUpdate: PropTypes.func,
onTrackPlaybackFailure: PropTypes.func,
getPosterImageForTrack: PropTypes.func.isRequired,
getMediaTitleAttributeForTrack: PropTypes.func.isRequired,
children: PropTypes.oneOfType([PropTypes.node, PropTypes.func]).isRequired
Expand Down