Skip to content

Commit a0e18d9

Browse files
committed
feat: enhance documentation for edge cases in trimming and update recording service with FFmpeg optimizations
1 parent cecdf89 commit a0e18d9

9 files changed

Lines changed: 142 additions & 29 deletions

File tree

.gitignore

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,4 +48,5 @@ coverage/
4848
electron-app/.superpowers/
4949
superpowers/
5050
design-prototypes/
51-
docs/superpowers/
51+
docs/superpowers/
52+
.obsidian/

docs/wiki/log.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,8 @@ _`## [YYYY-MM-DD] file | pages/type/name.md (mid-session)`_
3030
## [2026-04-08] file | pages/concepts/frontend-waveform-loading.md (mid-session)
3131
## [2026-04-08] file | pages/edge-cases/trim-resume-position.md (mid-session)
3232
## [2026-04-08] file | pages/edge-cases/trim-strictmode-double-finalize.md (mid-session)
33+
## [2026-04-08] file | pages/edge-cases/trim-duration-reset-race.md (mid-session)
34+
## [2026-04-08] file | pages/edge-cases/trim-finalize-cancelled-by-media-rerender.md (mid-session)
35+
## [2026-04-08] file | pages/edge-cases/trim-ffmpeg-stream-copy-duration.md (mid-session)
36+
## [2026-04-08] update | pages/concepts/frontend-clip-trim-flow.md — pathChanged guard extended to cover trim state; new related links
37+
## [2026-04-08] update | pages/modules/recordingService.md — added -movflags +faststart quirk

docs/wiki/pages/concepts/frontend-clip-trim-flow.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
type: concept
33
tags: [VideoPlayer, clip, trim, frontend, react, state, ipc]
44
updated: 2026-04-08
5-
sources: 0
5+
sources: 1
66
---
77

88
# Frontend Clip & Trim Flow
@@ -67,7 +67,11 @@ Trim involves six distinct state variables and a carefully ordered two-phase IPC
6767
When `handleLoadedMetadata` fires on the reloaded video, it checks `resumePositionRef.current` and seeks to that position (clamped to the new duration). If `resumeAfterTrimRef.current` is true, it also resumes playback — so the video restarts from approximately where it was before the trim.
6868

6969
### The path-equality guard on media change
70-
When `media` changes, the effect that resets state checks `pathChanged = media?.path !== lastMediaPathRef.current`. If the path is **the same** (which happens when `onTrimmed` triggers a re-render with the updated clip metadata), `resumeAfterTrimRef` and `resumePositionRef` are preserved so `handleLoadedMetadata` can still consume them.
70+
When `media` changes, the effect that resets state checks `pathChanged = media?.path !== lastMediaPathRef.current`. If the path is **the same** (which happens when `onTrimmed``fetchClips``setSelectedClip` returns a fresh object for the same clip), the following states are intentionally **not** reset:
71+
72+
- `currentTime` and `duration``handleLoadedMetadata` fires exactly once per video load. If it already fired and set the correct trimmed values before `setSelectedClip` triggered the media-change effect, zeroing these would permanently corrupt them (no second `loadedmetadata` event arrives because `src` is unchanged). So they are only reset when `pathChanged = true`.
73+
- `resumeAfterTrimRef` and `resumePositionRef` — preserved so `handleLoadedMetadata` can restore the seek position.
74+
- `trimFinalizePath`, `suppressVideoSrc`, `trimPending`, `virtualTrimStart/End`**must not be reset on same-path re-renders**. If the user starts a second trim quickly, `fetchClips` from the first `handleTrimmed` call can return and trigger a `pathChanged = false` re-render while the second trim's finalize effect is still awaiting the API. Resetting `trimFinalizePath` or `suppressVideoSrc` would change a dep of the finalize effect, running its cleanup (`cancelled = true`), causing the API response to be discarded. The video would then reload without incrementing `videoReloadToken`, serving the browser-cached pre-trim URL. See [[edge-case-trim-finalize-cancelled-by-media-rerender]].
7175

7276
---
7377

@@ -89,4 +93,6 @@ This means the user can still scrub and play the "trimmed" portion while waiting
8993
- [[video-processing-pipeline]]
9094
- [[recordingService]]
9195
- [[edge-case-trim-two-phase]]
96+
- [[edge-case-trim-finalize-cancelled-by-media-rerender]]
97+
- [[edge-case-trim-ffmpeg-stream-copy-duration]]
9298
- [[frontend-waveform-loading]]
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
---
2+
type: edge-case
3+
tags: [VideoPlayer, trim, react, state, duration, handleLoadedMetadata]
4+
updated: 2026-04-08
5+
sources: 0
6+
---
7+
8+
# Edge Case: Trim Leaves Duration = 0 After Reload (Race Condition)
9+
10+
**Trigger:** User trims a clip; `handleLoadedMetadata` fires quickly (local file serves fast) before `onTrimmed``setSelectedClip` completes.
11+
12+
**Symptom:** After trim, the video is correctly replaced on disk and reloads with the trimmed content, but the UI shows `duration = 0`, the progress bar is stuck at 0/0, and trying to enter trim mode again sets `clipEnd = 0` → Trim button disabled. The clip appears "unchanged."
13+
14+
**Root Cause:** `useEffect([media])` unconditionally called `setDuration(0)` and `setCurrentTime(0)` every time `media` changed — including when `onTrimmed` triggers `setSelectedClip` with the same-path clip after a successful trim. `handleLoadedMetadata` fires exactly once per video load. If it already fired and set `duration = trimmedDuration` before the media-change effect ran, the `setDuration(0)` call permanently zeroed the duration (the video src is unchanged, so no second `loadedmetadata` event arrives to correct it).
15+
16+
**Fix:** `setCurrentTime(0)` and `setDuration(0)` are now gated inside `if (pathChanged)`, exactly like the resume refs. When `pathChanged = false` (same-path re-render after trim), these values are left as-is — `handleLoadedMetadata` has either already set the correct trimmed value, or will set it when the load completes.
17+
18+
**Prevention:** Any state that is set by a one-shot DOM event (`loadedmetadata`, `canplaythrough`, etc.) must NOT be reset unconditionally in a media-change effect that also fires on same-path re-renders. Always gate with `pathChanged`.
19+
20+
## Related
21+
22+
- [[frontend-clip-trim-flow]]
23+
- [[edge-case-trim-two-phase]]
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
type: edge-case
3+
tags: [recordingService, ffmpeg, trim, mp4, duration, movflags, stream-copy]
4+
updated: 2026-04-08
5+
sources: 0
6+
---
7+
8+
# Edge Case: Trim Appears Unchanged — FFmpeg Stream-Copy Writes Wrong MP4 Duration
9+
10+
**Trigger:** Trim succeeds on the backend (file is renamed, size is smaller) but the UI shows the original duration after reload.
11+
12+
**Symptom:** After trim, `handleLoadedMetadata` reports the pre-trim duration (e.g. 60.928s) even though `fetchClips` returns the refreshed clip with a smaller `size_bytes`. The timeline appears unchanged. The media data *is* actually trimmed — if the video plays past the real end it simply stops — but the container header still advertises the old length.
13+
14+
**Root Cause:**
15+
16+
FFmpeg, when doing a stream-copy (`-c copy`) with input seeking (`-ss` before `-i`), may carry the source container's original `mvhd.duration` value into the output rather than computing it from the actual output samples. This happens because the moov atom is written early (as a placeholder) before all samples are known. The duration field in that placeholder reflects the source file's duration.
17+
18+
The symptom is the combination of: correct media data in mdat + wrong `mvhd.duration` / `tkhd.duration` fields in the moov atom. Players that trust the container header (like Chromium's `<video>`) report the wrong duration.
19+
20+
**Fix:** Add `-movflags +faststart` to the FFmpeg trim command:
21+
22+
```
23+
ffmpeg -y -ss <start> -i <source> -t <dur> -c copy -avoid_negative_ts make_zero -movflags +faststart <output>
24+
```
25+
26+
With `+faststart`, FFmpeg writes all the media data first (mdat), then writes the moov atom last — at which point the exact output duration is known from the actual samples — and finally moves moov to the front of the file. Because moov is computed *after* all samples are written, the duration fields are always correct.
27+
28+
**Side effect:** `+faststart` slightly increases FFmpeg's working time (requires a second pass over the moov to move it). For clips up to a few GB this is negligible; the rename step that follows takes longer anyway (EPERM retries on Windows).
29+
30+
**Detection:** Compare `refreshedClip.size_bytes` (smaller than original) against `videoDuration` from `handleLoadedMetadata`. If size decreased but duration is unchanged, this is the bug.
31+
32+
## Related
33+
34+
- [[recordingService]]
35+
- [[frontend-clip-trim-flow]]
36+
- [[edge-case-trim-two-phase]]
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
type: edge-case
3+
tags: [VideoPlayer, trim, react, state, media-change-effect, finalize, race]
4+
updated: 2026-04-08
5+
sources: 0
6+
---
7+
8+
# Edge Case: Trim 2 Finalize Cancelled by Trim 1's handleTrimmed Re-render
9+
10+
**Trigger:** User trims a clip twice in quick succession — specifically, they start trim 2 fast enough that trim 1's `handleTrimmed → fetchClips` HTTP response arrives while trim 2's finalize phase is still in-flight.
11+
12+
**Symptom:** The video appears to reload (suppressVideoSrc toggled), but shows the old trimmed content (trim 1 result, not trim 2). The backend succeeds for both trims (both renames complete). The frontend discards the trim 2 result silently.
13+
14+
**Root Cause:**
15+
16+
The `media-change effect` (`useEffect([media])`) unconditionally called `setTrimFinalizePath(null)` and `setSuppressVideoSrc(false)` — even when `pathChanged = false` (same clip path, new object reference).
17+
18+
When trim 1 completes, it calls `onTrimmed → handleTrimmed → fetchClips()`. When `fetchClips()` returns, it calls `setSelectedClip(refreshedClip)`. This causes `media` to change (new object reference, same path), firing the `media-change effect` with `pathChanged = false`.
19+
20+
If trim 2's finalize effect is still awaiting the API response at that moment, the media-change effect sets `trimFinalizePath = null` — which is a dep of the finalize effect. The finalize effect cleanup runs, setting `cancelled = true`. When the finalize API response arrives, the `if (cancelled)` guard discards it. `videoReloadToken` never increments.
21+
22+
Meanwhile, `suppressVideoSrc` was restored to `false` by the media-change effect (not by the finalize success handler), so the video element reappears. But because the token didn't change, the video src URL is identical to the previous trim's URL. Chromium's HTTP cache may serve the stale response, so the user sees the trim-1 result instead of the trim-2 result.
23+
24+
**Fix:** Moved `setTrimFinalizePath(null)`, `setSuppressVideoSrc(false)`, `setTrimPending(false)`, `setVirtualTrimStart(null)`, and `setVirtualTrimEnd(null)` inside the `if (pathChanged)` block in the media-change effect. When the same clip re-renders with a new object reference (`pathChanged = false`), these values are left as-is — any in-progress finalize can complete uninterrupted.
25+
26+
**Prevention:** State that is actively used by an in-flight async operation (finalize effect) must not be reset by side-effects that fire for unrelated reasons (same-path media object re-renders). Gate all trim-operation state inside `if (pathChanged)`.
27+
28+
## Related
29+
30+
- [[frontend-clip-trim-flow]]
31+
- [[edge-case-trim-duration-reset-race]]
32+
- [[edge-case-trim-two-phase]]

docs/wiki/pages/modules/recordingService.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ sources: 0
2828
- **`trimState` map** — persists in memory per process. If the main process restarts between `trimClip` and `finalizeTrim`, the state is lost and `finalizeTrim` will throw. The `.tmp.mp4` orphan must be cleaned up manually.
2929
- **Multi-track clip produces individual + mixed audio streams** — the mux step maps `1:a` (all streams from the audio temp), so the output MP4 contains both a mixed track and individual per-track streams.
3030
- **`avoid_negative_ts make_zero`** — used on all copy-cut operations to prevent playback issues in players that don't handle negative DTS.
31+
- **`-movflags +faststart` on trim** — required so that FFmpeg computes the moov atom from actual output samples rather than copying the source container's duration. Without it, stream-copy trims can produce an MP4 whose `mvhd.duration` still reflects the pre-trim length even though the media data is correctly shortened. See [[edge-case-trim-ffmpeg-stream-copy-duration]].
3132
- **5-second scan cache**`scanRecordings()` and `scanClips()` return stale data for up to 5 seconds after an external change (e.g., a file moved from outside the app). Mutations via the app invalidate immediately.
3233
- **`activeFFmpeg` map** — tracks all running FFmpeg child processes so `killAllProcesses()` (called on app quit) can clean them up and delete their partial output files.
3334

electron-app/electron/recordingService.js

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -633,12 +633,8 @@ function createClip(sourcePath, startTime, endTime, gameName = 'Unknown', audioT
633633
// Runs FFmpeg and waits for it to finish, then returns so the frontend can
634634
// coordinate clearing its video src before the rename (finalizeTrim).
635635
async function trimClip(sourcePath, startTime, endTime) {
636-
if (!sourcePath || !fs.existsSync(sourcePath)) {
637-
throw new Error('Source not found')
638-
}
639-
if (endTime <= startTime) {
640-
throw new Error('End time must be > start time')
641-
}
636+
if (!sourcePath || !fs.existsSync(sourcePath)) throw new Error('Source not found')
637+
if (endTime <= startTime) throw new Error('End time must be > start time')
642638

643639
const tempPath = sourcePath + '.tmp.mp4'
644640
const duration = endTime - startTime
@@ -657,6 +653,13 @@ async function trimClip(sourcePath, startTime, endTime) {
657653
'copy',
658654
'-avoid_negative_ts',
659655
'make_zero',
656+
// +faststart rewrites the moov atom from the actual output samples, which
657+
// guarantees the container duration field reflects the trimmed length.
658+
// Without it, FFmpeg can carry over the source file's original duration in
659+
// the moov header when doing stream-copy, causing the player to report the
660+
// pre-trim duration even though the media data is correctly trimmed.
661+
'-movflags',
662+
'+faststart',
660663
tempPath,
661664
]
662665
await new Promise((res, rej) => {
@@ -688,6 +691,7 @@ async function finalizeTrim(sourcePath) {
688691
const tempPath = sourcePath + '.tmp.mp4'
689692
// Capture original mtime before overwriting so sort position is stable after trim.
690693
let origStat = null
694+
let epermCount = 0
691695
try { origStat = fs.statSync(sourcePath) } catch {}
692696
for (;;) {
693697
try {
@@ -706,6 +710,7 @@ async function finalizeTrim(sourcePath) {
706710
trimState.set(sourcePath, { status: 'failed', error: err.message })
707711
throw err
708712
}
713+
epermCount++
709714
await new Promise((r) => setTimeout(r, 50))
710715
}
711716
}

electron-app/src/viewer/components/VideoPlayer.jsx

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -136,19 +136,13 @@ function VideoPlayer({
136136

137137
// Reset state when media changes
138138
useEffect(() => {
139-
const pathChanged = media?.path !== lastMediaPathRef.current
139+
const prevPath = lastMediaPathRef.current
140+
const pathChanged = media?.path !== prevPath
140141
lastMediaPathRef.current = media?.path ?? null
141142

142143
setIsPlaying(false)
143-
setCurrentTime(0)
144-
setDuration(0)
145144
setClipMode(false)
146145
setIsTrimMode(false)
147-
setVirtualTrimStart(null)
148-
setVirtualTrimEnd(null)
149-
setTrimPending(false)
150-
setTrimFinalizePath(null)
151-
setSuppressVideoSrc(false)
152146
setIsZoomTimelineExpanded(false)
153147
setMarkers([])
154148
setAudioTracks([])
@@ -163,13 +157,25 @@ function VideoPlayer({
163157
waveformQueueRef.current = null
164158
waveformNumChunksRef.current = 0
165159
viewportChunkRef.current = null
166-
// Only clear resume-after-trim state when navigating to a genuinely different
167-
// clip. After a trim, onTrimmed causes the same clip to re-render with updated
168-
// metadata (same path) — we must keep the refs so handleLoadedMetadata can
169-
// restore the playback position before they're consumed.
160+
// When navigating to a different clip, reset playback position, duration, and any
161+
// in-progress trim state so the UI doesn't flash stale values while the new video loads.
162+
// After a trim, onTrimmed re-renders the same clip (same path, new object reference) —
163+
// we must NOT reset trimFinalizePath, suppressVideoSrc, trimPending, or virtualTrim bounds
164+
// here because a second trim may be completing its finalize phase at this exact moment.
165+
// Zeroing these would cancel the in-flight finalize effect (its cleanup sets cancelled=true),
166+
// and the video would reload without incrementing the token, causing the browser cache to
167+
// serve the old version. The resume refs are kept for the same reason: handleLoadedMetadata
168+
// reads them to restore the seek position after the trim reload.
170169
if (pathChanged) {
170+
setCurrentTime(0)
171+
setDuration(0)
171172
resumeAfterTrimRef.current = false
172173
resumePositionRef.current = null
174+
setVirtualTrimStart(null)
175+
setVirtualTrimEnd(null)
176+
setTrimPending(false)
177+
setTrimFinalizePath(null)
178+
setSuppressVideoSrc(false)
173179
}
174180
pauseWaveformFetchRef.current = false
175181
if (resumeWaveformTimerRef.current) {
@@ -428,11 +434,12 @@ function VideoPlayer({
428434

429435
const handleLoadedMetadata = useCallback(() => {
430436
if (videoRef.current) {
431-
setDuration(videoRef.current.duration)
432-
setClipEnd(Math.min(30, videoRef.current.duration))
437+
const videoDur = videoRef.current.duration
438+
setDuration(videoDur)
439+
setClipEnd(Math.min(30, videoDur))
433440
if (resumePositionRef.current !== null) {
434441
// Restore position after a trim reload, clamped to the new duration
435-
const pos = Math.min(resumePositionRef.current, videoRef.current.duration)
442+
const pos = Math.min(resumePositionRef.current, videoDur)
436443
videoRef.current.currentTime = pos
437444
if (resumeAfterTrimRef.current) {
438445
videoRef.current.play().catch(() => {})
@@ -733,7 +740,7 @@ function VideoPlayer({
733740
setVirtualTrimEnd(null)
734741
alert(`Error trimming: ${error.message}`)
735742
}
736-
}, [media, clipStart, clipEnd, trimPending, onTrimmed, onTrimFailed])
743+
}, [media, clipStart, clipEnd, trimPending, duration, onTrimmed, onTrimFailed])
737744

738745
useEffect(() => {
739746
if (!trimFinalizePath || !suppressVideoSrc) return
@@ -780,14 +787,11 @@ function VideoPlayer({
780787

781788
finalizeTrim()
782789

783-
return () => {
784-
cancelled = true
785-
}
790+
return () => { cancelled = true }
786791
}, [trimFinalizePath, suppressVideoSrc, media, onTrimmed, onTrimFailed])
787792

788793
useEffect(() => {
789794
if (suppressVideoSrc || videoReloadToken === 0 || !videoRef.current) return
790-
791795
videoRef.current.load()
792796
}, [suppressVideoSrc, videoReloadToken])
793797

0 commit comments

Comments
 (0)