Dispose of player does not stop tracking current time

[apptv]

I have two players on my side. One with HTML5 Tech and one with youtube tech.

When calling dispose of the players the time tracking interval is not cleared correctly ( this.currentTimeInterval = undefined when stopTrackingCurrentTime is called ), leading to an endless loop of exceptions in the console.

Storing the Interval ID in the prototype fixed my problem.
Changing the following lines fixed my problem:

vjs.MediaTechController.prototype.trackCurrentTime = function(){
  if (vjs.MediaTechController.prototype.currentTimeInterval) { this.stopTrackingCurrentTime(); }
  vjs.MediaTechController.prototype.currentTimeInterval = setInterval(vjs.bind(this, function(){
    this.player().trigger('timeupdate');
  }), 250); // 42 = 24 fps // 250 is what Webkit uses // FF uses 15
};

// Turn off play progress tracking (when paused or dragging)
    vjs.MediaTechController.prototype.stopTrackingCurrentTime = function(){
      clearInterval(vjs.MediaTechController.prototype.currentTimeInterval);

  // #1002 - if the video ends right before the next timeupdate would happen,
  // the progress bar won't make it all the way to the end
  this.player().trigger('timeupdate');
};

[heff]

@dmlap can you take a look at this?

[dmlap]

@apptv: your fix will cause multiple players in the same page to overwrite existing interval handlers as they're initialized. I suspect it's working in your case because you're creating your HTML5 player after the YouTube player is initialized and that's incidentally cleaning up the YouTube tech's intervals. The MediaTechController prototype is a page-level singleton so it's probably not the right scope for player-level state.

I would say that the problem is the YouTube tech invokes its superclass init() before setting up its features object to indicate whether it needs manual progress and time updates. So, manual time tracking is enabled and then the YouTube tech does not call the superclass dispose() to clean them up when its disposed.

[apptv]

@dmlap: Thank you for the quick and deep analysis.

It's totally clear to me that putting instance data into the prototype is not a solution. It was only intended to provide you a hint and a more complete bug report.

Sure the youtube tech has to fix this issue, but nevertheless I think there should be a kind of exception handling inside of the MediaTechController in case the vdata node is not existing anymore for what reason ever. In that case the interval should be cleared avoiding infinite loops of exceptions.

[dmlap]

Sorry, I didn't mean to seem dismissive of the workaround. I was in "report the facts"-mode :) I think you have a point about the error behavior when vdata is missing. If it's safe to automatically clean them up when we discover they're gone, I'd be all for it. Can you think of a reason that wouldn't work, @heff?

[heff]

Once the youtube issue is fixed, what other specific cases might trigger that error? I don't love the idea of catching unknown errors, which would hide them from us an potentially make it harder to debug things. Assuming that's what's being described here.

[apptv]

In case you remove the player with dom operation like a one page app.

$('#player').remove();

The timer keeps running forever leaking cpu and memory which is especially bad for mobile devices.

I totally agree that it should not be hidden. A warning message in the console indicating that something has gone wrong disposing the player should be sufficient and then stop the timer not leaking cpu and memory.

[heff]

That makes sense, but sounds like a slightly different scenario. Could we set up a reduced test case to reproduce that (jsbin)? The original issue has to do with the element not existing anymore, but if you just do $.remove the player instance will still exist and should still have a reference to the element, preventing the browser from cleaning it up. So there might be something different we have to do to fix the issue of the timers still running after removing the player el.

Otherwise, for the vdata error, I'm wondering if there's another we can know that the player's been destroyed before we try to trigger the timeupdate. We could probably just check that the player el still exists.

[apptv]

Yes you are right. I think these are 2 seperate issues.

• vdata node does not exist any more
• Player el does not exists any more

The fix proposed in
videojs/videojs-youtube#189
for the first issue does not solve the problem because the MediaTech constructor function is called before the Youtube tech constructor, thus ignoring the settings.

this.features['timeupdateEvents'] = true;

has no effect, even if set on the very first line.

My feeling is that the timers are still in an experimental stage and therefore should be disabled by default.
Doing this really solved my problems and yes also the performance on mobile gets back to normal.

Are you sure you need a event triggered 4 times a sec?

[heff]

For the order of contractor triggering, I think #1412 might address that.

Are you sure you need a event triggered 4 times a sec?

As far as timeupdates go, 250ms is actually conservative. Some browsers trigger them every 15ms.

[mynameisstephen]

This seems to still be an issue, using the current master of video.js & youtube tech.

The currentTimeInterval is still being set despite manualTimeUpdates being false.

Although not getting to the root of the issue, we should ditch the two boolean flags manualTimeUpdates & manualProgress and just set the two Interval IDs to NaN in the constructor and clear them in dispose if isNaN returns false. We won't get out of synce state then.

[heff]

I'm not sure what the next steps should be on this issue. If we can set up a reduced test case I think it would help clear things up.

[jpfranco]

Hey guys, I'm experiencing this as well and I think I got the reduced test case. Please take a look at this fiddle:

http://jsfiddle.net/njhyaypw/

The trick here is that you need to seek before disposing the player, that's how the error occurs. So play the video, seek to some random time and then click the 'destroy' button, you'll see the errors in the console. Tested on latest Chrome. Hope this helps.

[mmcc]

Very interesting. Thanks for the reduced test case, @jpfranco, this is helpful.

[mmcc]

I've got a pretty simple fix for this that should be ready to go out soon (when I can get the tests to play nice).

[nery]

Is there a eta on this fix getting merged?

[mmcc]

@nery We fixed this issue specifically and improved timeout handling in general with the 4.11 release.