From 307322cc487a7f1d63a8b1e543139a00ca4bacfe Mon Sep 17 00:00:00 2001 From: John B Nelson Date: Wed, 30 Jun 2021 11:02:47 -0700 Subject: [PATCH] FIX stop event translation and remove preprocessing I think different browsers behave in different ways for the media API. As a result, I think I was losing information for browsers that weren't the same as mine (Chromium). For now, preprocessing is removed. In the future, I'll add it again (better storage and transmission properties). --- .../internal/plugins/videojs-recsys/plugin.js | 135 ++++++------------ 1 file changed, 45 insertions(+), 90 deletions(-) diff --git a/ui/component/viewers/videoViewer/internal/plugins/videojs-recsys/plugin.js b/ui/component/viewers/videoViewer/internal/plugins/videojs-recsys/plugin.js index 4dad695e9..aa2d5f82d 100644 --- a/ui/component/viewers/videoViewer/internal/plugins/videojs-recsys/plugin.js +++ b/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.playInitiated = false; + this.inPause = false; // Plugin event listeners player.on('playing', (event) => this.onPlay(event)); @@ -113,83 +113,13 @@ class RecsysPlugin extends Component { } addRecsysEvent(recsysEvent) { - if (!this.playInitiated) { - switch (recsysEvent.event) { - 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; - } - } - + // For now, don't do client-side preprocessing. I think there + // are browser inconsistencies and preprocessing loses too much info. this.recsysEvents.push(recsysEvent); } getRecsysEvents() { - return this.recsysEvents.map((event) => { - 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; - }); + return this.recsysEvents; } 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 - // 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); + const recsysEvent = newRecsysEvent(RecsysData.event.stop, this.player.currentTime()); 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; - this.currentTimeUpdate = this.player.currentTime(); + const nextCurrentTime = 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. - // So every seeks l + const curTime = this.player.currentTime(); - // If the immediately prior event is a pause? - const recsysEvent = newRecsysEvent(RecsysData.event.scrub, this.lastTimeUpdate, this.player.currentTime()); - this.log('onSeeked', recsysEvent); - this.addRecsysEvent(recsysEvent); + // There are three patterns for seeking: + // + // Assuming the video is playing, + // + // 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) {