media.currentTime doesn't reset to 0 when playlist ends

[danielr18]

The currentTime in state is reset to 0, but the one in the media element isn't, would you accept a PR to fix this?

[benwiley4000]

Hm, I think I must have only been considering the case where the active track index goes back to 0 before stopping playback.

It seems to me like maybe the currentTime shouldn't reset to 0 if we're just staying on the last track? What do you think?

[benwiley4000]

@danielr18 I tried to repro this bug and I can't. Could you share a repro or at least tell me what browser you're running? Maybe I didn't understand what the issue was. Here are the scenarios I tested:

Scenario A

Conditions:

• loadFirstTrackOnPlaylistComplete: false
• state.cycle: false Outcome on last track ended: Stays on last track, state.currentTime is not reset to 0, media.currentTime is not reset to 0

Scenario B

Conditions:

• loadFirstTrackOnPlaylistComplete: true
• state.cycle: false Outcome on last track ended: Loads first track, state.currentTime is reset to 0, media.currentTime is reset to 0.

Browser: Chrome 70

[danielr18]

Scenario A is the same for me, as well as Scenario B, but the latter one only when the playlist has more than one track.

Here's a repro: https://codesandbox.io/s/r5noo48pqm

state.currentTime is what we can see in the progress bar, and you can get media.currentTime, by accessing mediaRef.currentTime in the console.

Browser: Chrome 71.0.3578.98

[benwiley4000]

Ahhh, right, this makes sense. Thanks for clarifying.

The problem is this block:

https://github.com/benwiley4000/cassette/blob/next/packages/core/src/PlayerContextProvider.js#L357

    if (prevSources[0].src !== newSources[0].src) {
      setMediaElementSources(this.media, newSources);
      this.media.setAttribute(
        'poster',
        this.props.getPosterImageForTrack(newTrack)
      );

      if (!this.state.shuffle) {
        // after toggling off shuffle, we defer clearing the shuffle
        // history until we actually change tracks - if the user quickly
        // toggles  shuffle off then back on again, we don't want to have
        // lost our history.
        this.shuffler.clear();
      }
    }

We run those instructions only if the track src has changed. I test this instead of activeTrackIndex because I don't want to interrupt playback in case of a change to the playlist contents.

This doesn't work well when the new track is the same as the last. We would probably have a similar issue if you made a playlist filled with multiple instances of the same track.

My suggestion

• Create a new piece of state called awaitingNewTrack which is false by default.
• Set awaitingNewTrack to true in the function getGoToTrackState (https://github.com/benwiley4000/cassette/blob/next/packages/core/src/PlayerContextProvider.js#L73).
• Change the offending condition to be:

  if (
    this.state.awaitingNewTrack ||
    prevSources[0].src !== newSources[0].src
  ) {

• Set awaitingNewTrack back to false inside that conditional block.

Would you still be interested in working on a PR to fix? :slightly_smiling_face:

[benwiley4000]

@danielr18 were you still interested in helping with this?

[danielr18]

Yes, your suggestion seems to work well for all cases, although it's a breaking change, you could call onSelectTrackIndex with the activeTrackIndex before and it wouldn't reload the media element, however 2.0.0 is still alpha, so these should be expected.

I'll have the PR created today.

[benwiley4000]

Hm.. that's a good point. I hadn't considered that. And I do want to preserve that behavior. If someone clicks on a track they're already listening to it shouldn't reload. We just want that to happen when the track has actually finished and it's restarting.

Here's an idea... maybe getGoToTrackState should take an extra boolean argument called forceNewTrackLoad which is false by default and only passed as true when it's called internally by backSkip or forwardSkip? And then awaitingNewTrack will be based on that.

Thanks for helping with this!

[danielr18]

Yes, makes sense, basically like shouldPlay.

[benwiley4000]

Yeah, right. Maybe it should also start with should for consistency.. shouldForceTrackLoad?

[danielr18]

or shouldTrackReload

[danielr18]

We might not have to include track in the variables we are introducing, since what it really does is reloading the media element (media.load), and it would be more consistent, we don't have shouldTrackPlay.

Thus I think shouldReload/shouldForceLoad and awaitingLoad are better names.