Repository navigation
fix(video): promptly retry IDR after reference-invalidation fallback - #349
Conversation
Propagate backend RFI fallback into the async loop's pending IDR state, reusing #348's retry handling for repeated direct inputs. Preserve successful invalidation and process every queued feedback range. Add an extracted-production CPU regression covering 36 NVENC/AMF recovery scenarios, with a failing unchanged-canary negative control. Prepared with OpenAI Codex on behalf of the repository owner. Full Windows build and live GPU streaming validation remain unrun.
Nonary
left a comment
There was a problem hiding this comment.
Round 1 — blocked / stale target. No clearance.
Session: 01a16697-848b-72e3-ac79-99701d0a30cd (independent Windows review session).
Reviewed head: 73a00e8084194894a9da8ef999ebc7f95d828935.
Assigned diff base: 28f086f665ffd9102b3e4172c7c18b1076cdc9d9 (merged #348).
Immediately before posting, the head still matches, but live canary is 72c18359f2976f337631836186cc64cfa36efb0b, verified through both branch and Git-ref endpoints. PR metadata still reports the assigned base. This attempt does not cover integration with the advanced target.
Partial evidence: read the complete four-file assigned diff, all three session invalidation implementations, backend fallback behavior, event accumulation, and async/sync repeat handling. The async non-short-circuit |= preserves fallback while processing subsequent feedback; the existing retry_idr path carries it across repeats. No actionable finding is submitted from this partial inspection.
Reproduced locally with Python 3.13 / MinGW g++:
- 36 NVENC/AMF recovery scenarios passed.
- Unchanged assigned-base negative control compiled and failed behaviorally: fallback after one repeat waited 100 ms.
- Existing AMF output-first production-extraction regression passed all 21 assertions.
Profile/allocation blocker: the actual installed ultracode/SKILL.md is a generic workflow skill; it defines no “72 Luna High” profile/allocation or exact Luna mapping. Exposed controls offer both gpt-6-luna and gpt-5.6-luna with High effort. Neither was silently substituted. Requested ceiling: 72 total, six concurrent helpers, four independent confirmer/denier pairs per selected nontrivial candidate. Actual completed allocation: 0/72 helpers; 0 pairs; parent inspection only. Required profile execution remains unperformed.
No full application build, full CTest run, GPU/stream test, or independent helper adjudication was completed here. Arch Linux CI was successful and Windows CI in progress at the last check. Repository tree inspection found no applicable tracked AGENTS.md or .agents/skills under the reviewed paths; untracked local repository instructions were unavailable in this source snapshot.
Blocked action: complete the specified independent review of PR #349, due to unverifiable required profile and changed live target. No write action was denied or left uncertain. An updated target assignment and a compatible exact profile contract/environment are required. PR remains draft; no source edits, pushes, merge, release, permission changes, or readiness transition.
|
Thanks for catching this. #348 only covered explicit keyframe requests, and I missed that an invalidation can fall back to an IDR the loop never sees. The fix makes sense, and good call reusing the retry from #348. Next time I'll wait for your agent's review before merging anything into canary. I'm adding this fix to the Vibepollo port in Nonary/Vibepollo#530 and will leave that PR for your review. |
Nonary
left a comment
There was a problem hiding this comment.
Round 1 completed — clear (code review). No actionable findings.
Supersedes the stale, blocked attempt. The updated assignment clarified the supported model mapping and that 72 is a total ceiling.
- Session:
01a16697-848b-72e3-ac79-99701d0a30cd. - Reviewed head:
73a00e8084194894a9da8ef999ebc7f95d828935. - Reviewed live canary target:
8f94e2932a22afa2411324a85c644f37a46e1cf0. - Diff merge base:
28f086f665ffd9102b3e4172c7c18b1076cdc9d9. Live head/target refs and existing comments were rechecked immediately before posting.
Execution: actual installed UltraCode workflow; supported native spawn_agent calls explicitly used gpt-6-luna / High effort / fresh context and completed. Allocation: 14 distinct helpers plus seven clarification/integration follow-ups (21 bounded passes), below the 72 ceiling, at most six concurrent helpers. Four fresh independent confirmer/denier pairs evaluated the core recovery claim; confirmers accepted it and deniers found no introduced counterexample. Model provenance is the accepted native model/effort controls, with no invented alias or branded preset.
The full four-file diff, all three session implementations, backend success/fallback and acceptance lifetimes, accumulated feedback, retained direct-input pixels, and both codec loops were reviewed. Fallback survives duplicate skips; successful RFI retains cadence and every consumed feedback range is dispatched. Every RFI-capable codec entry routes through async encoding; PyroWave's separate intra-frame handling needs no RFI action.
Current-base integration: inspected all target advances through 72c1835…, 6b4e1a3…, and the final target. The latest capability-diagnostics changes are separate from recovery logic. Three-way checks combine video.cpp, video.h, and tests/CMakeLists.txt cleanly. Two independent refresh reviewers found no interaction, and the combined source snapshot passed the CPU regressions.
Validation: 36 NVENC/AMF recovery cases passed on the head and combined snapshot; unchanged-base negative control compiled and reproduced the 100 ms delay; AMF output-first regression passed all 21 assertions. Windows CI and Arch Linux CI passed on this head. All observed check runs completed successfully or were skipped.
Limits: no local full application build, full CTest configuration, or GPU/stream test. The new fixture observes extracted control flow and pending-IDR state rather than hardware bitstream output. Reading /repos/Nonary/vibeshine/branches/canary/protection was denied HTTP 403 (Resource not accessible by integration); stopped without retry, so required contexts could not be independently enumerated. Actual enforced repository checks remain mandatory.
Round 1 only; PR left draft. No source edits, pushes, merges, release actions, permission changes, or readiness transition. Parent-owned final review remains separate.
… Propagate backend RFI fallback into the async loop's pending IDR state, reusing Nonary#348's retry handling for repeated direct inputs. Preserve successful invalidation and process every queued feedback range. Add an extracted-production CPU regression covering 36 NVENC/AMF recovery scenarios, with a failing unchanged-canary negative control. Prepared with OpenAI Codex on behalf of the repository owner. Full Windows build and live GPU streaming validation remain unrun. Vibepollo port of Nonary/vibeshine#349. (cherry picked from commit 73a00e8084194894a9da8ef999ebc7f95d828935) Signed-off-by: Ramazan Kara <github@r-kara.de>
Propagate backend RFI fallback into the async loop's pending IDR state, reusing Nonary#348's retry handling for repeated direct inputs. Preserve successful invalidation and process every queued feedback range. Add an extracted-production CPU regression covering 36 NVENC/AMF recovery scenarios, with a failing unchanged-canary negative control. Prepared with OpenAI Codex on behalf of the repository owner. Full Windows build and live GPU streaming validation remain unrun. Vibepollo port of Nonary/vibeshine#349. (cherry picked from commit 73a00e8084194894a9da8ef999ebc7f95d828935) Signed-off-by: Ramazan Kara <github@r-kara.de>
Nonary
left a comment
There was a problem hiding this comment.
Replacement independent final review — clear (code review). No actionable findings.
- Session:
01a166dd-dfef-7615-bc79-ec43b8d870f2. - Exact reviewed head:
73a00e8084194894a9da8ef999ebc7f95d828935. - Live canary integration target:
1184c8732fc6a93bad2e754c057643facbf6562b. - Diff merge base:
28f086f665ffd9102b3e4172c7c18b1076cdc9d9. PR metadata still reports this historical base; branch/ref endpoints independently verify the live target. - Separate completed round 1, session
01a16697-848b-72e3-ac79-99701d0a30cd, reconciled against actual session records (14 distinct completed helpers). No first-pass actionable finding or outstanding change request. The cancelled prior final session supplied no clearance and was not used. Comments/reviews were checked again before posting; no other final review appeared.
Execution/allocation: actual installed UltraCode skill at C:/Users/Chase/.codex/skills/ultracode/SKILL.md, using the assignment's verified supported controls gpt-6-luna / High / fresh context. 8 distinct completed helpers, four independent confirmer/denier pairs on C1 (fallback/repeat/success/mixed-event correctness), plus 2 completed backend/integration follow-up turns and 2 clarification exchanges; at most six helpers concurrent, below the 72 total ceiling. All pairs accept C1 after source adjudication. No invented model alias or claim that 72 helpers ran.
Fresh review covered all four changed files, the bool contract and all three session overrides, callers/routing, NVENC/AMF fallback and acceptance state, async/sync loops, retained input, and tests. Failed invalidation accumulates with non-short-circuit |=, survives duplicate skips via retry_idr, and bypasses sparse-capture waiting once the queue is empty. Successful invalidation preserves cadence; mixed feedback orders dispatch every range without clearing fallback. RFI-capable codecs route asynchronously; PyroWave's all-intra path needs no invalidation recovery.
Integration: inspected target advances including installer-only #350. Actual three-way merges of src/video.cpp, src/video.h, and tests/CMakeLists.txt all return cleanly. Combined snapshot preserves target diagnostics/adapter-identity API and Boost.Regex/sanitizer wiring. No real conflict reproduced.
Validation: Python 3.13 / MinGW g++ ran 36 recovery scenarios and all 21 AMF output-first assertions on both exact-head and combined snapshots. Unchanged-base negative control compiled and reproduced the 100 ms delay. Windows CI and Arch CI pass on this head; observed checks are completed success/skipped. Normal branch metadata reports required-check enforcement off with empty contexts; the active canary ruleset enforces deletion/non-fast-forward protection, no required-check rule. The previously denied protection endpoint was not retried.
Limits: no local full application build, complete CTest configuration or GPU/stream testing. The new fixture checks extracted control flow, pending IDR state and capture waits, not hardware bitstream output; the separate AMF fixture tests production submission/delivery with fake edges. No applicable tracked AGENTS.md under reviewed paths; unrelated docs/linux instructions do not apply. Local canonical checkout instructions were inaccessible in this restricted review environment.
Code review complete; final live gate/readiness transition is checked separately. No source edits, pushes, merges or release actions. Merge and final cloud check remain with the parent.
Summary
Follow-up to merged #347 and #348. This fixes a remaining recovery-delay path introduced by #347's repeated-direct-input skip; it does not attribute that regression to #348 and preserves/reuses #348's retry mechanism.
A supported IDX_INVALIDATE_REF_FRAMES request can make NVENC or AMF fall back to an IDR (for example, NVENC's invalidated range covers the DPB). The backend sets its IDR flag, but the async loop did not know a keyframe was pending. If the next direct input was a repeat, it skipped the image and could wait max_frametime on a static desktop before submitting that recovery IDR.
Validation
Commands:
These are CPU control-flow tests, not GPU/stream measurements. Full Windows application build, complete CTest configuration, and live GPU streaming tests were not run locally. CI results are pending.
Prepared with OpenAI Codex on behalf of the repository owner. Kept draft pending independent review and remaining validation; not ready for merge.