fix: audit src/js and add a frame-accurate export path
Two halves of one pass; TODO-FIXES.md lists every finding. The audit fixed, among ~90 items: MP4 export dead-ending and locking the UI behind a stuck "Downloading..." button; height keyframes storing the width; shadow defaults and keyframes storing undefined because fabric's get() is not a path getter; every letter animation writing to the last letter; keyframe times drifting on each drag because data-time was layer-relative on expanded rows; O(n^2 log n) playback, now indexed once per frame; and a save() that rebuilt the record canvas on every edit, now debounced. Exports also lost audio-layer sound outright: MediaRecorder records only the first audio track, so every source now mixes through one AudioContext into one destination. The new half is src/js/render.js, an offline renderer. Real-time capture was the root cause of dropped frames on heavy scenes and smeared video layers: a bare currentTime assignment is async, so drawing straight after it captures the previous frame. Each frame is now seeked, awaited on 'seeked', drawn, and pushed through a VideoEncoder; audio is mixed in one OfflineAudioContext pass and encoded to Opus. webm-writer2.js gained a second Opus track and lost three bugs, including a MAX_CLUSTER_DURATION_MSEC of ~58 days that overflowed the signed 16-bit block timecode past ~32s. Real-time capture remains the fallback: the hand-rolled muxer's output is decoded in a <video> element before being handed over, and any failure returns null so record() falls back transparently. test/webm-muxer.test.js parses the muxer output with an EBML reader in plain Node. It caught the fixed-256-byte header overflow that inspection missed. Not verified in a browser: node --check passes on every script and the muxer test passes, but a running Chrome held the Playwright profile lock, so the manual pass listed at the end of TODO-FIXES.md is still outstanding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+273
@@ -0,0 +1,273 @@
|
||||
# Bugs & Issues — audit + fixes
|
||||
|
||||
Audit of `src/js`, then all findings fixed. Every file below passes `node --check`.
|
||||
No live browser smoke test was run (a Chrome instance held the Playwright profile
|
||||
lock), so the changes are verified statically only — see **Not verified** at the end.
|
||||
|
||||
Legend: `[x]` fixed · P0 = feature broken · P1 = wrong data written · P2 = crash on edge path · P3 = silently wrong UI · P4 = perf/leak · P5 = cleanup
|
||||
|
||||
---
|
||||
|
||||
## P0 — Broken features
|
||||
|
||||
- [x] **MP4 export dead-ended and locked the UI** — `src/js/functions.js`
|
||||
The mp4 branch of `downloadRecording` was `type = 'video/mp4'`, an implicit global
|
||||
assignment that did nothing: no download, `recording` stuck at `true`, the button
|
||||
stuck on "Downloading...", and `downloadModal()` refusing to reopen until reload.
|
||||
mp4 now goes through `convertStreams(blob, 'mp4')` like gif, and a new shared
|
||||
`resetRecordingUI()` unlocks the editor on every exit path (success, unknown
|
||||
format, worker error, FileReader error, `MediaRecorder` error).
|
||||
|
||||
- [x] **Copying keyframes threw** — `src/js/events.js`
|
||||
`canvas.getActiveObject().isEditing` ran on `null`, because selecting keyframes
|
||||
clears the canvas selection — the normal path. The active object is now resolved
|
||||
once and guarded, and empty `$.grep` results are no longer pushed to the clipboard.
|
||||
|
||||
- [x] **`canvas.getItemByid` typo** — `src/js/functions.js`
|
||||
Lowercase `i`; TypeError when pasting text/`charSpacing` keyframes.
|
||||
|
||||
- [x] **Ctrl+Z crashed on an empty stack; Ctrl+Shift+Z was a no-op** — `src/js/events.js`
|
||||
Merged into one guarded handler: shift picks redo, and each branch checks its own
|
||||
stack length. Previously Ctrl+Shift+Z matched both `if` blocks (redo then undo).
|
||||
|
||||
- [x] **Blur slider threw with nothing selected** — `src/js/events.js`
|
||||
`obj.applyFilters()` was outside the `if (canvas.getActiveObject())` guard. Also
|
||||
renamed the shadowed `x` in the blur/noise/chroma `find()` callbacks.
|
||||
|
||||
## P1 — Wrong values written
|
||||
|
||||
- [x] **`height` keyframes stored the width** — `src/js/functions.js` (3 sites: twice in `keyframeChanges`, once in `crop`)
|
||||
- [x] **`var scaleX = obj, scaleX;`** — `src/js/text.js`; the fabric object was being assigned as a scale factor.
|
||||
- [x] **NaN letter delay + shadowed global `duration`** — `src/js/text.js`
|
||||
`delay = i * duration` read the hoisted local before its own initialiser. Renamed
|
||||
to `step`, computed before use, and `!(delay > 0)` now catches NaN.
|
||||
- [x] **Every letter animation wrote to the last letter** — `src/js/text.js`
|
||||
`index`, `animation`, `start` and `instance` are `let` per iteration instead of `var`.
|
||||
- [x] **Shadow defaults/keyframes stored `undefined`** — `src/js/functions.js`
|
||||
fabric's `get()` is not a path getter. Added `getPropValue()` (handles `shadow.*`)
|
||||
and `setDefaultValue()` / `getDefaultValue()`, and routed ~26 direct
|
||||
`.defaults.find(...).value = …` writes through them — which also creates the entry
|
||||
when a project predates a property instead of throwing.
|
||||
- [x] **WebGL filter backend clobbered with `undefined`** — `src/js/init.js`, `src/js/database.js`
|
||||
Both assignments are now conditional on the backend having constructed.
|
||||
|
||||
## P2 — Crashes on edge paths
|
||||
|
||||
- [x] **`RangeError: Invalid array length`** — `src/js/functions.js`
|
||||
`temparr.length = findIndex(...)` went negative for a stale keyframe reference.
|
||||
`lastKeyframe` / `nextKeyframe` are now index lookups that return `false`.
|
||||
- [x] **`animate()` had no null guards** — `src/js/functions.js`
|
||||
Object and `p_keyframes` lookups are resolved once and guarded in every per-frame
|
||||
loop (`animate`, `recordAnimate`, the playback update loop, `playVideos`,
|
||||
`playAudio`), which also removed dozens of repeated `.find()` calls.
|
||||
- [x] **`getAssets()` recursed synchronously forever** — `src/js/database.js`
|
||||
Retries on a 250 ms timer, capped at 20 attempts, and rebuilds the asset arrays
|
||||
instead of appending duplicates on re-entry after an import.
|
||||
- [x] **`deleteObject` threw and leaked `files`** — `src/js/functions.js`
|
||||
The entry was compared to a string so it never matched; now filtered by `name`.
|
||||
Video elements are also paused and unloaded on delete.
|
||||
- [x] **Unguarded `keyarr[0]` / `.defaults.find(...)`** — `copyKeyframes`, `updateKeyframe`,
|
||||
`applyEasing`, `keyframeProperties`, `removeKeyframe`, `checkAnyKeyframe`.
|
||||
The four repetitive counterpart blocks were replaced by one
|
||||
`KEYFRAME_COUNTERPARTS` map, so a missing counterpart is skipped, not fatal.
|
||||
- [x] **Other unguarded lookups** — `deleteAsset`, `reGroup`, `renderLayer`,
|
||||
`renderProp`, `setDuration`, `setTimelineZoom`, `saveLayerName`, `updateInputs`,
|
||||
`updatePanel`, `updateStrokeValues`, `animateText`, `scrollIntoView` (3 sites),
|
||||
`object:modified` / `object:rotating` / `mouse:out` / `mouse:up` handlers.
|
||||
`importProject` validates the payload before touching `data.project[0]`, and
|
||||
`line_h`/`line_v` go through a new `hideGuides()` helper.
|
||||
|
||||
## P3 — Silently wrong behaviour
|
||||
|
||||
- [x] **`document.onmousedown` permanently disabled** — `src/js/functions.js`
|
||||
`dragTimeline` installed a `return false` handler and never removed it. It now
|
||||
only overrides `onselectstart`, and restores it on mouseup.
|
||||
- [x] **Keyframe time drifted on every drag** — `src/js/functions.js`
|
||||
`data-time` is now always the absolute timeline time (the value every lookup keys
|
||||
off), with the visual offset applied as CSS only. This also fixes keyframes on an
|
||||
*expanded* row, whose `data-time` used to be layer-relative so no lookup matched.
|
||||
`updateKeyframe`'s unused third argument is gone.
|
||||
- [x] **Shift-deselect never removed a keyframe** — `this` inside a `$.grep` callback is not the element.
|
||||
- [x] **`e.shiftDown`** → `e.shiftKey`.
|
||||
- [x] **Snap guide never hid** — the row-local index was compared against the global `.keyframe` count.
|
||||
- [x] **Comparison / typo bugs**
|
||||
`canvas.getActiveObjects.length` → `getActiveObjects().length` ·
|
||||
`strokeDashArray == [10, 5]` → element comparison ·
|
||||
chroma `setValue(distance)` → `distance * 100` · `'#FFFFF'` → `'#FFFFFF'` ·
|
||||
`videoPlayer.videoheight` → `videoHeight` (and both thumbnail helpers no longer
|
||||
draw the canvas onto itself) · `if (start && play && !paused)` in `playAudio`
|
||||
(`play` is the global function, always truthy) · `#redo` gated on `redo.length` ·
|
||||
`:last-child()` → `:last-child`.
|
||||
- [x] **Paste loop closure** — `var imgObj` → `let`, so each thumbnail saves its own file.
|
||||
- [x] **Layers added mid-timeline were shortened** — `end: duration - currenttime` → `duration`
|
||||
(media layers clamp to `min(start + assetDuration, duration)`).
|
||||
- [x] **Malformed HTML** — 5 unterminated `<img …'` strings and a stray `</div>` in `src/js/ui.js`.
|
||||
- [x] **Duplicate DOM ids** — `id="easing"` on both wrapper and `<select>` (the select
|
||||
is now `easing-select`; `#easing select` still matches), `id="filters-title"` ×4 and
|
||||
`id='item-text'` ×6 became classes, with `src/styles.css` updated to match.
|
||||
Added the missing `<meta charset>`.
|
||||
|
||||
## P4 — Performance, leaks, export gaps
|
||||
|
||||
- [x] **`save()` rebuilt the record canvas on every edit** — `src/js/functions.js`
|
||||
`updateRecordCanvas()` + `autoSave()` now run through a 400 ms debounce
|
||||
(`schedulePersist`). `record()` still awaits `updateRecordCanvas()` directly, so an
|
||||
export always captures current state. `updateObjectValues` no longer calls
|
||||
`autoSave()` on every keystroke.
|
||||
- [x] **`async forEach` raced the snapshot** — `src/js/functions.js`, `src/js/database.js`
|
||||
Both filter-stripping loops are sequential `for…of` in `async` functions, so
|
||||
`toJSON` / `toDatalessJSON` can no longer run mid-strip.
|
||||
- [x] **O(n²·log n) playback** — `src/js/functions.js`
|
||||
`buildKeyframeIndex()` groups keyframes by `id|name` once per rendered frame;
|
||||
`lastKeyframe`, `nextKeyframe` and `checkAnyKeyframe` use it instead of sorting a
|
||||
copy of the entire keyframe list per keyframe per frame. The two duplicated inner
|
||||
`nextKeyframe` copies are gone.
|
||||
- [x] **Exports lost audio-layer sound** — `src/js/recorder.js`
|
||||
Audio layers were never routed into the capture stream. All sources (video
|
||||
elements, audio layers, background audio) now mix through **one** AudioContext
|
||||
into **one** destination track — necessary because `MediaRecorder` only records
|
||||
the first audio track. Audio layers start/stop on their `p_keyframes` boundaries.
|
||||
- [x] **Export timing** — `src/js/recorder.js`
|
||||
The render clock starts after `recorder.start()` (it used to run before, losing the
|
||||
first frames), stops when the animation clock covers `duration` rather than on a
|
||||
wall-clock `setTimeout`, and has a `duration + 5s` safety stop.
|
||||
- [x] **Leaks** — AudioContexts are closed and stream tracks stopped on export end;
|
||||
object URLs are revoked (`downloadRecording`, `PostBlob`, `exportProject`,
|
||||
`importProject`); the browser scroll handler is unbound before rebinding; `sortable`
|
||||
is initialised once instead of per rendered layer; deleted videos are paused and
|
||||
unloaded; category/audio grids are cleared before repopulating.
|
||||
|
||||
## P5 — Dead code, config, cleanup
|
||||
|
||||
- [x] **Unreachable code calling an undefined `waitForEvent`** — `src/js/recorder.js`
|
||||
`initRecorder` / `recordFrame` / `exportRecording` and ~180 lines of commented-out
|
||||
abandoned experiments were removed; the file is now just the live export path.
|
||||
- [x] **Placeholder API keys** — `src/js/init.js`
|
||||
`HAS_FONTS_KEY` / `HAS_PIXABAY_KEY` gate the requests. Without a fonts key the
|
||||
pickers fall back to the bundled families instead of staying empty; without a
|
||||
Pixabay key search shows an explanatory message instead of 401-ing.
|
||||
- [x] **`var timeout` collided with `recorder.js`'s `timeout()`** — removed (it was unused).
|
||||
- [x] **Implicit globals** — `srcfreeze`, `osc`, `type`, `newcolorkeyframe` (deleted, never read).
|
||||
- [x] **Duplicate / unreachable branches** — the second `shadow.blur` arm in all three
|
||||
`setValue` copies, the duplicated `objectCaching` key, the three copies of the
|
||||
keyframe sort comparator (now `sortKeyframes()`), and the two identical crop blocks
|
||||
in `events.js` (now `updateCropBounds()`).
|
||||
- [x] **Converter was unfinished** — `src/js/converter.js`
|
||||
The `workerReady` handshake never fired (`buffersReady` was never set); worker
|
||||
readiness is now tracked across calls, since the worker is created once and reused.
|
||||
Added `onerror` handling, an empty-result guard, and a user-visible failure path.
|
||||
- [x] **Misc** — `overlay()` no longer reuses the artboard's `overlay` id ·
|
||||
`getItemById` recurses so nested groups are found and stops at the first hit ·
|
||||
`fabric.Lottie` guards `this.canvas` before the object is added ·
|
||||
`newLottieAnimation` no longer multiplies an ms duration by 1000 ·
|
||||
lottie layers get a colour · `calculateTextWidth` honours the requested font ·
|
||||
`changeFont` / `loadImage` / `loadVideo` / `handleLottieUpload` have failure paths ·
|
||||
`checkFilter` no longer tests for a non-existent `video` fabric type ·
|
||||
`AnimatedText.assignTo` applies `text`/`props` · `animate(currenttime, false)` →
|
||||
`animate(false, currenttime)` (3 sites — text animation changes never refreshed) ·
|
||||
`exportProject` uses a Blob instead of a `data:` URL · `clearProject` reloads after
|
||||
the deletes resolve · `readTextFile` handles blob-URL status 0 and errors ·
|
||||
`.catch` added to the Localbase promises · opacity inputs clamp the input, not the
|
||||
wrapper · arrow-key nudge calls `setCoords()` and is suppressed while typing ·
|
||||
layer reorder shortcuts accept Ctrl as well as Cmd · alignment guides ignore hidden
|
||||
objects · `alignObject` saves and dropped its `console.log`.
|
||||
|
||||
---
|
||||
|
||||
---
|
||||
|
||||
# Frame-accurate export (new)
|
||||
|
||||
The real-time capture path was the root cause of "heavy scenes drop frames" and
|
||||
"video layers are sampled at the wrong moment". It is now the *fallback*, not the
|
||||
default.
|
||||
|
||||
## `src/js/render.js` — offline renderer
|
||||
|
||||
No clock is attached to the render. For each of `duration × 30` frames:
|
||||
|
||||
1. every video layer is seeked to the exact frame time and the code **awaits
|
||||
`seeked`** before drawing (a bare `currentTime =` is async — drawing straight
|
||||
after captures the *previous* frame, which is why real-time exports smeared
|
||||
video layers);
|
||||
2. lottie layers are advanced to the same time;
|
||||
3. `recordAnimate(time)` lays out the frame, `canvasrecord.renderAll()` draws it;
|
||||
4. a `VideoFrame` is built from the canvas with an explicit timestamp and pushed
|
||||
through a `VideoEncoder` (VP9, falling back to VP8), with the encoder queue
|
||||
held at ≤ 8 frames so memory stays bounded.
|
||||
|
||||
Audio is mixed in one `OfflineAudioContext` pass — every audio layer, every video
|
||||
layer's soundtrack and the background track, each placed at its own
|
||||
`start`/`trimstart`/`end` with its own gain — then encoded to Opus with
|
||||
`AudioEncoder`. Sample-accurate, and unlike the live path it does not depend on
|
||||
playback keeping up.
|
||||
|
||||
Both tracks are sorted by timestamp and muxed into one WebM.
|
||||
|
||||
`recordAnimate()` skips `playVideos()` while `offlinerender` is set, so real-time
|
||||
playback cannot fight the explicit seeking.
|
||||
|
||||
## `src/js/webm-writer2.js` — extended
|
||||
|
||||
Was single-track video only, and unreferenced by `index.html`. Now loaded, and:
|
||||
|
||||
- optional second **Opus** track (`A_OPUS` + 19-byte `OpusHead` CodecPrivate,
|
||||
`CodecDelay`, `SeekPreRoll`, `Audio` element with sample rate/channels);
|
||||
- `addFrame(chunk, trackNumber)` accepts `EncodedAudioChunk` as well as
|
||||
`EncodedVideoChunk`; `addFrameToCluster` no longer hardcodes track 1.
|
||||
|
||||
Three bugs fixed in the vendored library along the way:
|
||||
|
||||
- **`MAX_CLUSTER_DURATION_MSEC` was `5000000`** (~58 days), so every frame went
|
||||
into a single cluster and block timecodes — a signed 16-bit offset — overflowed
|
||||
past ~32 s. Now 5000 ms, and clusters close on a video keyframe.
|
||||
- **The header buffer was a fixed 256 bytes**, which a second `TrackEntry`
|
||||
overflows (`ArrayBufferDataStream's pos lies beyond end of buffer`). Now 1024.
|
||||
- **`instanceof Uint8Array`** for byte payloads fails across a realm boundary;
|
||||
`ArrayBuffer.isView` is now accepted too.
|
||||
|
||||
## Safety net
|
||||
|
||||
The muxer is hand-rolled, so `renderFrameAccurate()` loads the finished blob into
|
||||
a `<video>` element and checks it decodes before handing it over. Any failure —
|
||||
no WebCodecs, unsupported codec, encoder error, failed verification — returns
|
||||
`null` and `record()` transparently falls back to the real-time encoder.
|
||||
|
||||
## Tests — `test/webm-muxer.test.js`
|
||||
|
||||
Runs in plain Node (no deps): `node test/webm-muxer.test.js`
|
||||
|
||||
Loads the muxer in a VM context, feeds fake encoded chunks, then parses the
|
||||
resulting bytes with a small EBML reader and asserts the EBML header, Segment,
|
||||
one or two `TrackEntry` with the right `CodecID`/`TrackType`, a 19-byte
|
||||
`OpusHead`, multiple clusters, every block present on the right track, block
|
||||
timecodes inside signed 16-bit range, ascending cluster timecodes, and that
|
||||
`cluster timecode + block timecode` reconstructs the input timestamps to within
|
||||
1 ms. Covers video-only and video+audio at 75 s (13 clusters, 3000 blocks).
|
||||
|
||||
Both cases pass. The 256-byte header bug above was caught by this test, not by
|
||||
inspection.
|
||||
|
||||
---
|
||||
|
||||
## Left in place deliberately
|
||||
|
||||
- **`src/js/encode-worker.js`** is still unreferenced. It was the old
|
||||
File-System-Access-API sketch; `render.js` supersedes it and buffers to memory
|
||||
instead of requiring a save-file picker. Deleting it is a product decision.
|
||||
- **MP4 and GIF** still go through the ancient asm.js ffmpeg worker
|
||||
(`converter.js`, loaded from archive.org). The frame-accurate renderer feeds it
|
||||
a better WebM, but that dependency is unchanged and may be offline.
|
||||
|
||||
## Not verified
|
||||
|
||||
`node --check` passes on every script; the muxer is covered by the Node test
|
||||
above. The app itself was **not** loaded in a browser (a running Chrome instance
|
||||
held the Playwright profile lock), so the following still needs a manual pass:
|
||||
|
||||
1. `cd src && python -m http.server 8765`, open `http://localhost:8765`
|
||||
2. Add a shape, keyframe it, drag the keyframe, scrub, undo/redo
|
||||
3. Export as WEBM with a video layer and an audio layer present — confirm the
|
||||
console shows `Rendering n%` (frame-accurate path) and not
|
||||
`Falling back to the real-time encoder`
|
||||
4. Play the result: check audio is in sync and the video layer is not smeared
|
||||
Reference in New Issue
Block a user