Merge pull request #6356 from jbn/odysee

FIX stop event translation and remove preprocessing

ui/component/viewers/videoViewer/internal/plugins/videojs-recsys/plugin.js

@@ -95,10 +95,10 @@ class RecsysPlugin extends Component {
     this.player = player;
 
     this.recsysEvents = [];
+    this.loadedAt = Date.now();
     this.lastTimeUpdate = null;
     this.currentTimeUpdate = null;
-    this.loadedAt = Date.now();
+    this.inPause = false;
-    this.playInitiated = false;
 
     // Plugin event listeners
     player.on('playing', (event) => this.onPlay(event));
@@ -113,83 +113,13 @@ class RecsysPlugin extends Component {
   }
 
   addRecsysEvent(recsysEvent) {
-    if (!this.playInitiated) {
+    // For now, don't do client-side preprocessing. I think there
-      switch (recsysEvent.event) {
+    // are browser inconsistencies and preprocessing loses too much info.
-        case RecsysData.event.start:
-          this.playInitiated = true;
-          break;
-        case RecsysData.event.scrub:
-          // If playback hasn't started, swallow scrub events. They offer some
-          // information, but if there isn't a subsequent play event, it's
-          // mostly nonsensical.
-          return undefined;
-        case RecsysData.event.stop:
-          // If playback hasn't started, swallow stop events. This means
-          // you're going to start from an offset but the start event
-          // captures that information. (With the ambiguity that you can't
-          // tell if they scrubbed, landed at the offset, or restarted. But
-          // I don't think that matters much.)
-          return undefined;
-        case RecsysData.event.speed:
-          if (this.recsysEvents.length > 0 && this.recsysEvents[0].event === RecsysData.event.speed) {
-            // video.js will sometimes fire the default play speed followed by the
-            // user preference. This is not useful information so we can keep the latter.
-            this.recsysEvents.pop();
-          }
-      }
-    } else {
-      const lastEvent = this.recsysEvents[this.recsysEvents.length - 1];
-
-      switch (recsysEvent.event) {
-        case RecsysData.event.scrub:
-          if (lastEvent.event === RecsysData.event.stop) {
-            // Video.js fires a stop before the seek. This extra information isn't
-            // useful to log though, so this code prunes the stop event if it was
-            // within 0.25 seconds.
-            if (Math.abs(lastEvent.offset - recsysEvent.offset) < 0.25) {
-              this.recsysEvents.pop();
-              recsysEvent.offset = lastEvent.arg;
-            }
-          } else if (lastEvent.event === RecsysData.event.start) {
-            // If the last event was a play and this event is a scrub close to
-            // that play position, I think it's just a weird emit order for
-            // video.js and we don't need to log the scrub.
-            if (Math.abs(lastEvent.offset - recsysEvent.arg) < 0.25) {
-              return undefined;
-            }
-          }
-          break;
-        case RecsysData.event.start:
-          if (lastEvent.event === RecsysData.event.scrub) {
-            // If the last event was a seek and this is a play,
-            // it's reasonable to just implicitly assume the play occurred,
-            // no need to create the play event.
-            return undefined;
-          } else if (lastEvent.event === RecsysData.event.start) {
-            // A start followed by a start is a buffering event.
-            // It may make sense to keep these. A user may abandon
-            // a page *not because it's bad content but because
-            // there are network troubles right now*.
-          }
-          break;
-      }
-    }
-
     this.recsysEvents.push(recsysEvent);
   }
 
   getRecsysEvents() {
-    return this.recsysEvents.map((event) => {
+    return this.recsysEvents;
-      if (event !== RecsysData.event.stop) {
-        return event;
-      }
-
-      // I used the arg in stop events to smuggle the seek time into
-      // the scrub events. But the backend doesn't expect it.
-      const dup = { ...event };
-      delete dup.arg;
-      return dup;
-    });
   }
 
   sendRecsysEvents() {
@@ -207,16 +137,17 @@ class RecsysPlugin extends Component {
     const recsysEvent = newRecsysEvent(RecsysData.event.start, this.player.currentTime());
     this.log('onPlay', recsysEvent);
     this.addRecsysEvent(recsysEvent);
+
+    this.inPause = false;
+    this.lastTimeUpdate = recsysEvent.offset;
   }
 
   onPause(event) {
-    // The API doesn't want an `arg` for `STOP` events. However, video.js
+    const recsysEvent = newRecsysEvent(RecsysData.event.stop, this.player.currentTime());
-    // emits these before the seek events, and that seems to be the easiest
-    // way to lift time you are seeking from into the scrub record (via lastTimeUpdate).
-    // Hacky, but works.
-    const recsysEvent = newRecsysEvent(RecsysData.event.stop, this.player.currentTime(), this.lastTimeUpdate);
     this.log('onPause', recsysEvent);
     this.addRecsysEvent(recsysEvent);
+
+    this.inPause = true;
   }
 
   onEnded(event) {
@@ -226,26 +157,50 @@ class RecsysPlugin extends Component {
   }
 
   onRateChange(event) {
-    // This is actually a bug. The offset should be the offset. The change speed should be change speed.
-    // Otherise, you don't know where it changed and the time calc is wrong.
     const recsysEvent = newRecsysEvent(RecsysData.event.speed, this.player.currentTime(), this.player.playbackRate());
     this.log('onRateChange', recsysEvent);
     this.addRecsysEvent(recsysEvent);
   }
 
   onTimeUpdate(event) {
-    this.lastTimeUpdate = this.currentTimeUpdate;
+    const nextCurrentTime = this.player.currentTime();
-    this.currentTimeUpdate = this.player.currentTime();
+
+    if (!this.inPause && Math.abs(this.lastTimeUpdate - nextCurrentTime) < 0.5) {
+      // Don't update lastTimeUpdate if we are in a pause segment.
+      //
+      // However, if we aren't in a pause and the time jumped
+      // the onTimeUpdate event probably fired before the pause and seek.
+      // Don't update in that case, either.
+      this.lastTimeUpdate = this.currentTimeUpdate;
+    }
+
+    this.currentTimeUpdate = nextCurrentTime;
   }
 
   onSeeked(event) {
-    // The problem? `lastTimeUpdate` is wrong.
+    const curTime = this.player.currentTime();
-    // So every seeks l
 
-    // If the immediately prior event is a pause?
+    // There are three patterns for seeking:
-    const recsysEvent = newRecsysEvent(RecsysData.event.scrub, this.lastTimeUpdate, this.player.currentTime());
+    //
-    this.log('onSeeked', recsysEvent);
+    // Assuming the video is playing,
-    this.addRecsysEvent(recsysEvent);
+    //
+    // 1. Dragging the player head emits: onPause -> onSeeked -> onSeeked -> ... -> onPlay
+    // 2. Key press left right emits: onSeeked -> onPlay
+    // 3. Clicking a position emits: onPause -> onSeeked -> onPlay
+    //
+    // If the video is NOT playing,
+    //
+    // 1. Dragging the player head emits: onSeeked
+    // 2. Key press left right emits: onSeeked
+    // 3. Clicking a position emits: onSeeked
+    const fromTime = this.lastTimeUpdate;
+
+    if (fromTime !== curTime) {
+      // This removes duplicates that aren't useful.
+      const recsysEvent = newRecsysEvent(RecsysData.event.scrub, fromTime, curTime);
+      this.log('onSeeked', recsysEvent);
+      this.addRecsysEvent(recsysEvent);
+    }
   }
 
   onDispose(event) {