fix(tools): require consecutive frames for flash analysis #1467

Merged

Arkex-ai merged 1 commit from fix/frame-log-integrity into main

2026-11-10 05:01:44 +00:00

Arkex-ai commented

2026-11-10 04:01:09 +00:00

(Migrated from github.com)

The binary frame-log writer may drop queued records. scripts/framelog.py reports missing frames but still compares neighbouring stored records as consecutive presented frames, so a missing immediate neighbour can produce a false one-frame flash.

Require both sequence steps of a flash triplet to be one modulo uint32, keep advancing the rolling brightness cache across gaps, and report eligible/skipped triplets. Preserve brightness thresholds and PNG event ranking. Validate incomplete/short captures and invalid headers with useful CLI errors; --inspect checks metadata without NumPy. Analyse a completed stable capture; this does not infer completeness across pauses or diagnose every visual defect.

Validation:

Isolated stdlib binary-header/CLI regressions: gaps, resumed continuity, uint32 wrap, invalid kinds/order, short and truncated records, legacy output argument.
Real NumPy authored black/white/black control: main reports one event for sequences 0,1,3; candidate reports zero. Contiguous/wrapped triplets and a later valid triplet still report the intended event. Flat resumed frames remain quiet; PNG positive control works. Ungated-comparison and stale-cache mutations revive false events.
Private existing game/renderer build a13a403 on a37ab47: 192 captured records, 96 game/96 made, 190 eligible triplets, no missing records, analysis exit0. Original checkpoint hashes unchanged. This is producer/reader format compatibility, not runtime validation of the newer main renderer.
Local strict suite on main 9d6936a plus this patch: 190 passed, 4 failed, 16 skipped. The same baseline environment failures remain (Windows symlink privileges, fps-overlay fixture initializer_list, native SDK precompile guard, display-startup unused-global warning under local LLVM). Exact-head fork CI follows.

Python-only change; no renderer/host implementation or capture-format change. No game files, captures, images or private logs included. PR1418 addresses the separate text probe reader.

Current-head validation

Current head ddfe875, based on main 75f6d89. Earlier local/runtime results describe their recorded source hashes.

The standalone contribution patch is unchanged after the audited main refresh; exact-head build/source checks are refreshed below. Earlier runtime evidence remains tied to its recorded hash.

Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

The binary frame-log writer may drop queued records. `scripts/framelog.py` reports missing frames but still compares neighbouring stored records as consecutive presented frames, so a missing immediate neighbour can produce a false one-frame flash. Require both sequence steps of a flash triplet to be one modulo uint32, keep advancing the rolling brightness cache across gaps, and report eligible/skipped triplets. Preserve brightness thresholds and PNG event ranking. Validate incomplete/short captures and invalid headers with useful CLI errors; `--inspect` checks metadata without NumPy. Analyse a completed stable capture; this does not infer completeness across pauses or diagnose every visual defect. Validation: - Isolated stdlib binary-header/CLI regressions: gaps, resumed continuity, uint32 wrap, invalid kinds/order, short and truncated records, legacy output argument. - Real NumPy authored black/white/black control: main reports one event for sequences 0,1,3; candidate reports zero. Contiguous/wrapped triplets and a later valid triplet still report the intended event. Flat resumed frames remain quiet; PNG positive control works. Ungated-comparison and stale-cache mutations revive false events. - Private existing game/renderer build a13a403 on a37ab47: 192 captured records, 96 game/96 made, 190 eligible triplets, no missing records, analysis exit0. Original checkpoint hashes unchanged. This is producer/reader format compatibility, not runtime validation of the newer main renderer. - Local strict suite on main 9d6936a plus this patch: 190 passed, 4 failed, 16 skipped. The same baseline environment failures remain (Windows symlink privileges, fps-overlay fixture initializer_list, native SDK precompile guard, display-startup unused-global warning under local LLVM). Exact-head fork CI follows. Python-only change; no renderer/host implementation or capture-format change. No game files, captures, images or private logs included. PR1418 addresses the separate text probe reader.  ### Current-head validation Current head `ddfe875`, based on main `75f6d89`. Earlier local/runtime results describe their recorded source hashes. The standalone contribution patch is unchanged after the audited main refresh; exact-head build/source checks are refreshed below. Earlier runtime evidence remains tied to its recorded hash. - [Linux AppImage](https://github.com/Arkex-ai/Hookshot/actions/runs/42756434231): pending. - [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42756430601): pending. - [Game-free source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42756427489): pending. Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 04:18:44 +00:00

hckrman101 (Migrated from github.com) left a comment

Nits only, no need to act:

Rejecting a whole capture on a non-advancing timestamp is stricter than before. It matches how the writer assigns them, so I think it's fine, but it's worth knowing if an older capture ever trips it.
main() returns 0 only on the --inspect path and implicitly None otherwise. Both exit 0, so it's harmless, but an explicit return 0 at the end would be tidier.

I didn't build or run it; CI is not reported on this branch upstream, only the fork runs in the description.

Looks good. I read the gating against the writer in gpu/vk/vk_replay.cpp. The queue can drop records (the 512 cap), and seq/at are assigned under present_image_mu, so requiring both steps of a triplet to be +1 mod 2^32 is the right fix. The rolling prev/cur still advances across gaps, so the next contiguous triplet is compared against the right neighbours, and the thresholds and PNG ranking are untouched. The stdlib-only header validation and `--inspect` are a nice touch. The new test needs no run_all.py entry because it is picked up by the tests/test_*.py discovery, and it only touches scripts/framelog.py, so it doesn't clash with #1418 (that one changes scripts/probe.py). Nits only, no need to act: - Rejecting a whole capture on a non-advancing timestamp is stricter than before. It matches how the writer assigns them, so I think it's fine, but it's worth knowing if an older capture ever trips it. - `main()` returns 0 only on the `--inspect` path and implicitly None otherwise. Both exit 0, so it's harmless, but an explicit `return 0` at the end would be tidier. I didn't build or run it; CI is not reported on this branch upstream, only the fork runs in the description.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101