refactor(export): drive GIF from the video exporter's frame walk - #202
Draft
EtienneLescot wants to merge 2 commits into
Draft
refactor(export): drive GIF from the video exporter's frame walk#202EtienneLescot wants to merge 2 commits into
EtienneLescot wants to merge 2 commits into
Conversation
GIF had its own loop over the LIVE PREVIEW Player, stepping one SOURCE frame
per OUTPUT frame. Consequences, both proven by the new test:
- a 4 s / 60 fps source at 12 fps produced 48 frames taken from the first
0.8 s — right frame count, 5x slow motion, 80% of the recording missing;
- it could not decode files run_composited_multi handles (Player::open ->
'frame sans texture D3D11'), so it was not merely mistimed.
Extract the format-agnostic half of run_multi_inner as walk_composited_timeline
(clip iteration, decoder reuse, availability clamping, scene windowing,
keyframe seeks, cursor binding, speed segments, output-time advancement) and
drive both exporters through it. MP4 keeps its encoder/mux/audio; GIF supplies
readback + quantize. 'Which source frame belongs at output frame N' now has
exactly one definition.
Also fixes the dither, which was a measurable no-op: it diffused error against
round() of an already-integer channel, so err stayed 0 for every pixel by
induction. Fused into the index mapping, where the error against the CHOSEN
palette entry actually exists.
tests/export_timing.rs pins both: MP4 frame count vs output fps, and a GIF
whose per-frame palettes must span a source that changes colour every second —
the assertion frame count alone cannot make. MP4 verified unchanged (120
frames) before and after the extraction.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…path Finishes the wiring #200 left open (the IPC route did not exist, so nothing could reach the native path even with the flag on) and removes the renderer-side GIF renderer it replaces. Wiring: exportGif takes the same clips + scene as exportMulti, gains the missing 'exportGif' dispatcher case, a client method, and resolveSceneAssetPaths (the GIF path never had it). ExportDialog now has one native branch for both formats — GIF differs only in output size preset, frame rate and loop count. Deleted, all consumer-less once GIF stopped using them: gifExporter, its documentExporter adapter, frameRenderer, the WGSL shaders it owned (composite/shadowCascade/evaluate), threeDPass, cropSchedule, frameExtract, timestampedVideoFrameQueue, and the NATIVE_GIF_EXPORT_ENABLED flag. gif.js and @types/gif.js drop out of package.json. This ends the second graphical SSOT: preview and export were both native already, but GIF still rasterised through a parallel WGSL implementation that had to be hand-synced with shaders.hlsl — and silently degraded when its 3D pass failed to init ('rotation fields will be ignored'), producing different pixels from the preview with no error. pixi.js stays: cursor rendering (pixiCursorRenderer, nativeCursor, CursorPreviewLayer) and zoomTransform still use it. That is the remaining half. The browser test suite went with it — its only test was the pixi GIF exporter, and the config existed to give Pixi software WebGL in headless CI. Test-file typecheck errors drop 80 -> 74; baseline lowered to match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #200. Base is `ponytail/native-gif-export`, so this diff is only the unification.
Why
#200 landed native GIF as a third rendering path: it drove the live preview `Player` with its own loop, separate from `pipeline::run_composited_multi`. Two consequences, both now proven by tests:
The bug was a symptom of not sharing, so patching the timing in place would have left the duplication that caused it.
What
Extract the format-agnostic half of `run_multi_inner` as `walk_composited_timeline`: clip iteration, decoder reuse, availability clamping, per-clip scene windowing, keyframe seeks, cursor binding, speed segments, and output-time decoder advancement. Both exporters drive it; only the per-frame sink differs (hardware NV12 encode vs CPU readback + palette quantize). MP4 keeps its encoder, mux and audio.
GIF also becomes multiclip — the old path was single-file by construction.
Also fixes the dither, which was a measurable no-op: it diffused error against `round()` of an already-integer channel, so the error was 0 for every pixel by induction. Now fused into the index mapping, against the chosen palette entry.
Verification
`crates/compositor/tests/export_timing.rs`, opt-in via `OPENSCREEN_TEST_MEDIA` (no CI builds this crate — see below).
Not in this PR
Risk
Touches the primary export path. Mitigated by the before/after MP4 assertion, but worth noting no workflow builds the Rust workspace, so none of this is covered by CI.