fix: export frame counter exceeding total frames#414
Conversation
📝 WalkthroughWalkthroughGif and video exporters now call Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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: d21dd1cbf1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
LorenzoLancia
left a comment
LorenzoLancia
left a comment
There was a problem hiding this comment.
Nice improvement overall — centralizing the export metrics is definitely the right direction👍
That said, I think there may still be a mismatch risk with totalFrames.
Right now it’s calculated using a ceil per segment, but decodeAll() appears to stop frame emission using the endSec - epsilon cutoff. In some cases (short segments, fragmented trims, speed changes), that means the decoder may emit fewer frames than totalFrames.
So this may still leave us with an inaccurate counter, just in the opposite direction.
I think totalFrames should be computed using the same counting rule as the decoder, rather than a separate approximation.
Happy to get a second opinion here as well.
Sorry I missed that, I've updated the code to account for the subtraction of |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/exporter/streamingDecoder.ts (2)
248-250: lowkey inconsistent withgetExportMetricscomputationthis computes segment frame counts without the epsilon subtraction, but
getExportMetrics(line 552) doesseg.endSec - seg.startSec - EPSILON_SEC. they don't match.it works rn because the emission logic at lines 362/429 has epsilon boundary checks that are the real termination condition. but this is kinda cursed -
segmentOutputFrameCountsis a generous upper bound that doesn't reflect actual emitted frames.if someone tweaks the emission logic later without realizing this, they could reintroduce the frame count overshoot bug. consider aligning them:
♻️ suggested fix for consistency
const segmentOutputFrameCounts = segments.map((segment) => - Math.ceil(((segment.endSec - segment.startSec) / segment.speed) * targetFrameRate), + Math.max(0, Math.ceil(((segment.endSec - segment.startSec - EPSILON_SEC) / segment.speed) * targetFrameRate)), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` around lines 248 - 250, segmentOutputFrameCounts is computed as Math.ceil(((segment.endSec - segment.startSec) / segment.speed) * targetFrameRate) which differs from getExportMetrics that subtracts EPSILON_SEC; update the computation in streamingDecoder.ts to subtract EPSILON_SEC from (segment.endSec - segment.startSec) before dividing by segment.speed so both use the same duration logic (use the same EPSILON_SEC constant and keep Math.ceil for the upper-bound), ensuring segmentOutputFrameCounts, getExportMetrics, and the emission logic are consistent.
587-587: nit: hardcoded threshold vs EPSILON_SECthis uses
0.0001but the rest of the file now usesEPSILON_SEC(0.001). might be intentional since this is a different kind of check (filtering degenerate segments vs boundary comparisons), but worth a comment or using something likeEPSILON_SEC / 10to make the relationship explicit.not blocking, just a consistency nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` at line 587, Replace the hardcoded 0.0001 threshold in the degenerate-segment filter with a reference to EPSILON_SEC to keep consistency (either use EPSILON_SEC if that semantic matches, or use EPSILON_SEC / 10 if you intended a tighter threshold) and add a brief comment in streamingDecoder (the filter using result.filter((s) => s.endSec - s.startSec > ...)) explaining why you chose the particular relationship (exact EPSILON_SEC vs scaled) so the intent is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 248-250: segmentOutputFrameCounts is computed as
Math.ceil(((segment.endSec - segment.startSec) / segment.speed) *
targetFrameRate) which differs from getExportMetrics that subtracts EPSILON_SEC;
update the computation in streamingDecoder.ts to subtract EPSILON_SEC from
(segment.endSec - segment.startSec) before dividing by segment.speed so both use
the same duration logic (use the same EPSILON_SEC constant and keep Math.ceil
for the upper-bound), ensuring segmentOutputFrameCounts, getExportMetrics, and
the emission logic are consistent.
- Line 587: Replace the hardcoded 0.0001 threshold in the degenerate-segment
filter with a reference to EPSILON_SEC to keep consistency (either use
EPSILON_SEC if that semantic matches, or use EPSILON_SEC / 10 if you intended a
tighter threshold) and add a brief comment in streamingDecoder (the filter using
result.filter((s) => s.endSec - s.startSec > ...)) explaining why you chose the
particular relationship (exact EPSILON_SEC vs scaled) so the intent is explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eec2238b-52c4-45c5-9bfc-e0bb8c586f4d
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/exporter/streamingDecoder.ts (1)
249-251: nit: cleaner if the segment-frame math lives in one helper.This PR is specifically about keeping predicted and emitted counts aligned, so duplicating the segment count formula in two places is kinda cursed. One helper keeps the next epsilon tweak from drifting the UI again.
possible cleanup
+ private getSegmentOutputFrameCount( + segment: { startSec: number; endSec: number; speed: number }, + targetFrameRate: number, + ): number { + const segmentDurationSec = segment.endSec - segment.startSec - EPSILON_SEC; + return Math.max(0, Math.ceil((segmentDurationSec / segment.speed) * targetFrameRate)); + } + const segmentOutputFrameCounts = segments.map((segment) => - Math.ceil( - ((segment.endSec - segment.startSec - EPSILON_SEC) / segment.speed) * targetFrameRate, - ), + this.getSegmentOutputFrameCount(segment, targetFrameRate), ); ... - totalFrames: segments.reduce((sum, seg) => { - const segDur = seg.endSec - seg.startSec - EPSILON_SEC; - return sum + Math.max(0, Math.ceil((segDur / seg.speed) * targetFrameRate)); - }, 0), + totalFrames: segments.reduce( + (sum, seg) => sum + this.getSegmentOutputFrameCount(seg, targetFrameRate), + 0, + ),Also applies to: 553-556
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` around lines 249 - 251, Duplicate segment-frame-count math is used in streamingDecoder.ts (occurrences around the Math.ceil expression and again at the other block noted). Extract that formula into a single helper (e.g., getSegmentFrameCount(segment, targetFrameRate)) that uses segment.startSec, segment.endSec, segment.speed and EPSILON_SEC and returns the computed frame count (Math.ceil of the adjusted duration * targetFrameRate). Replace both existing inline calculations with calls to this helper so predicted and emitted counts stay aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 249-251: getExportMetrics() is over-counting totalFrames based
solely on duration which can include slices that never produce a heldFrame in
decodeAll(); update the totalFrames calculation to mirror decodeAll()'s
held-frame availability rules (or alternatively keep the nearest pre-start frame
alive until the segment resolves). Concretely: in the logic that computes
totalFrames (the Math.ceil branch using
segment.startSec/endSec/speed/targetFrameRate and EPSILON_SEC), only count
frames for a segment if it will actually provide a heldFrame per decodeAll()'s
criteria (i.e., a decoded sample at/after startSec - EPSILON_SEC or an explicit
heldFrame flag), or preserve the nearest pre-start frame for the segment until
decodeAll() finishes; ensure symbols referenced are decodeAll(),
getExportMetrics(), totalFrames, heldFrame, EPSILON_SEC, and
segment.startSec/endSec/speed so the counting and decoder EOF reuse rules stay
consistent.
---
Nitpick comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 249-251: Duplicate segment-frame-count math is used in
streamingDecoder.ts (occurrences around the Math.ceil expression and again at
the other block noted). Extract that formula into a single helper (e.g.,
getSegmentFrameCount(segment, targetFrameRate)) that uses segment.startSec,
segment.endSec, segment.speed and EPSILON_SEC and returns the computed frame
count (Math.ceil of the adjusted duration * targetFrameRate). Replace both
existing inline calculations with calls to this helper so predicted and emitted
counts stay aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27e54d7f-f47e-443e-a773-45b6ef311ef0
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
3 participants
Pull Request Template
Description
When exporting a video or GIF, the frame counter would exceed the
totalFramesvalue in some cases (when there are trim or speed segments). Reason is that currently we gettotalFramesby simply calculatinMath.ceil(effectiveDuration * this.config.frameRate);, basically a "ceil of sums" instead the true frame count which is a "sum of ceils".Motivation
Technically, the wrong total-frame count is being shown to the user and then it's also unsatisfying to see the the frame counter exceeding that value (see screenshot), even for a second before it gets set to equal the total-frame value. Why not use and show the correct values when we can.
I consolidated the total-frame counter and effective-duration counter into a single
getExportMetricsmethod so that it reuses the calculation ofthis.computeSegmentsandthis.splitBySpeed.Type of Change
Related Issue(s)
--
Screenshots / Video
Screenshots (if applicable):
Video (if applicable):
Testing
Most videos with some trim or speed segements should reproduce this. The more segments the higher the chances for a wrong frame count.
Checklist
Thank you for contributing!
Summary by CodeRabbit