flowplayer / flowplayer-hlsjs

Flowplayer HLS.js plugin
MIT License
81 stars 35 forks source link

unregister hls.js listeners on unload? #26

Closed phloxic closed 8 years ago

phloxic commented 8 years ago

Should the hls.js events https://github.com/flowplayer/flowplayer-hlsjs/blob/master/flowplayer.hlsjs.js#L229-L240 be off'ed on unload? If yes, is it enough to do api.off for each? Or must this be done for the container as well?

nnarhinen commented 8 years ago

Can you update the line range, it seems the file has changed..

phloxic commented 8 years ago

Sorry - yes, just now. https://github.com/flowplayer/flowplayer-hlsjs/blob/master/flowplayer.hlsjs.js#L352 ff.

nnarhinen commented 8 years ago

So you are referencing to the events registered with hls.on('...') ?

How are you unloading the HLS instance? Does it keep triggering events and loading fragments after flowplayer unload? If yes, then of course the event listeners have to be removed so that Flowplayer does not get any invalid events.

The other story is garbage collection. By registering events we create circular references to instances.

Consider following example:

hls.on('<EVENT>', function() {
  api.on('something', function() {
    if (hls.somethin === 'something') {
      // Do something
    }
  });
});

Here the usage of api and hls in closures prevents the objects from being garbage collected. This is a general problem in browser javascript and hard to avoid.

phloxic commented 8 years ago

The hls instance is destroyed on unloading the engine: https://github.com/flowplayer/flowplayer-hlsjs/blob/master/flowplayer.hlsjs.js#L458-L470 The short version of the registering code would be:

Object.keys(Hls.Events).foreach(function (key) { 
  hls.on(Hls.Events[key], function (etype, data) {
    // some switches depending on key go here
    api.trigger(etype, [api, data]);
  });
});

So the naïve question was whether it makes sense to do the following on engine unload:

Object.keys(Hls.Events).forEach(function (key) {
  api.off(Hls.Events[key]):
});
nnarhinen commented 8 years ago

You are not registering any events with api.on so it does not make sense to unregister them either.

Btw, if you register some events on the flowplayer API object, you want to register them with a namespace so they can be easily unregistered:

// Register N amount of handlers
api.on('load.hlsjs', function() { ... });
api.on('resume.hlsjs', function() { ...});
api.on('cuepoint.hlsjs', function() { ... });

//  Remove all handlers at once
api.off('.hlsjs');
phloxic commented 8 years ago

Ah, right. There are currently 2 events around the poster contortions, which I already meticulously namespaced ;-) https://github.com/flowplayer/flowplayer-hlsjs/blob/master/flowplayer.hlsjs.js#L489 So I'll make use of that nice compact offing. Thanks a lot for your help.