setMediaElement not updating progress bar

[jahangiranwari]

With the new setMediaElement feature to attach a <audio> at runtime we noticed that although we are able to share the same underlying HTMLMediaElement element between two players it doesn't update the progress bar on both players.

To demonstrate the issue and using React Global Player example as reference created this gist. When we click Play the track starts playing in the global player and we can seek on either player indicating that they are sharing the same underlying <audio> element. But the track player's progress bar doesn't move.

On deeper investigation identified the root cause as missing timeupdate listener on the newly attached HTMLMediaElement. The same can be confirmed by uncommenting these lines in the same gist.

Possible Solutions

One possible solution could be to track the timeupdate listener independently of other subscriptions and subscribe/unsubscribe to it in setMediaElement.

Another solution could be to have public methods that would allow us to subscribe/unsubscribe from all player events currently used in initPlayerEvents.

[katspaugh]

Good catch. I think adding setMediaElement was a bad idea. It essentially requires a re-initialization of the entire waveform and all the events, so I don’t see the point of such a setter.

As you can see in the original global player example, it’s possible to simply re-create a wavesurfer instance.

[katspaugh]

You can avoid destroying it if you pass an external media to your playlist tracks. Wavesurfer in that case won't touch it on destroy.
E.g.

const audio = new Audio()
const playlistTrack = WaveSurfer.create({
  media: audio,
  // ...
})

[jahangiranwari]

We experimented with that as well and realized that it resulted in a lot of zombie audio objects lying around as the user navigates through different screens and in some cases when the user navigated away and came back to the same screen we were creating duplicate audio objects for the same playlist track player.

[katspaugh]

If you just create an audo w/o adding it to the DOM, it should be garbage-collected as soon as the last reference to it is deleted. As long as you're destroying your wavesurfer instances correctly, only the global player will keep a reference to the currently played audio.

[jahangiranwari]

Thanks for the suggestion. We experimented changing the flow of setting the media on global player instead of track player. Although it works for on-page track changes and screen navigation it breaks our "Playlist Play" feature where the user is able to continue listening to playlist even after they navigate away from playlist screen. This is because once user navigates away from playlist page the previous/next track's audio is no longer available as it has been garbage collected.

We observed another side-effect of using external media, between screen navigation the global player stops playing. On deeper investigation we discovered that when destroy of wavesurfer instance is called we pause the current audio.

[katspaugh]

I should probably fix this, it shouldn’t pause it.

[jahangiranwari]

@katspaugh is it safe to assume that setMediaElement would be retained? Even with its limitation it appears to be best suited for complex apps where performance is critical and we would be relying on it heavily in our app.

[jahangiranwari]

Thoughts on fixing it as shown below?

class WaveSurfer extends Player<WaveSurferEvents> {
  protected mediSubscriptions: Array<() => void> = []

  // ...
  // ...

  private initPlayerEvents() {
    this.mediSubscriptions.push(
      this.onMediaEvent("timeupdate", () => {
        const currentTime = this.getCurrentTime()
        this.renderer.renderProgress(currentTime / this.getDuration(), this.isPlaying())
        this.emit("timeupdate", currentTime)
      }),

      this.onMediaEvent("play", () => {
        this.emit("play")
        this.timer.start()
      }),

      this.onMediaEvent("pause", () => {
        this.emit("pause")
        this.timer.stop()
      }),

      this.onMediaEvent("emptied", () => {
        this.timer.stop()
      }),

      this.onMediaEvent("ended", () => {
        this.emit("finish")
      }),

      this.onMediaEvent("seeking", () => {
        this.emit("seeking", this.getCurrentTime())
      })
    )
  }

  private unsubscribePlayerEvents() {
    this.mediSubscriptions.forEach((unsubscribe) => unsubscribe())
  }

  public setMediaElement(element: HTMLMediaElement) {
    this.unsubscribePlayerEvents()
    super.setMediaElement(element)
    this.initPlayerEvents()
  }

  public destroy() {
    this.emit("destroy")
    this.plugins.forEach((plugin) => plugin.destroy())
    this.subscriptions.forEach((unsubscribe) => unsubscribe())
    this.unsubscribePlayerEvents()
    this.timer.destroy()
    this.renderer.destroy()
    super.destroy()
  }
}

[katspaugh]

Yeah, looks good to me!
The only thing I would change is to clear the this.mediSubscriptions array after unsubscribing.

[jahangiranwari]

Sound good. Will work on a PR.

[katspaugh]

Appreciate the PR! I’ve merged it and released 7.3.9. Kudos!

[jahangiranwari]

Thank you for the quick release.