videojs / videojs-contrib-hls

HLS library for video.js
http://videojs.github.io/videojs-contrib-hls/
Other
2.84k stars 793 forks source link

timeupdate event is not triggered while a video is paused #160

Closed chikathreesix closed 8 years ago

chikathreesix commented 10 years ago

Because the current time is set in drainBuffer and it doesn't trigger timeupdate event, this event is not triggered when a video is paused. If the video is playing, the callback of setInterval triggers timeupdate every 250 millisecond so it won't be a big problem.

  if (segment.discontinuity) {
    if (event.type !== 'waiting') {
      return;
    }
    this.sourceBuffer.abort();
    // tell the SWF where playback is continuing in the stitched timeline
    this.el().vjs_setProperty('currentTime', segmentOffset * 0.001);
  }

Therefore I would like to trigger timeupdate right after vjs_setProperty is called, however, it will be an infinite loop because drainBuffer is one of a callback for timeupdate.

So I came up with an idea below. Like manualTimeupdatesOn in the original video.js, implement setInterval to trigger timeupdate in videojs-hls and call all callbacks including drainBuffer in the interval. And also set featuresTimeupdateEvents to false to avoid original manualTimeupdatesOn from being called.

videojs.Hls.prototype.trackCurrentTime = function(){
  if (this.currentTimeInterval) { this.stopTrackingCurrentTime(); }
  this.currentTimeInterval = setInterval(vjs.bind(this, function(){
    this.player().trigger('timeupdate');
    this.fillBuffer();
    this.drainBuffer();
  }), 250);
};

Then timeupdate can be triggered right after vjs_setProperty is called in drainBuffer. But I feel this solution is a bit redundant...

Could you tell me your opinion?

gkatsev commented 10 years ago

At first, I would've been inclined to say that we don't want to trigger timeupdate while the video is paused but reading the spec, seems like timeupdate should also be called when currentTime changed in an interesting way, like during a discontinuity, so, we definitely need to fix that.

Given that videojs already has the manual time updates because this is currently using the flash tech, I'd be inclined to not way another setInterval. Instead, we probably do want to just trigger the timeupdate right after setting currentTime in the disconinuity but make sure that fillBuffer and drainBuffer don't react to our own timeupdates.

chikathreesix commented 10 years ago

we probably do want to just trigger the timeupdate right after setting currentTime in the disconinuity but make sure that fillBuffer and drainBuffer don't react to our own timeupdates.

Yes, that is definitely the best way to fix this. I'm currently triggering timeupdate in setCurrentTime but as you mentioned, it makes fillBuffer and drainBuffer to be called twice. Do you have any idea to implement it? Trigger with an event object that has some data which can be considered as a our own timeupdate? Sounds not so good though... I'd like to somehow catch the events at the lower level however there is no lower level so far.

gkatsev commented 10 years ago

I have no thoughts as of yet. Maybe set a flag that fillBuffer and drainBuffer check, though, flags are not great. Maybe remove the handler, trigger the event and reattach the handler. @dmlap any thoughts?

dmlap commented 10 years ago

Using timeupdate to trigger buffer management is starting to look like the wrong mechanism. I think using a simple recurring setTimeout() like @chikathreesix suggests may be the way to go. As a side benefit, it would allow us to fix #133 without doing any gymnastics.

If we go this route, I'd want to de-couple the buffer management from time tracking entirely. That is, just set up polling to trigger fillBuffer/drainBuffer and let the Flash tech do its thing with timeupdate generation. We could still trigger the timeupdate event in drainBuffer() if we wanted to signal interesting timeline events, however.

gkatsev commented 10 years ago

I may have misread the original post but having a timer for fillBuffer/drainBuffer sounds ok to me. I just don't think this tech needs to handle manual timeupdates itself.

dmlap commented 10 years ago

@gkatsev right, we're on the same page on this one.

The quick summary:

chikathreesix commented 10 years ago

My original suggestion was having a timer for manual timeupdates, but having a timer for fillBuffer/drainBuffer sounds much better. Thanks!

dmlap commented 9 years ago

False alarm on this one. fillBuffer() should now get called when the player is paused but we still should fire timeupdates when we cross a discontinuity.

dmlap commented 8 years ago

I think we're all set with this one in 1.x. Thanks for the report!