Fix video export stall when trim regions cause long decoder gaps#682
Fix video export stall when trim regions cause long decoder gaps#682m8i-51 wants to merge 21 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0623b38c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !this.cancelled | ||
| ) { | ||
| if (Date.now() - this.lastEncoderOutputAt > ENCODER_STALL_TIMEOUT_MS) { | ||
| if (Date.now() - stallWaitStartAt > ENCODER_STALL_TIMEOUT_MS) { |
There was a problem hiding this comment.
Because the stall check now compares against stallWaitStartAt, lastEncoderOutputAt is no longer read anywhere in the repo (only assigned in initializeEncoder, the encoder output callback, and cleanup). This project has noUnusedLocals: true in tsconfig.json, so the next tsc build will report TS6133 for the private field and fail; either remove the field/assignments or keep the stall logic reading it in a way that still avoids the trim-gap false positive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Removed lastEncoderOutputAt and all its assignments — it was only ever written after this change, never read.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughVideoExporter replaces a global ChangesEncoder stall timeout isolation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
m8i-51
force-pushed
the
fix/export-stall-trim-region
branch
from
a0623b3 to
1f02e7e
Compare
When a recording has a large trim region (e.g. 400s–828s removed), the decoder must sequentially decode and discard all frames in that region to maintain P/B-frame state. On a 3320x2160 source this can take 40–50 seconds of wall time. During that decode pass the encoder queue drains to empty and lastEncoderOutputAt stops updating. When the next segment's frames arrive and fill the encode queue, the stall detector would compare Date.now() against the stale lastEncoderOutputAt (~50 s ago) and incorrectly throw a stall error, aborting the export. Fix: measure stall timeout from when the queue-full while-loop is entered (stallWaitStartAt), not from the last global encoder output. This gives the encoder a fresh 15 s window to produce output each time the queue fills up, regardless of how long the decoder spent on trimmed frames. Also remove VideoFrame leak-tracker debug code added during diagnosis, and switch latencyMode to "realtime" with a smaller maxEncodeQueue to reduce encoder internal buffering depth.
m8i-51
force-pushed
the
fix/export-stall-trim-region
branch
from
1f02e7e to
f3ec6b0
Compare
Extracts the queue-full wait loop into waitForEncoderQueueSpace() so the timing logic can be unit tested without real WebCodecs. Covers the original false-positive: a long gap before the call (e.g. decoder discarding frames in a trim region) must not count against the 15s timeout, since the timer starts at call time, not from the encoder's last output.
… fix/export-stall-trim-region
The "long gap before this call" case was mathematically identical to the queue-drain test once now()/sleep() are injected — shifting the fake clock's epoch doesn't change now() - stallWaitStartAt. Replaced with a comment on why the bug can't recur: the function takes no external "last output" timestamp to go stale in the first place.
2 participants
Description
Fixes a false-positive encoder stall that causes video export to abort when a
project has trim regions covering a large portion of the source recording.
Motivation
StreamingVideoDecoderreads the source file sequentially — it cannot seek.Frames inside trimmed regions must still be decoded to maintain P/B-frame
state, but are discarded. For a recording with a large trim in the middle, this
discard phase can take tens of seconds of wall time.
During the discard phase the encoder queue is empty and
lastEncoderOutputAtgoes stale. When real frames arrive after the trim and fill the queue, the
timeout has already elapsed — a false positive that aborts a healthy export.
Fix: reset the timer at the start of each queue-full wait instead of
measuring from the last encoder output.
Type of Change
Related Issue(s)
None
Screenshots / Video
Testing
The stall detection path exercises
VideoEncoderdirectly, which is a browserWebCodecs API not available in the Node test environment. I verified the fix
manually with a project that previously reproduced the issue consistently.
recording (e.g. trimming out several minutes from the middle)
Checklist
Summary by CodeRabbit