Repository navigation
fix(video): stop re-encoding earlier segments when extending videos - #9833
Conversation
video_concat and extract_video_range stream-copy H.264 frames and AAC packets the app already encoded, re-encoding only the frames and audio around each cut, with lossless splices verified and a full re-encode as fallback. Video encodes now use CRF 18 with a 2 s IDR interval so later joins have cut points.
JPPhoto
left a comment
There was a problem hiding this comment.
First, a high-level overview of what I think the aims should be and where this PR stands:
This is a useful step toward stopping quality loss during repeated extensions, but it solves a narrower problem.
| Aim | PR 9833 |
|---|---|
| Copy safe regions and encode only transition bridges across the whole timeline | Partial. It copies only a prefix of the first clip. It re-encodes that clip’s tail and every later clip. It also decodes the copied prefix to skip it. |
| Preserve existing cuts, fades, frame counts, and audio behavior | Mostly. The legacy timeline and blending path remains, and the new tests cover frame counts and audio seams. |
| Safely decide which media can be spliced | Partial. It checks IDR boundaries, codec configuration, edit lists, rotation, and sample counts. It does not check all relevant timing and container metadata, such as PTS/DTS cadence, color metadata, and sample aspect ratio. |
| Fall back safely when splicing is unsupported or uncertain | Partial. Several cases fall back to full encoding, but the sample-table parser can allocate based on an unchecked input count, and cancellation cannot stop the new splice subprocesses promptly. |
| Validate browser playback and demonstrate smart-render performance | No. The PR says browser playback was not checked. It reports quality comparisons, but not a representative speed or memory benchmark for the broader smart-render approach. |
So it meets part of the quality-preservation goal for chained extensions. It does not yet meet the broader goal of planning a timeline across multiple clips, copying every safe region, and encoding only the transition bridges.
Next, the code review...
Merge blockers:
invokeai/app/util/h264_splice.py:317: When an MP4 has nostssbox, the parser buildstuple(range(sample_count))from the input’s 32-bitstszcount before applying_MAX_SAMPLES. A crafted, probeable H.264 MP4 can trigger a huge allocation in the application process. H.264 MP4 uploads can reach storage without transcoding (invokeai/app/api/routers/videos.py:494-503), and concat calls this parser atinvokeai/app/invocations/video_concat.py:323. Effect: memory exhaustion and possible process termination. Likelihood: crafted malformed media; ordinary clips are unaffected. Recovery: restart the application. Test: pass an oversizedstszcount with nostssthrough concat preflight in a bounded subprocess; verify rejection before a count-sized allocation.
Other findings/issues:
-
invokeai/app/invocations/video_concat.py:327: Splice eligibility comparesavcCbut ignores container color and pixel-aspect metadata. MP4colrandpaspproperties are separate from the codec configuration (FFmpeg MP4 muxer, sample aspect handling). A remuxed input can retain the sameavcCwhile adding non-default metadata; the copied head can then supply track metadata for the re-encoded continuation. Effect: changed color or aspect in the output. Likelihood: plausible for remuxed or uploaded media; normal app output is unaffected. Recovery: reject mismatched metadata and use the full-encode path. Test: remux an eligible source withcolrorpasp, confirmavcCis unchanged, and compare the joined continuation with a forced full encode. -
invokeai/app/util/h264_splice.py:349: Splice subprocesses block insubprocess.run(timeout=600)without cancellation. Cancellation during video copy or concat waits for the process; during AAC splicing, cancellation can also go unchecked through subsequent stages and reach save. Effect: canceled work can keep a worker occupied for minutes and may save an unwanted output. Likelihood: plausible for long clips or slow storage. Recovery: wait for completion or timeout; delete any output saved after cancellation. Test: cancel during video and AAC splice subprocesses; verify prompt child termination and reaping, no later stages or save, and temporary-file cleanup.
Suggestions:
- Consider treating this as an incremental extension optimization. It copies only the first clip’s prefix;
_encode_joined()decodes and discards that prefix, then encodes the first clip’s tail and every later clip. The broader smart-render outcome needs safe copy spans planned across clips, with encoding limited to transition bridges.
…l splices Sample-table and sample-entry reads are bounded before allocating; eligibility compares the full sample entry (colr/pasp/...) and the joined file must keep it. ffmpeg splice children are killed on cancellation, and nodes re-check cancellation before saving.
extract_video_range stream-copies the source's AAC packets inside the range, encoding only a lead-in and tail; video_concat shares the same verified splice. Soundtracks are encoded without PNS so copied packets decode exactly after a lead-in.
|
Thanks for the thorough review. All three findings are confirmed and fixed in 9a607cc, each with a regression test that fails against the previous code. Blocker: unbounded allocation. Confirmed: an upload that is already an H.264/AAC MP4 is stored as-is, so the sample tables are the uploader's. A file claiming ~4 billion samples with no
Colour / pixel aspect. Confirmed: remuxing one of our outputs with a BT.2020/PQ
Cancellation.
Timing. Every spliced result is also probed: its fps and duration must match the expected timeline. This is in addition to the existing checks (uniform frame duration, sample counts, IDR-only cuts). Browser playback. Checked in headless Chromium 153 (Playwright), which reports
Benchmark. Extend graph (
On this branch the first segment's audio is bit-identical every round; on main it drifts. The PR's remaining per-round growth is the copied prefix still being decoded to skip it, which is your scope point. Scope. Agreed, this is an incremental extension optimisation. It fixes the reported compounding loss in the earliest segments: afterwards each segment takes one re-encode, as the new clip, and is then carried forward. The timeline-wide version will be a separate follow-up PR. It plans copy spans for every clip (for Extend, the new clip after its first IDR past the transition, so it never loses a generation either) and seeks past copied prefixes rather than decoding them. #9834, stacked on this, already applies the same splice to trimmed sources' audio. |
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
invokeai/app/util/aac_splice.py:168(same issue ininvokeai/app/util/h264_splice.py:347and:362): The splice readers load uploader controlledesds,avcC, and sample entry boxes into memory without a byte limit. A largefreechild in an H.264 entry is copied into multiple byte strings; the AAC reader also reads its fullesdspayload. Accepted uploads can be up to 1 GiB. Effect: a crafted first clip can exhaust backend memory and potentially terminate the server. Likelihood: requires a crafted uploaded MP4. Recovery: restart the server if the process is killed. Test: use oversizedfreeand paddedesdsboxes; confirm the parser refuses them without payload sized allocation.
Other findings/issues:
-
invokeai/app/util/h264_splice.py:343:find_box()scans foravcCbefore the later 33 box limit is applied. PuttingavcCafter many small boxes forces the parser to scan them all before rejecting the entry. Effect: a crafted upload can hold the invocation worker in an uncancellable scan. Likelihood: unusual, malformed sample entries. Recovery: wait for the scan to finish. Test: put a box flood beforeavcCand confirm scanning stops at a fixed bound. This is unusual and probably can be ignored - your call. -
invokeai/app/util/aac_splice.py:342andinvokeai/app/invocations/video_concat.py:496: The new AAC validation decodes callextract_audio_pcm(), which runs ffmpeg with a 600 second timeout and has no cancellation callback. Canceling during either decode leaves the child running until it finishes or times out. Effect: the canceled job continues occupying a worker; the save guard prevents output from being saved. Likelihood: plausible with long audio inputs. Test: cancel after each validation decode starts; confirm the child is killed and reaped promptly and nothing is saved.
## Summary Follow-up to #9833, stacked on its branch. #9833 stopped re-encoding the earlier video when a source is trimmed before extending. Its soundtrack was still re-encoded in full: `extract_video_range` decoded the kept audio and encoded all of it to AAC again. So every extension of a trimmed source cost the earlier audio one AAC generation. The docs' own advice for quality drift, "trim the bridge point a few frames back from the end", made this the common case. `extract_video_range` now stream-copies the source's AAC packets for every output frame wholly inside the range. It encodes only a short lead-in before them (unless the range starts at frame 0) and a short tail after them. - **One splice primitive, shared.** `aac_splice.splice_soundtrack` is used by both nodes: an optional lead-in, the copied packet run, an optional tail, then verification. The verification requires a bit-exact copied interior and seams no noisier than an ordinary encode of the same samples. `video_concat` keeps its behaviour and now calls the same function. - **Each seam stays clean.** Every new encode starts 4 frames of pre-roll before its seam, up from 2. Two still left an occasional ~40 ms burst on music, at 8× a full re-encode's error locally. The lead-in and the tail each hold at least 5 frames, which keeps every seam away from the encoder's cold start and its end-of-stream flush. A tail of under a frame had produced a glitch. `video_concat` uses the same constants, so its audio cut now sits 4 frames before its transition. - **The lead-in is grid-aligned.** It is padded so its frames fall on the copied packets' grid, and the padding is hidden by the audio edit list: `_set_audio_edit` now also writes `media_time`. A trimmed output therefore skips more than the encoder's priming, and the splice gate now accepts any leading edit at least that long. So a trimmed clip can be trimmed again or extended, and the copy still applies. - **PNS is off (`-aac_pns 0`)** for every soundtrack the app writes, through both the WAV mux path and `make_mp4_writer`. The decoder fills noise-substituted bands from a random generator whose state carries across packets. A lead-in that used PNS therefore changed how the copied packets after it decode, and verification failed. That happened even on pure tones: with PNS on, 6 of the 8 new trim-audio tests fail. Sources written earlier with PNS on can still be copied wherever their packets verify. Trimming a test clip before extending it, in each case after the trim: | | Before | After | |---|---|---| | Source audio inside the range | one AAC generation per extension | bit-identical | | Re-encoded audio | the whole range | 5–6 AAC frames at each end (≈ 0.1 s each at 48 kHz) | ## Related Issues / Discussions Stacked on #9833; merge that first. This PR's base is #9833's branch, so the diff shows only this change. ## QA Instructions - `uv run --no-sync pytest tests/app -n 4 -k "video or mp4 or metadata or wan or ltx2 or minimax or h264 or audio"`: 1680 passed (the `ingest` tests included). - `uv tool run ruff@0.11.2 check invokeai tests` and `format --check`: clean. - New `tests/app/invocations/test_video_frame_extract_range_audio_splice.py`, real ffmpeg. It covers: - trims starting at frame 0, mid-clip, and running to the last frame, on tone and on noise content; - a bit-exact interior, seams within a full re-encode's error, and an exact container duration; - verification rejecting copied audio that doesn't decode back; - the Extend chain (trim → join) keeping the source's audio bit-exact. - Mutation-checked: turning PNS back on, setting the lead-in/tail minimum to 1 frame, or dropping the lead-in padding each fails tests. - Not run in the browser. To check manually: trim a Wan or LTX-2 clip with sound a few frames back from its end, extend it a few times, and listen across the joins. ## Review Review found no correctness defects in about 300 real-ffmpeg trims, joins and chains, including trim→trim, trim→join→trim and join→trim. Those chains produced leading edits other than 1024 (1232–1824), and every result verified bit-exact against the original source. Material findings, all resolved: - **Ingest kept PNS on:** the upload transcoder still encoded AAC with PNS, so transcoded uploads passed the gate and then usually failed verification. It now uses the same encoder options. - **Silent skip at fractional samples-per-frame:** at rates where a frame holds a fractional number of samples (44.1 kHz at 24 or 16 fps, 16 kHz at 30 fps), the trim splice was silently skipped. The trimmed span was rounded independently of the sample window, so it came out one sample off. That also made the full path linearly resample the whole window by ±1 sample. At the source's own rate the window is now used as-is. - **Occasional tail-seam burst:** with 2 frames of pre-roll, the seam after an encoded tail occasionally carried a ~40 ms burst on music, at 8× a full re-encode's error locally. The seam check can't see this, because its reference encode has the same cold start. Pre-roll is now 4 frames. Remaining limitations: - **Older files mostly fall back.** Soundtracks written before this change (PNS on), and uploads, usually fail verification on trims that start mid-clip, and are then re-encoded once. That's the previous behaviour, plus a wasted attempt of about 0.4 s on a 5-minute source. - **Seams are not quite as clean as a full encode.** For 1–2 frames (20–40 ms) at the lead-in and tail seams, error is still about 2.5–3× a full re-encode's, measured in 512-sample blocks. - **The seam check is coarse.** It averages over two frames against a reference that shares the encoder's cold start, so correctness rests mainly on the pre-roll and margins. - **Players that ignore edit lists** would start a trimmed soundtrack up to about 43 ms late, instead of the usual 21 ms. ffmpeg and browsers honour edit lists. ## Compatibility / Rollout No persisted-state or API changes. New soundtracks are encoded without PNS, so noisy content gets spent as waveform bits rather than substituted noise. At the default 128 kb/s AAC-LC bitrate this is a minor trade. ## Checklist - [x] _The PR has a short but descriptive title, suitable for a changelog_ - [x] _Meaningful regression coverage added / updated where needed; obsolete tests/code removed_ - [ ] _Persisted-state and API changes include required migrations / compatibility validation_ - [x] _Relevant performance/efficiency opportunities considered; material claims have evidence_ - [x] _Material review findings resolved and relevant checks rerun_ - [x] _Documentation added / updated (if applicable)_ - [ ] _Updated `What's New` copy (if doing a release after this PR)_ <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/invoke-ai/codesmith/InvokeAI/pr/9834?autoLogin=true&ref=codesmith_pr_footer"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1794449838&installation_model_id=441795&pr_number=9834&ref=codesmith_pr_footer&repository=invoke-ai%2FInvokeAI&return_to=https%3A%2F%2Fgithub.com%2Finvoke-ai%2FInvokeAI%2Fpull%2F9834&signature=85dfebc0316c5fffc033baadfb969a8401d6a58d0cfb315e5a4b141c7b06dc44"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer -->
Sample entries are size-capped before any read and every container walk is box-capped, so crafted uploads cannot exhaust memory or hold a worker. One cancellable ffmpeg runner now backs the splice copies, audio decodes and muxes, and the passthrough tag remux.
|
Thanks — all three are fixed in ca9837f, each with a regression test that fails against the previous code. #9834, which was stacked on this branch, has been merged into it, so the trim-audio splice is now part of this PR. The description is updated to cover it. Blocker: unbounded box reads.
Validation decodes.
|
This comment has been minimized.
This comment has been minimized.
JPPhoto
left a comment
There was a problem hiding this comment.
The earlier findings about oversized parser reads, unbounded box walks, and uncancellable validation decodes are fixed!
Merge blockers:
- macOS Python tests fail. In test_h264_splice.py:140, the subprocess unconditionally sets
RLIMIT_ASto 4 GiB. On the macOS runner its hard limit is lower, so both parameterizations fail withValueError: current limit exceeds maximum limit, before the parser is tested. The Python test run reports 13,680 passed and 2 failed. Set a limit supported by the runner or handle this platform's limit.
Other findings/issues:
- P2 - Full-range copy can preserve audio past the video's duration. video_frame_extract_range.py:235 bypasses audio rebuilding for a whole-range copy.
_save_unchangedremuxes every audio stream unchanged (mp4_metadata.py:254), while reporting duration from the video frame count. A compatible H.264 file with 5 seconds of video and 10 seconds of AAC audio therefore keeps the extra audio, unlike the normal path, which trims audio to the selected frame range. Gate the copy on matching audio duration or trim the copied soundtrack; add coverage for longer source audio.
The whole-source copy is taken only when no stream plays past the last frame (probed duration), and carries only the first audio track, as a trim does. The RLIMIT_AS allocation test runs only on Linux, which enforces the limit.
|
Thanks. Both findings are confirmed and fixed in 4b52ab1. macOS Python tests. The macOS runner's hard Full-range copy keeping audio past the video. Confirmed: 2.5 s of H.264 with 5 s of AAC came out with all 5 s.
|
JPPhoto
left a comment
There was a problem hiding this comment.
@lstein I am making a test for the following issue, expect it shortly:
invokeai/app/invocations/video_frame_extract_range.py:241: If imageio probing fails and the supported OpenCV fallback runs,_probe()sets duration from video frame count and FPS alone (invokeai/app/util/video_decode_worker.py:163). A full-range H.264 clip with longer AAC audio then passes the gate, and_save_unchanged()preserves the extra audio. Effect: audio outlasts the output video and its reported duration. Likelihood: uncommon; requires OpenCV to open a spliceable clip after imageio probing fails. Recovery: manually trim or re-encode the output. Test: force the fallback with an H.264/AAC fixture whose audio outlasts its video; verify full-range extraction cuts audio at the video end.
Use the MP4 movie timeline when probe fallback reports only video duration. Cover longer AAC on the OpenCV probe fallback.
Rename the duration-stub regression and cover the real worker fallback.
Summary
Extending a video repeatedly made artifacts build up in its earliest segments. Each Extend run decoded the whole source and re-encoded it twice, first in
extract_video_range(even for an untrimmed source) and then invideo_concat. Both used imageio's default quality, which is libx264 CRF 25. After N extensions the first segment had been through 2N lossy encodes, and its soundtrack through 2N AAC encodes.The earlier footage is now carried forward without re-encoding wherever the files allow it:
video_concat. When the first clip was encoded by this app at the output canvas and frame rate, everything up to its last usable IDR keyframe is stream-copied. Only the rest is encoded: the clip's tail, the transition and the later clips. The soundtrack works the same way. The first clip's own AAC packets are kept up to four frames before its outgoing transition, and only the remainder is encoded, with four frames of pre-roll so the seam stays at coding-noise level.extract_video_range. A whole-range extract at the source's own frame rate is a copy of the source file. A trim stream-copies the keyframe-aligned run of frames inside the range and encodes only the partial keyframe intervals at each end. Its soundtrack is treated the same way: the source's AAC packets for every frame wholly inside the range are copied, and only a short lead-in and tail (5–6 AAC frames each) are encoded. Both nodes use one verified splice,aac_splice.splice_soundtrack.-aac_pns 0). The decoder fills PNS bands from a random generator whose state carries across packets, so a lead-in that used PNS would change how the copied packets after it decode, and they could not be verified.make_mp4_writer. Every video node now encodes at CRF 18 and forces an IDR every 2 seconds, so later joins have cut points.Anything that can't be spliced losslessly falls back to the previous full re-encode. That covers uploads, files from older Invoke versions, retimed joins, rotated or edit-list-trimmed sources, and open-GOP streams. Every splice is checked before it is used:
avcC) must match, there must be a single timeline with a uniform frame duration, and the expected frame counts must appear.Before and after, extending a test clip three times (CRF 25 → CRF 18 plus splicing):
Related Issues / Discussions
Reported in discussion: repeated "extend video" runs showed growing artifacts in the earliest segments.
#9834, which was stacked on this branch, has been merged into it, so this PR now also includes the trimmed-source soundtrack splice described above.
Follow-up planned: copy the safe regions of every clip in a join, not just the first clip's head, and seek past copied prefixes instead of decoding them.
QA Instructions
uv run --no-sync pytest -n 4: 13305 passed, 210 skipped, 8 xfailed. This run predates the final review round. Since then, the video/audio/metadata subset (-k "video or mp4 or metadata or wan or ltx2 or minimax or h264 or audio") has been rerun on the final code (withingestanduploadadded): 1737 passed.uv tool run ruff@0.11.2 check invokeai testsandformat --check: clean.tests/app/invocations/test_video_concat_splice.py: bit-exact head across 3 chained extensions (video and audio); container duration matches the video; spliced soundtrack matches a full re-encode under every transition, seam included; fallbacks for foreign encoders, retimes, edit-list trims, rotation and short remainders.tests/app/invocations/test_video_frame_extract_range.py: whole-range copy; trims with bit-exact copied runs; short encoded ends keep the frame rate; open-GOP sources; the source's tags aren't carried over.tests/app/util/test_h264_splice.py: the layout gates.MIN_ENCODED_FRAMESor the pre-roll to 1, fails its test.canPlayTypereportsprobablyfor H.264 + AAC). Files tested, all passed through the production metadata remux: a 5-round spliced extension chain,main's 5-round chain as a control, and a trim of the spliced chain that itself spliced. Each played to the end with no media errors, every frame decoded, 0 corrupted frames, and the full audio track decoded. Seeking to every frame and comparing an 8×8 block signature with ffmpeg's decode, every frame matched its own index (difference 0.025, against 0.77 for a neighbour), so there were no misplaced frames at any seam.Review
Material findings, all resolved:
ffmpeg -c copy/ LosslessCut) listed more samples than they decode, so a splice made the node fail. Such tracks are now refused.avcCcomparison now runs before the long encode, avoiding a wasted second encode.mvhdoffset is fixed.From maintainer review, all resolved:
stszwith nostsscould exhaust memory). Sample-entry box lists are bounded too: concatenating them was quadratic, and listing themp4achildren amplified memory.avcC:colr/pasp/clapetc., with the encoder-version name masked. The copied head and the joined file must keep it, which catches apaspffmpeg derives from atkhddisplay size.From the second maintainer review, all resolved:
avc1) or 4 KiB (mp4a) are refused before any of their bytes are read, so a hugefreebox or a paddedavcC/esdsis never loaded into memory.moov,trak,mdia,minf,stbl,edts) stops after 64 child boxes, andavcCis found within the bounded child list. A box flood therefore can't hold a worker for minutes. Measured before the fix: ~240 s for a 1 GiB flood.video_audio.run_ffmpeg_process) now backs the splice copies, the audio decodes (including the splice's validation decodes), the audio mux, and the whole-range copy's tag remux.From the third maintainer review, all resolved:
RLIMIT_ASallocation test runs only on Linux, which enforces that limit.Remaining limitations:
Compatibility / Rollout
No persisted-state or API changes.
Checklist
What's Newcopy (if doing a release after this PR)Need help on this PR? Tag
@codesmith-botwith what you need.