From edf84da5ab9435283c0bb11d0f5d4024f542836e Mon Sep 17 00:00:00 2001 From: Chris Vickery Date: Wed, 11 Sep 2019 13:36:15 -0700 Subject: [PATCH] Refactor graph data flow, re-add subsection looping and seeking (#19) * Close web workers when they're done * Have graphs generate their own data, insert new data better * Replace all messages in graphs to handle out of order signals * Fix bugs, add video looping * Fix looping a bit, optimizations * Fix test cases * Remove debug statements * Make jest comma package workaround more generic --- .babelrc | 17 ++- package.json | 7 +- src/CanExplorer.js | 10 +- src/__tests__/components/PartSelector.test.js | 2 +- src/__tests__/components/RouteSeeker.test.js | 2 +- .../components/RouteVideoSync.test.js | 3 + src/components/CanGraph.js | 144 ++++++++++++------ src/components/CanGraphList.js | 2 - src/components/Explorer.js | 110 +++---------- src/components/HLS.js | 1 + src/components/RouteSeeker/RouteSeeker.js | 27 +++- src/components/RouteVideoSync.js | 62 ++++++-- src/config.js | 2 +- src/models/graph-data.js | 1 + src/workers/rlog-downloader.worker.js | 9 +- yarn.lock | 8 +- 16 files changed, 224 insertions(+), 183 deletions(-) diff --git a/.babelrc b/.babelrc index 32c6764..e3c9316 100644 --- a/.babelrc +++ b/.babelrc @@ -7,10 +7,23 @@ "targets": { "browsers": ["last 2 versions", "safari >= 7"] }, - useBuiltIns: "usage" + "useBuiltIns": "usage" } ], "stage-0" ], - "plugins": ["emotion"] + "plugins": ["emotion"], + "env": { + "test": { + "presets": [ + [ + "env", + { + "modules": false + } + ], + "stage-0" + ] + } + } } diff --git a/package.json b/package.json index 7bc663f..1334726 100644 --- a/package.json +++ b/package.json @@ -4,7 +4,7 @@ "private": true, "homepage": "https://community.comma.ai/cabana", "dependencies": { - "@commaai/comma-api": "^1.1.2", + "@commaai/comma-api": "^1.1.4", "@commaai/hls.js": "^0.12.2", "@commaai/log_reader": "^0.3.1", "@commaai/my-comma-auth": "^1.1.0", @@ -90,5 +90,10 @@ "*.{graphql,gql}": ["prettier --parser graphql --write", "git add"], "*.{md,markdown}": ["prettier --parser markdown --write", "git add"], "*.scss": ["prettier --parser postcss --write", "git add"] + }, + "jest": { + "moduleNameMapper": { + "^@commaai/(.*)$": "/node_modules/@commaai/$1/dist/index.js" + } } } diff --git a/src/CanExplorer.js b/src/CanExplorer.js index acab732..f9470b3 100644 --- a/src/CanExplorer.js +++ b/src/CanExplorer.js @@ -673,8 +673,14 @@ export default class CanExplorer extends Component { currentParts = [minPart, maxPart]; currentPart = part; - // update state then load new parts - this.setState({ currentParts, currentPart }, this.partChangeDebounced); + if ( + currentPart !== this.state.currentPart || + currentParts[0] !== this.state.currentParts[0] || + currentParts[1] !== this.state.currentParts[1] + ) { + // update state then load new parts + this.setState({ currentParts, currentPart }, this.partChangeDebounced); + } } showEditMessageModal(msgKey) { diff --git a/src/__tests__/components/PartSelector.test.js b/src/__tests__/components/PartSelector.test.js index bc1776e..7cd5a9d 100644 --- a/src/__tests__/components/PartSelector.test.js +++ b/src/__tests__/components/PartSelector.test.js @@ -6,7 +6,7 @@ import { shallow, mount, render } from "enzyme"; test("PartSelector successfully mounts with minimal default props", () => { const component = shallow( - {}} partsCount={0} /> + {}} partsCount={0} selectedPart={0} /> ); expect(component.exists()).toBe(true); }); diff --git a/src/__tests__/components/RouteSeeker.test.js b/src/__tests__/components/RouteSeeker.test.js index a10a1d9..aa624aa 100644 --- a/src/__tests__/components/RouteSeeker.test.js +++ b/src/__tests__/components/RouteSeeker.test.js @@ -9,7 +9,7 @@ test("RouteSeeker successfully mounts with minimal default props", () => { {}} - secondsLoaded={0} + videoLength={0} segmentIndices={[]} onUserSeek={() => {}} onPlaySeek={() => {}} diff --git a/src/__tests__/components/RouteVideoSync.test.js b/src/__tests__/components/RouteVideoSync.test.js index 11c240a..c745265 100644 --- a/src/__tests__/components/RouteVideoSync.test.js +++ b/src/__tests__/components/RouteVideoSync.test.js @@ -1,5 +1,6 @@ global.__JEST__ = 1; +import API from "@commaai/comma-api"; import RouteVideoSync from "../../components/RouteVideoSync"; import React from "react"; import { shallow, mount, render } from "enzyme"; @@ -20,9 +21,11 @@ test("RouteVideoSync successfully mounts with minimal default props", () => { message={null} secondsLoaded={0} startOffset={0} + segment={[]} seekIndex={0} userSeekIndex={0} playing={false} + playSpeed={1} url={"http://comma.ai"} canFrameOffset={0} firstCanTime={0} diff --git a/src/components/CanGraph.js b/src/components/CanGraph.js index 0a67dcc..ded0e3e 100644 --- a/src/components/CanGraph.js +++ b/src/components/CanGraph.js @@ -4,7 +4,9 @@ import PropTypes from "prop-types"; import cx from "classnames"; import Signal from "../models/can/signal"; +import GraphData from "../models/graph-data"; import CanPlot from "../vega/CanPlot"; +import debounce from "../utils/debounce"; const DefaultPlotInnerStyle = { position: "absolute", @@ -16,7 +18,7 @@ export default class CanGraph extends Component { static emptyTable = []; static propTypes = { - data: PropTypes.object, + plottedSignal: PropTypes.string, messages: PropTypes.object, messageId: PropTypes.string, messageName: PropTypes.string, @@ -43,7 +45,8 @@ export default class CanGraph extends Component { shiftX: 0, shiftY: 0, bounds: null, - isDataInserted: false + isDataInserted: false, + data: this.getGraphData(props) }; this.onNewView = this.onNewView.bind(this); this.onSignalClickTime = this.onSignalClickTime.bind(this); @@ -52,6 +55,40 @@ export default class CanGraph extends Component { this.onDragAnchorMouseUp = this.onDragAnchorMouseUp.bind(this); this.onDragStart = this.onDragStart.bind(this); this.onPlotResize = this.onPlotResize.bind(this); + this.insertData = this.insertData.bind(this); + } + + getGraphData(props) { + let firstRelTime = -1; + let lastRelTime = -1; + let series = props.plottedSignals + .map(signals => { + const { messageId, signalUid } = signals; + let entries = props.messages[messageId].entries; + if (entries.length) { + let messageRelTime = entries[0].relTime; + if (firstRelTime === -1) { + firstRelTime = messageRelTime; + } else { + firstRelTime = Math.min(firstRelTime, messageRelTime); + } + messageRelTime = entries[entries.length - 1].relTime; + lastRelTime = Math.max(lastRelTime, messageRelTime); + } + return GraphData._calcGraphData( + props.messages[messageId], + signalUid, + 0 + ); + }) + .reduce((m, v) => m.concat(v), []); + + return { + updated: Date.now(), + series, + firstRelTime, + lastRelTime + }; } segmentIsNew(newSegment) { @@ -61,14 +98,6 @@ export default class CanGraph extends Component { ); } - dataChanged(prevProps, nextProps) { - return ( - nextProps.data.series.length !== prevProps.data.series.length || - !prevProps.signalSpec.equals(nextProps.signalSpec) || - nextProps.data.updated !== this.props.data.updated - ); - } - visualChanged(prevProps, nextProps) { return ( prevProps.canReceiveGraphDrop !== nextProps.canReceiveGraphDrop || @@ -79,7 +108,6 @@ export default class CanGraph extends Component { onPlotResize({ bounds }) { this.setState({ bounds }); - this.view.run(); this.view.signal("width", bounds.width - 70); this.view.signal("height", 0.4 * (bounds.width - 70)); // 5:2 aspect ratio this.view.run(); @@ -87,48 +115,51 @@ export default class CanGraph extends Component { shouldComponentUpdate(nextProps, nextState) { if (this.view) { - // only update if segment is new - let segmentChanged = false; - if (this.segmentIsNew(nextProps.segment)) { - if (nextProps.segment.length > 0) { - // Set segmented domain - this.view.signal("segment", nextProps.segment); - } else { - // Reset segment to full domain - this.view.signal("segment", 0); + this.view.runAfter(() => { + // only update if segment is new + let segmentChanged = false; + if (this.segmentIsNew(nextProps.segment)) { + if (nextProps.segment.length > 0) { + // Set segmented domain + this.view.signal("segment", nextProps.segment); + } else { + // Reset segment to full domain + this.view.signal("segment", 0); + } + segmentChanged = true; } - segmentChanged = true; - } - if (!nextProps.live && nextProps.currentTime !== this.props.currentTime) { - this.view.signal("videoTime", nextProps.currentTime); - segmentChanged = true; - } + if ( + !nextProps.live && + nextProps.currentTime !== this.props.currentTime + ) { + this.view.signal("videoTime", nextProps.currentTime); + segmentChanged = true; + } - if (segmentChanged) { - this.view.run(); - } + if (segmentChanged) { + this.view.runAsync(); + } + }); + + return false; } - const dataChanged = this.dataChanged(this.props, nextProps); - - return ( - dataChanged || - JSON.stringify(this.state) !== JSON.stringify(nextState) || - this.visualChanged(this.props, nextProps) - ); + return true; } - insertData() { - this.view.remove("table", () => true).run(); - this.view.insert("table", this.props.data.series).run(); - } + insertData = debounce(() => { + let { series } = this.state.data; - componentDidUpdate(prevProps, prevState) { - if (this.dataChanged(prevProps, this.props)) { - this.insertData(); - } - } + // adding plot points by diff isn't faster since it basically has to be n^2 + // out-of-order events make it so that you can't just check the bounds + let changeset = this.view + .changeset() + .remove(v => true) + .insert(series); + this.view.change("table", changeset); + this.view.run(); + }, 250); componentWillReceiveProps(nextProps) { if ( @@ -139,6 +170,21 @@ export default class CanGraph extends Component { } else if (!nextProps.dragPos && this.state.plotInnerStyle !== null) { this.setState({ plotInnerStyle: null }); } + if ( + this.props.messages !== nextProps.messages || + this.props.plottedSignal !== nextProps.plottedSignal + ) { + let data = this.getGraphData(nextProps); + if ( + data.series.length === this.state.data.series.length && + data.firstRelTime === this.state.data.firstRelTime && + data.lastRelTime === this.state.data.lastRelTime + ) { + // do nothing, the data didn't *actually* change + } else { + this.setState({ data }, this.insertData); + } + } } updateStyleFromDragPos({ left, top }) { @@ -177,9 +223,8 @@ export default class CanGraph extends Component { this.view.runAfter(() => { const state = this.view.getState(); state.subcontext[0].signals.brush = 0; - this.view.setState(state).runAfter(() => { - this.insertData(); - }); + this.view.setState(state); + this.insertData(); }); } @@ -292,8 +337,7 @@ export default class CanGraph extends Component { className="cabana-explorer-visuals-plot-container" > plot.filter(({ messageId, signalUid }, index) => { - const messageExists = - Object.keys(nextProps.messages).indexOf(messageId) !== -1; + const messageExists = !!nextProps.messages[messageId]; let signalExists = true; - if (!messageExists) { - graphData.splice(index, 1); - } else { + if (messageExists) { signalExists = Object.values( nextProps.messages[messageId].frame.signals ).some(signal => signal.uid === signalUid); - - if (!signalExists) { - graphData[index].series = graphData[index].series.filter( - entry => entry.signalUid !== signalUid - ); - } } return messageExists && signalExists; }) ) .filter(plot => plot.length > 0); - this.setState({ plottedSignals, graphData }); + + this.setState({ plottedSignals }); if ( nextProps.selectedMessage && @@ -228,15 +220,6 @@ export default class Explorer extends Component { } } } - - if (partsDidChange) { - const { userSeekTime } = this.state; - const nextSeekTime = - userSeekTime - - this.props.currentParts[0] * 60 + - nextProps.currentParts[0] * 60; - this.setState({ userSeekTime: nextSeekTime }); - } } changePlaySpeed(value) { @@ -356,18 +339,10 @@ export default class Explorer extends Component { const { segment, segmentIndices } = this.state; const { messages, selectedMessage } = this.props; if (segment.length > 0 || segmentIndices.length > 0) { - let userSeekTime = 0; - if ( - messages[selectedMessage] && - messages[selectedMessage].entries.length > 0 - ) { - userSeekTime = messages[selectedMessage].entries[0].relTime; - } this.setState({ segment: [], segmentIndices: [], - userSeekIndex: 0, - userSeekTime + userSeekIndex: 0 }); } } @@ -387,8 +362,13 @@ export default class Explorer extends Component { if (entries.length === 0) return null; const { segmentIndices } = this.state; - if (segmentIndices.length === 2) { - for (let i = segmentIndices[0]; i <= segmentIndices[1]; i++) { + if (segmentIndices.length === 2 && segmentIndices[0] >= 0) { + for ( + let i = segmentIndices[0], + l = Math.min(entries.length - 1, segmentIndices[1]); + i <= l; + i++ + ) { if (entries[i].relTime >= time) { return i; } @@ -407,22 +387,9 @@ export default class Explorer extends Component { onUserSeek(time) { this.setState({ userSeekTime: time }); const message = this.props.messages[this.props.selectedMessage]; - if (!message) { - this.props.onUserSeek(time); - return; - } - const { entries } = message; - const userSeekIndex = this.indexFromSeekTime(time); - if (userSeekIndex) { - const seekTime = entries[userSeekIndex].relTime; - - this.setState({ userSeekIndex, userSeekTime: seekTime }); - this.props.onSeek(userSeekIndex, seekTime); - } else { - this.props.onUserSeek(time); - this.setState({ userSeekTime: time }); - } + this.props.onUserSeek(time); + this.setState({ userSeekTime: time }); } onPlaySeek(time) { @@ -439,20 +406,7 @@ export default class Explorer extends Component { } onGraphTimeClick(messageId, time) { - const canTime = time + this.props.firstCanTime; - - const { entries } = this.props.messages[messageId]; - if (entries.length) { - const userSeekIndex = Entries.findTimeIndex(entries, canTime); - - this.props.onUserSeek(time); - this.setState({ - userSeekIndex, - userSeekTime: time - }); - } else { - this.setState({ userSeekTime: time }); - } + this.onUserSeek(time); } onPlay() { @@ -471,34 +425,6 @@ export default class Explorer extends Component { return this.props.partsCount * 60; } - startOffset() { - return 0; - const partOffset = this.props.currentParts[0] * 60; - const message = this.props.messages[this.props.selectedMessage]; - if (!message || message.entries.length === 0) { - return partOffset; - } - - const { entries } = message; - const { segment } = this.state; - let startTime; - if (segment.length === 2) { - startTime = segment[0]; - } else { - startTime = entries[0].relTime; - } - - if ( - startTime > partOffset && - startTime < (this.props.currentParts[1] + 1) * 60 - ) { - // startTime is within bounds of currently selected parts - return startTime; - } else { - return partOffset; - } - } - onVideoClick() { const playing = !this.state.playing; this.setState({ playing }); @@ -651,8 +577,7 @@ export default class Explorer extends Component {
= 1) { + if (newRatio >= 1 || newRatio < 0) { newRatio = 0; - currentTime = 0; + currentTime = startTime; this.props.onUserSeek(newRatio); } @@ -236,3 +243,7 @@ export default class RouteSeeker extends Component { ); } } + +function roundTime(time) { + return Math.round(time * 1000) / 1000; +} diff --git a/src/components/RouteVideoSync.js b/src/components/RouteVideoSync.js index ab0b9cd..3bda7e0 100644 --- a/src/components/RouteVideoSync.js +++ b/src/components/RouteVideoSync.js @@ -46,8 +46,7 @@ const Styles = StyleSheet.create({ export default class RouteVideoSync extends Component { static propTypes = { userSeekIndex: PropTypes.number.isRequired, - secondsLoaded: PropTypes.number.isRequired, - startOffset: PropTypes.number.isRequired, + segment: PropTypes.array.isRequired, message: PropTypes.object, canFrameOffset: PropTypes.number.isRequired, url: PropTypes.string.isRequired, @@ -56,7 +55,8 @@ export default class RouteVideoSync extends Component { onUserSeek: PropTypes.func.isRequired, onPlay: PropTypes.func.isRequired, onPause: PropTypes.func.isRequired, - userSeekTime: PropTypes.number.isRequired + userSeekTime: PropTypes.number.isRequired, + playSpeed: PropTypes.number.isRequired }; constructor(props) { @@ -87,6 +87,14 @@ export default class RouteVideoSync extends Component { ) { this.setState({ shouldRestartHls: true }); } + if ( + nextProps.userSeekTime && + this.props.userSeekTime !== nextProps.userSeekTime + ) { + if (this.state.videoElement) { + this.state.videoElement.currentTime = nextProps.userSeekTime; + } + } } nearestFrameUrl() { @@ -121,20 +129,40 @@ export default class RouteVideoSync extends Component { }); } - segmentProgress(currentTime) { - // returns progress as number in [0,1] - - if (currentTime < this.props.startOffset) { - currentTime = this.props.startOffset; + videoLength() { + if (this.props.segment.length) { + return this.props.segment[1] - this.props.segment[0]; } - const ratio = - (currentTime - this.props.startOffset) / this.props.secondsLoaded; + if (this.state.videoElement) { + return this.state.videoElement.duration; + } + + return 0; + } + + startTime() { + if (this.props.segment.length) { + return this.props.segment[0]; + } + + return 0; + } + + segmentProgress(currentTime) { + // returns progress as number in [0,1] + let startTime = this.startTime(); + + if (currentTime < startTime) { + currentTime = startTime; + } + + const ratio = (currentTime - startTime) / this.videoLength(); return Math.max(0, Math.min(1, ratio)); } ratioTime(ratio) { - return ratio * this.props.secondsLoaded + this.props.startOffset; + return ratio * this.videoLength() + this.startTime(); } onVideoElementAvailable(videoElement) { @@ -145,7 +173,11 @@ export default class RouteVideoSync extends Component { /* ratio in [0,1] */ let { videoElement } = this.state; - let seekTime = videoElement.duration * ratio; + if (isNaN(videoElement.duration)) { + this.setState({ shouldRestartHls: true }, funcSeekToRatio); + return; + } + let seekTime = this.ratioTime(ratio); videoElement.currentTime = seekTime; const funcSeekToRatio = () => this.props.onUserSeek(seekTime); @@ -177,7 +209,8 @@ export default class RouteVideoSync extends Component { this.props.url, process.env.REACT_APP_VIDEO_CDN ).getRearCameraStreamIndexUrl()} - startTime={this.props.userSeekTime} + startTime={this.startTime()} + videoLength={this.videoLength()} playbackSpeed={this.props.playSpeed} onVideoElementAvailable={this.onVideoElementAvailable} playing={this.props.playing} @@ -194,7 +227,8 @@ export default class RouteVideoSync extends Component { className={css(Styles.seekBar)} nearestFrameTime={this.props.userSeekTime} segmentProgress={this.segmentProgress} - secondsLoaded={this.props.secondsLoaded} + startTime={this.startTime()} + videoLength={this.videoLength()} segmentIndices={this.props.segmentIndices} onUserSeek={this.onUserSeek} onPlaySeek={this.props.onPlaySeek} diff --git a/src/config.js b/src/config.js index e2b522a..286b623 100644 --- a/src/config.js +++ b/src/config.js @@ -24,7 +24,7 @@ export const LOGENTRIES_TOKEN = "4bc98019-8277-4fe0-867c-ed21ea843cc5"; export const PART_SEGMENT_LENGTH = 3; -export const CAN_GRAPH_MAX_POINTS = 10000; +export const CAN_GRAPH_MAX_POINTS = 5000; export const STREAMING_WINDOW = 60; diff --git a/src/models/graph-data.js b/src/models/graph-data.js index 545c2b9..4468529 100644 --- a/src/models/graph-data.js +++ b/src/models/graph-data.js @@ -23,6 +23,7 @@ function _calcGraphData(msg, signalUid, firstCanTime) { // Always include last message entry, which faciliates graphData comparison samples.push(msg.entries[msg.entries.length - 1]); } + // sorting these doesn't fix the phantom lines return samples .filter(e => e.signals[signal.name] !== undefined) .map(entry => { diff --git a/src/workers/rlog-downloader.worker.js b/src/workers/rlog-downloader.worker.js index 8c7d678..eefb531 100644 --- a/src/workers/rlog-downloader.worker.js +++ b/src/workers/rlog-downloader.worker.js @@ -56,15 +56,16 @@ function sendBatch(entry) { ); }); - if (entry.ended) { - console.log("Sending finished"); - } - self.postMessage({ newMessages: messages, maxByteStateChangeCount, isFinished: entry.ended }); + + if (entry.ended) { + console.log("Sending finished"); + close(); + } } async function loadData(entry) { diff --git a/yarn.lock b/yarn.lock index 4aef152..f13c173 100644 --- a/yarn.lock +++ b/yarn.lock @@ -27,10 +27,10 @@ joi-browser "^13.4.0" querystringify "^2.1.1" -"@commaai/comma-api@^1.1.2": - version "1.1.2" - resolved "https://registry.yarnpkg.com/@commaai/comma-api/-/comma-api-1.1.2.tgz#2abf967b708f2d650c8e9cb0eecd5044a3a39da2" - integrity sha512-Pp19FnHSimzkBlbJCgPObklZc2VZvtYrHVQoCcfbsZMYZaOEFolHek4lZ0l2XkCNB8n71QleBbK6xGY56/jb+Q== +"@commaai/comma-api@^1.1.4": + version "1.1.4" + resolved "https://registry.yarnpkg.com/@commaai/comma-api/-/comma-api-1.1.4.tgz#75f727bfa43f6f446a341d1f9154f15334cfb79e" + integrity sha512-S25QY2OwO1aO5HKtvJQUnCwDsfBhgj7+SANZG99kHvxfFxOwWzT/GcV7G/xEtEJ06OsJ+Ih8XoUFm7IxNc1bwA== dependencies: babel-runtime "^6.26.0" config-request "^0.5.1"