Is it possible to use this plugin having multiple videos on the page?

[nicu-chiciuc]

Description

It seems that because when using the offset plugin, the Player prototype is overriden, it is not possible to have multiple videos on the page that use the plugin with the same version of VideoJs. In order to fix this use case I had to fork the project and add the new functions directly on the player object. For example:

Player.prototype.currentTime = function(seconds) {
      if (seconds !== undefined) {
        return Player.__super__.currentTime
          .call(this, seconds + this._offsetStart) - this._offsetStart;
      }
      return Player.__super__.currentTime
        .apply(this, arguments) - this._offsetStart;
    };

was transformed to:

this.currentTime = function(seconds) {
    if (seconds !== undefined) {
      return (
        Player.prototype.currentTime.call(this, seconds + this._offsetStart) -
        this._offsetStart
      );
    }
    return (
      Player.prototype.currentTime.apply(this, arguments) - this._offsetStart
    );
  };

I've made these changes half a year ago and didn't think it was necessary to create a pull request since it seemed that the project is not maintained anymore/is going in a different direction. So my question is: Would such a change be useful or there were specific reasons why the __super__ prototype method was chosen?

It seems like this would solve the problem with having multiple videos on the page using the plugin.

Sorry for the rant and for not respecting the issue standard. I'm currently trying to understand whether I should try to contribute directly to this plugin or if the work should be done in parallel.

[cladera]

Hi @nicu-chiciuc! Thank you so much for sharing this.

The reason I choose to use __super__ is just because I was trying to follow the inherence pattern.

To be honest your use case never came to my mind, which is pretty embarrassing because it makes totally sense.

Feel free to post a PR, I'll check it out and we can discuss the details of the implementation together.

[nicu-chiciuc]

That's great to hear, I'll clean-up me version a little and submit the PS as soon as possible.

[nicu-chiciuc]

I've had to make some small adjustments to the tests but I'm having some problems with the rest time update handler when video reaches the end test. Initially I forked the project from m1ch3lp3r3z/videojs-offset and I liked that onPlayerTimeUpdate was never reset. For me it seems like this would simplify the logic, and also I'm not sure if there is any big performance improvement when disabling the event for some time. Finally my version of onPlayerTimeUpdate is this:

// utils.js
const Epsilon = 0.0001;

export const timeAlmostEqual = (num1, num2) => {
  const almost = Math.abs(num1 - num2) < Epsilon;

  return almost;
};

// plugin.js
const onPlayerTimeUpdate = function() {
  const curr = this.currentTime();

  if (timeAlmostEqual(curr, 0)) {
    return;
  }

  if (curr < 0) {
    this.currentTime(0);
  }

  if (this._offsetEnd > 0 && curr > this._offsetEnd - this._offsetStart) {
    this.trigger('ended');

    if (!this._restartBeginning) {
      this.currentTime(this._offsetEnd - this._offsetStart);
      this.pause();
    } else {
      this.trigger('loadstart');
      this.currentTime(0);
    }
  }
};

In my version I also trigger the pause() just in one single case, since most of the time triggering pause() or play() wasn't necessary. And I also added a function which checks if the currentTime is near 0 since sometimes I was getting numbers which were -2e-7 and the player would sometimes be in loading state for several seconds. But these are details.

Basically my question is whether there is any important reason for which it would be necessary to turn off the 'timeupdate' event?

[cladera]

I think it is, although how much it might affect the performance? Probably not so much.

But you never know in which context/environment this plugin will be used, perhaps the device, the app or whatever is short on resources, and then it could make a difference.

Say someone uses the plugin in a blog app. Post may contain videos and users may or may not play them. Or just play them once, whatever. The case is a video that is not being consumed has a subscription to an event that is fired every ~250 milliseconds, just to realize that it has to do nothing. Even worse, if the blog app is a SAP, every time the user navigates to a new resource I'm not sure the GC will clean those listeners at all, so for each video that was printed in the screen there will be a listener executing every 250ms.

In my opinion, when the plugin is not in use it shouldn't waste any resource at all (or at least that we are aware of). Of course it is just my opinion.