benwiley4000 / cassette

📼 A flexible media player component library for React that requires no up-front config
https://benwiley4000.github.io/cassette/styleguide
MIT License
185 stars 28 forks source link

Source load error event handler #352

Closed danielr18 closed 5 years ago

danielr18 commented 5 years ago

I'm interested in being able to track errors related to a track not loading, e.g. the URL returns a 404. In order to do that, it looks like you need to add an event listener to the source elements. See: https://stackoverflow.com/questions/23231404/detecting-html5-audio-element-file-not-found-error

My first though was having something similar this.props.createMediaElement for source elements, and use that here: https://github.com/benwiley4000/cassette/blob/2cc6e100f1214d46c5209ac15dadcaf77f6188a7/packages/core/src/PlayerContextProvider.js#L100. It would create source element, add event listener and then return it. But I'm not sure if there could be memory leaks if we remove the source elements without also removing the event listeners.

I would like to know your thoughts on how something like this could be handled

benwiley4000 commented 5 years ago

Hm - I have a couple of thoughts.

  1. There shouldn't be a memory leak - event listeners for garbage collected elements are removed automatically by the browser
  2. Do we really need a custom source element for this? Seems like we should be handling errors in a first-class way. Either with a handler context prop, or storing the error in the context, or both .. trying to figure out what makes the most sense.
danielr18 commented 5 years ago

1 - Great. 2 - Depends on how we want to handle it. Errors thrown in source element aren't MediaError, so it doesn't tell you much about why it failed, thus I don't think we need to store it in the context, if you want to add a handler context prop, that's good with me.

benwiley4000 commented 5 years ago

it doesn't tell you much about why it failed

It might tell us enough. Note the section of the spec that talks about source element errors:

The following event fires on source element:

Event name Interface Fired when...
error Event An error occurs while fetching the media data or the type of the resource is not supported media format.

So we know that either: a. The fetch could not be completed b. The media type is unsupported

Based on that I think it could be sufficient to put a boolean value in the context called mediaCannotPlay (or similar) which is set to true when a source element error is fired, and is set back to false when a new track is loading.

What do you think?

benwiley4000 commented 5 years ago

And if you really need to add an event listener to the source element from above you could just do:

<PlayerContextProvider
  playlist={playlist}
  mediaElementRef={elem =>
    for (let i = 0; i < elem.children.length; i++) {
      const source = elem[i];
      source.addEventListener('error', err => {
        console.log('do something');
      });
    }
  }}
/>
danielr18 commented 5 years ago

I think mediaCannotPlay could be good, you can easily use it display something and I could track the change in a componentDidUpdate.

With respect to the snippet, I'm not sure if that would work, since mediaElementRef is only called in componentDidMount and when the track changes, previous source elements are removed: https://github.com/benwiley4000/cassette/blob/4a3bb4f45dca44018f4f5b9623943a68a3b9822d/packages/core/src/PlayerContextProvider.js#L96

benwiley4000 commented 5 years ago

Ok I changed my mind.

Since it might be a common use case to want to remove a track from a playlist if playback fails, maybe we could also support a prop to the PlayerContextProvider called onTrackPlaybackFailure which receives the same arguments as onActiveTrackUpdate (track, trackIndex). But in addition to that I think we should have the state passed through context.

benwiley4000 commented 5 years ago

With respect to the snippet, I'm not sure if that would work, since mediaElementRef is only called in componentDidMount and when the track changes, previous source elements are removed:

Good point!

benwiley4000 commented 5 years ago

If you want to work on a PR for this go ahead - otherwise I can try to get to it soon-ish. 🙂

Thanks for bringing this up! This is something I should have addressed awhile ago. The current user experience when a track play fails is pretty subpar - you have no idea why it's not working.

danielr18 commented 5 years ago

I can probably get the PR done tonight, just to recap, we'll go with:

benwiley4000 commented 5 years ago

Yep!

benwiley4000 commented 5 years ago

Closed in #356.