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

Track duration = Infinity when software caused connection abort (iOS devices) #355

Closed danielr18 closed 5 years ago

danielr18 commented 5 years ago

This one is a bit tricky to replicate, you have to start playing a track, put browser on background (e.g. by going home) before it plays, and then opening the browser again after a few seconds. It might take a few tries, since it doesn't always happen.

You can read more about it here: https://github.com/AFNetworking/AFNetworking/issues/4279.

The weird thing when this error happens is that the track duration is Infinity, so the user experience is pretty bad, since you can't seek or even when you play/pause, it starts from the beginning.

I opened this issue in case other people might have it later, so they know why it happens, but I don't think there's much we can do in the library to prevent this. What I'm doing on my app side is to call load() once if a track duration is Infinity, and if it's Infinity after, just let it play, since duration = Infinity isn't an error per se, it could be a live stream or bad configured host. I'm ok with the tradeoff of reloading the track, since in my use case it's pretty weird to have live stream urls, so most of the times duration will be Infinity because of this error. I only call load() once in this case, because the probability of getting the "software caused connection abort" just after it happened is pretty low, and to not be in an infinite loop if the duration = Infinity wasn't caused by the error.

benwiley4000 commented 5 years ago

Is there a MediaError we can listen to in order to detect the aborted connection that would give us a more precise indication than only checking if the track duration is Infinity? Always loading once more when it's Infinity might cause weird hiccups for unbounded streams (not sure..).

danielr18 commented 5 years ago

The media element doesn't trigger an error, it goes through the normal flow, loads metadata and then can play, but duration is Infinity. That leads me in the awkward position of having to handle the error by looking at the only "symptom" I can find. I've spent countless hours looking for something in the media element that differentiates an unbounded stream from a track that had this error, and have not been successful :(

benwiley4000 commented 5 years ago

Ah, that's annoying. This seems like something that could be semi-common on an iOS device with a slow network connection so I'd like to find a way to avoid it. I might give a go at investigating this later.. and I'm considering using your solution. If we're able to detect the Infinite track length on durationchange before the playback even starts, we might be able to prevent audio hiccups.

danielr18 commented 5 years ago

I think I've solved the hiccup issues in a similar way, when I get a different duration and the new one is Infinity, I set media.autoplay to !media.paused and call load(), and then after canplay or play event is fired, I set autoplay back to false.

benwiley4000 commented 5 years ago

How do you avoid doing this in an infinite loop?

danielr18 commented 5 years ago

I only do that on this condition: prevDuration != newDuration && newDuration === Infinity, if it's an unbounded stream, then it's going to reload and get Infinity duration again and just play. If it's a normal audio file:

danielr18 commented 5 years ago

After a month or so of tracking Infinity durations, these are the times it has happened:

image

We get around 5k daily pageviews, and each of these call media.load() after page loads, so it's not something that happens so frequently. That's why I plan to only has the fallback reload to occur only once.

benwiley4000 commented 5 years ago

Yeah that makes sense. Thanks for all the info! I'll think about this.

danielr18 commented 5 years ago

I ended up doing this, no need to use autoplay.

const DurationInfinityHandler = ({ mediaRef, activeTrack, isPlaying, onTogglePause }) => {
  const [{ duration, track }, setTrackDuration] = useState({ duration: 0, track: null })
  const durationChangeCallback = useRef()
  const isPlayingRef = useRef()

  useEffect(() => {
    durationChangeCallback.current = () => {
      setTrackDuration({ track: activeTrack, duration: mediaRef.duration })
    }
    isPlayingRef.current = isPlaying
  })

  useEffect(() => {
    const onDurationChange = () => {
      durationChangeCallback.current()
    }
    mediaRef.addEventListener('durationchange', onDurationChange)
    return () => mediaRef.removeEventListener('durationchange', onDurationChange)
  }, [])

  useEffect(
    () => {
      if (duration === Infinity) {
        mediaRef.load()
        onTogglePause(!isPlayingRef.current)
      }
    },
    [track]
  )

  return null
}
benwiley4000 commented 5 years ago

Forgot to look at this earlier sorry! I'm not a hooks master so it took me a second to understand the code but I think I get it.

Here's my basic summary of what I think is happening...

  1. You listen for the durationchange event.
  2. Each time you save a reference to the activeTrack so in the future you'll be able to reference the activeTrack at the time of the previous duration change.
  3. If the duration is infinity and the activeTrack is different than the last time the duration changed, you: a. Take note of whether the track is playing b. Reload the track c. Play the track if it was playing at the time you reloaded
benwiley4000 commented 5 years ago

Ok btw I am sort of learning hooks right now so as an exercise I tried to rewrite the custom hook you shared.. isn't this doing the same thing?

const DurationInfinityHandler = ({ mediaRef, duration, activeTrack, isPlaying, onTogglePause }) => {
  const trackRef = useRef();

  useEffect(
    () => {
      if (duration === Infinity && trackRef.current !== activeTrack) {
        mediaRef.load();
        onTogglePause(!isPlaying);
      }
      trackRef.current = activeTrack;
    },
    [activeTrack, duration]
  );

  return null;
}
danielr18 commented 5 years ago

Saw the twitter thread šŸ˜‚, it's ok to share the code btw, might help others learn doing things better (like me). This was one of my first hooks implementation to try them out, and it got to that point because I was getting bad results with the closures. Something to keep in mind in your example is that you are getting the duration as prop, and I was getting it from media ref as I wasn't using cassette props.

I also thought that I should only subscribe once to durationchange instead of sub/unsub when props change.

danielr18 commented 5 years ago

Also, I wanted to make sure that the duration I had in state, was for the correct track, in case the track prop changed. But I think using a ref for that would have worked as well. With regards to isPlaying, I'm not sure if the isPlaying value in the effect will be the correct one always if it's not in the inputs, like [activeTrack, duration, isPlaying], but because I was missing this condition: trackRef.current !== activeTrack, if I added it to the input, when isPlaying changed, the track would have reloaded.

benwiley4000 commented 5 years ago

Nooooooo lol do you follow me on twitter? šŸ˜±šŸ˜­šŸ˜„

I didn't want to share the code because I felt like I was being an ass since I haven't even tried to spend time learning how hooks work. I finally did and I think what I come up is a bit more concise but I get why you did it that way.

I see what you're saying about wanting to make sure it's the correct track. My expectation would be that the update would always run before a track changes but I guess that's not easy to guarantee since React's decision process for when to batch state updates is a bit undocumented.

I also thought that I should only subscribe once to durationchange instead of sub/unsub when props change.

Ok this might be over my head in terms of hooks knowledge.. šŸ˜„

danielr18 commented 5 years ago

I follow Dan, and he replied to the tweet, I couldn't help but think it was related to this snippet haha. Don't worry about it!

danielr18 commented 5 years ago

With regards to the track, I was getting the issue that when I played another track, it would go to the mediaRef.load code, as duration in state was Infinity still. This was because track changed first in cassette state, and then I got the new duration at some point from the event.

benwiley4000 commented 5 years ago

Right that makes sense.. I think that's where the ref set at the end of the last effect could help, but the thing you said about the subscription running again every time lets me know I still don't know what's going on with hooks šŸ˜„

danielr18 commented 5 years ago

Did my own refactor.

If we unsub on track change, it can look like this:

const DurationInfinityHandler = ({ mediaRef, activeTrack, isPlaying, onTogglePause }) => {
  const trackRef = useRef()

  useEffect(() => {
    const onDurationChange = () => {
      if (mediaRef.duration === Infinity && trackRef.current !== activeTrack) {
        mediaRef.load();
        onTogglePause(!isPlaying);
      }
      trackRef.current = activeTrack
    }
    mediaRef.addEventListener('durationchange', onDurationChange)
    return () => mediaRef.removeEventListener('durationchange', onDurationChange)
  }, [activeTrack, /* isPlaying ? */])

  return null
}

If not, something like this.

const DurationInfinityHandler = ({ mediaRef, activeTrack, isPlaying, onTogglePause }) => {
  const durationChangeCallback = useRef()
  const trackRef = useRef()

  useEffect(() => {
    // Does this need to be in an useEffect?
    durationChangeCallback.current = () => {
      if (mediaRef.duration === Infinity && trackRef.current !== activeTrack) {
        mediaRef.load();
        onTogglePause(!isPlaying);
      }
      trackRef.current = activeTrack
    }
  })

  useEffect(() => {
    const onDurationChange = () => {
      durationChangeCallback.current()
    }
    mediaRef.addEventListener('durationchange', onDurationChange)
    return () => mediaRef.removeEventListener('durationchange', onDurationChange)
  }, [])

  return null
}
benwiley4000 commented 5 years ago

Nice! Yeah I think in the first one you would need to pass isPlaying to the useEffect... So you would have the updated value in the closure outside the new onDurationChange subscription. I've decided hooks and closures don't mix super well šŸ˜„. But this is clever!