MagicBeanDev/Hookshot
Archived
Watch
2
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

fix(video): balance Media Foundation buffer locks on rejected frames #2859

Open
github-sync wants to merge 2 commits from github/pr/2859 into main
pull from: github/pr/2859
merge into: MagicBeanDev:main
Conversation 5 Commits 2 Files changed 3 +198 -9
github-sync commented 2026-11-12 17:57:21 +00:00
Owner

@Arkex-ai opened this pull request on GitHub · 2026-11-12 17:15 UTC · original

Summary

This hardens the Windows Media Foundation fallback against rare buffer-lock and attachment failures. A successfully locked but rejected output buffer could remain locked, and a failed input lock could reach an invalid copy. There is no observed gameplay symptom; the normal Windows movie path tries dav1d first, so this is not a change affecting every Windows player. Linux does not compile this file.

Fixes #2835. Draft: resource-lifetime regressions are reproduced, but this change has not been run in the game. No gameplay incidence, GPU-specific improvement, physical-RAM saving or FPS gain is claimed.

Technical changes

  • Separate successful lock ownership from picture acceptance; every successful 2D or linear lock is unlocked before releasing the buffer, including rejected null/zero/negative-pitch output.
  • Check input locking and sample-buffer attachment, release acquired references on failure, and never copy into a failed or null locked destination.
  • Preserve valid-frame bytes, callback dimensions, decoding selection, output-layout policy and the existing GPU interface. This does not add support for bottom-up NV12 pictures.
  • Add 15 game-free scenarios exercising the actual pull and mf_av1_decode bodies with controlled HRESULT failures and counted lock/reference lifetimes.

The current open-PR file inventory found no other change to host/mf_av1.c. Private investigation preceded the delivered team-room claim.

Validation

Current head: bc5078ac39828af4aa5271b4d4a450f27dd3142e. The review followup restores docs/worklist.md byte-for-byte to the base, removing the unmeasured reach/priority row and fixing the unrelated encoding change. Source and tests are unchanged from 3cf32313b95a8ec96aaf69562ec23968742ec886, which was refreshed onto de8192345086c7c79c115fff260163508bb19c43. The source and focused tests are byte-identical to the previously tested commit 5d8d0d167d453f18a6e2a2c57817fb88ae8ecc38; the upstream changes were reviewed before the refresh.

  • Historical complete Linux suite at 3cf32313 (identical source/test bytes): python -I -B tests/run_all.py --jobs 2 finished 426 passed, 3 failed, 71 skipped, including the focused buffer-lifetime regression. The complete tracked tree and authored commit object were SHA-verified; the checkout was shallow at the exact head, without parent history. Failed checks: tests/test_bake_portability.py, tests/test_hookshot_ctl.py, tests/test_mem_gate_children.py. The unchanged-base controls at 51da0464 also failed test_bake_portability.py (missing Python zstandard), test_hookshot_ctl.py (its runtime-directory prerequisite) and test_mem_gate_children.py (child resident-memory accounting below its expected amount). These controls distinguish the observed local failures from a claim that the complete suite is green; skipped prerequisites were not suppressed.
  • Windows / Clang 23.1.5, before refresh on identical source/test bytes: all 15 HRESULT/lifetime scenarios pass. Nine scenarios fail behaviorally on the unchanged base and six controls pass. Coverage includes 2D/linear/fallback output, rejected picture layouts, both lock failures, sample attachment, input lock/null/length/attachment failures, and sample/buffer allocation failure. The harness intercepts unsafe copies in negative controls rather than deliberately performing invalid memory access.
  • Native Windows Media Foundation control: an actual MFCreate2DMediaBuffer supplied through an authored one-output transform reproduced the unreleased output lock on the base; the fix left it unlocked. The deliberately unsupported bottom-up RGB picture exercises rejection cleanup, not a real AV1 decode, an observed GPU-driver incident or a gameplay corruption. Valid picture behavior is separately covered by the controlled regression.
  • Repeated Linux focused validation on the same source/test bytes: three GCC 14 runs, three Clang 17 runs and three Clang AddressSanitizer + UndefinedBehaviorSanitizer runs pass with leak detection enabled. Linux runs use the authored COM stand-ins, not the Windows MF runtime.
  • Both complete base and fixed mf_av1.c translation units passed strict Windows syntax checks against the actual host interface and existing local generic runtime headers. No linked-game build or complete Windows suite is claimed for this PR.

Current-head hosted checks: Game-free source regressions, Linux AppImage, PR title, Windows builder. Their live conclusions are separate from local tests. Independent code review was COMMENTED, not approval. The remaining runtime gate is a Windows build using Media Foundation, not dav1d: identify the decoder from HOS_VIDEO_LOG, play the intro and a movie through its end/drain, and report GPU, driver, OS, hardware/software decoding and any regressions. The authored native buffer test is not a decoder-owned D3D sample or a real AV1/movie run. Full linked-game compilation and this explicit fallback playback validation remain outstanding; no speed or memory measurement is claimed.

> [@Arkex-ai](https://github.com/Arkex-ai) opened this pull request on GitHub · 2026-11-12 17:15 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2859) ## Summary This hardens the Windows Media Foundation fallback against rare buffer-lock and attachment failures. A successfully locked but rejected output buffer could remain locked, and a failed input lock could reach an invalid copy. There is no observed gameplay symptom; the normal Windows movie path tries dav1d first, so this is not a change affecting every Windows player. Linux does not compile this file. Fixes #2835. **Draft: resource-lifetime regressions are reproduced, but this change has not been run in the game.** No gameplay incidence, GPU-specific improvement, physical-RAM saving or FPS gain is claimed. ## Technical changes - Separate successful lock ownership from picture acceptance; every successful 2D or linear lock is unlocked before releasing the buffer, including rejected null/zero/negative-pitch output. - Check input locking and sample-buffer attachment, release acquired references on failure, and never copy into a failed or null locked destination. - Preserve valid-frame bytes, callback dimensions, decoding selection, output-layout policy and the existing GPU interface. This does not add support for bottom-up NV12 pictures. - Add 15 game-free scenarios exercising the actual `pull` and `mf_av1_decode` bodies with controlled HRESULT failures and counted lock/reference lifetimes. The current open-PR file inventory found no other change to `host/mf_av1.c`. Private investigation preceded the delivered team-room claim. ## Validation Current head: **`bc5078ac39828af4aa5271b4d4a450f27dd3142e`**. The review followup restores `docs/worklist.md` byte-for-byte to the base, removing the unmeasured reach/priority row and fixing the unrelated encoding change. Source and tests are unchanged from **`3cf32313b95a8ec96aaf69562ec23968742ec886`**, which was refreshed onto `de8192345086c7c79c115fff260163508bb19c43`. The source and focused tests are byte-identical to the previously tested commit `5d8d0d167d453f18a6e2a2c57817fb88ae8ecc38`; the upstream changes were reviewed before the refresh. - **Historical complete Linux suite at `3cf32313` (identical source/test bytes):** `python -I -B tests/run_all.py --jobs 2` finished **426 passed, 3 failed, 71 skipped**, including the focused buffer-lifetime regression. The complete tracked tree and authored commit object were SHA-verified; the checkout was shallow at the exact head, without parent history. Failed checks: `tests/test_bake_portability.py`, `tests/test_hookshot_ctl.py`, `tests/test_mem_gate_children.py`. The unchanged-base controls at `51da0464` also failed `test_bake_portability.py` (missing Python zstandard), `test_hookshot_ctl.py` (its runtime-directory prerequisite) and `test_mem_gate_children.py` (child resident-memory accounting below its expected amount). These controls distinguish the observed local failures from a claim that the complete suite is green; skipped prerequisites were not suppressed. - **Windows / Clang 23.1.5, before refresh on identical source/test bytes:** all 15 HRESULT/lifetime scenarios pass. Nine scenarios fail behaviorally on the unchanged base and six controls pass. Coverage includes 2D/linear/fallback output, rejected picture layouts, both lock failures, sample attachment, input lock/null/length/attachment failures, and sample/buffer allocation failure. The harness intercepts unsafe copies in negative controls rather than deliberately performing invalid memory access. - **Native Windows Media Foundation control:** an actual `MFCreate2DMediaBuffer` supplied through an authored one-output transform reproduced the unreleased output lock on the base; the fix left it unlocked. The deliberately unsupported bottom-up RGB picture exercises rejection cleanup, not a real AV1 decode, an observed GPU-driver incident or a gameplay corruption. Valid picture behavior is separately covered by the controlled regression. - **Repeated Linux focused validation on the same source/test bytes:** three GCC 14 runs, three Clang 17 runs and three Clang AddressSanitizer + UndefinedBehaviorSanitizer runs pass with leak detection enabled. Linux runs use the authored COM stand-ins, not the Windows MF runtime. - Both complete base and fixed `mf_av1.c` translation units passed strict Windows syntax checks against the actual host interface and existing local generic runtime headers. No linked-game build or complete Windows suite is claimed for this PR. Current-head hosted checks: [Game-free source regressions](https://github.com/NotRexed/Hookshot/actions/runs/43273126595), [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/43273126603), [PR title](https://github.com/NotRexed/Hookshot/actions/runs/43273123167), [Windows builder](https://github.com/NotRexed/Hookshot/actions/runs/43273126612). Their live conclusions are separate from local tests. Independent code review was COMMENTED, not approval. The remaining runtime gate is a Windows build using Media Foundation, not dav1d: identify the decoder from HOS_VIDEO_LOG, play the intro and a movie through its end/drain, and report GPU, driver, OS, hardware/software decoding and any regressions. The authored native buffer test is not a decoder-owned D3D sample or a real AV1/movie run. Full linked-game compilation and this explicit fallback playback validation remain outstanding; no speed or memory measurement is claimed. <!-- github-sync:item:2859 -->
Preserve existing lifetime eligibility and release resources only after successful
acquisition and their permitted finalization path. Include causal regressions.

Fixes #2835

Assisted-by: GPT-6 Astra Pro
Co-Authored-By: GPT-6 Astra Pro <[email protected]>
Address review on #2859. Keep gameplay and reach unmeasured until
local hardware validation; restore the unrelated worklist bytes.

Assisted-by: GPT-6 Astra Pro
Co-Authored-By: GPT-6 Astra Pro <[email protected]>
github-sync commented 2026-11-12 18:00:08 +00:00
Author
Owner

@hckrman101 commented on docs/worklist.md line 33 on GitHub · 2026-11-12 17:24 UTC · original

This line was re-encoded by mistake: instalaçao became instalaçao (UTF-8 read as Latin-1 and written back). Please restore it; the PR shouldn't touch this row.

> [@hckrman101](https://github.com/hckrman101) commented on `docs/worklist.md` line 33 on GitHub · 2026-11-12 17:24 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2859#discussion_r4553908806) This line was re-encoded by mistake: `instalaçao` became `instalaçao` (UTF-8 read as Latin-1 and written back). Please restore it; the PR shouldn't touch this row. <!-- github-sync:rc:4553908806 -->
github-sync commented 2026-11-12 18:00:08 +00:00
Author
Owner

@hckrman101 commented on docs/worklist.md line 42 on GitHub · 2026-11-12 17:24 UTC · original

Two things about this row:

  • Reach 0.57 counts every Windows player, but Windows builds decode the movies with dav1d first, and MF is only the fallback. Real reach is the players on a build without dav1d (or with dav1d failing to open), which is much smaller.
  • Nothing in the game shows this today (see the review body), so I'd either leave the row out and let #2835 carry it, or score it with that reach and say "hardening, no user report". Also, once this PR merges, the row should go rather than stay as open work.
> [@hckrman101](https://github.com/hckrman101) commented on `docs/worklist.md` line 42 on GitHub · 2026-11-12 17:24 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2859#discussion_r4553908819) Two things about this row: - Reach 0.57 counts every Windows player, but Windows builds decode the movies with dav1d first, and MF is only the fallback. Real reach is the players on a build without dav1d (or with dav1d failing to open), which is much smaller. - Nothing in the game shows this today (see the review body), so I'd either leave the row out and let #2835 carry it, or score it with that reach and say "hardening, no user report". Also, once this PR merges, the row should go rather than stay as open work. <!-- github-sync:rc:4553908819 -->
github-sync commented 2026-11-12 18:00:08 +00:00
Author
Owner

@hckrman101 reviewed on GitHub · 2026-11-12 17:24 UTC · original

Thanks, this is a careful one. I read the change and the test against main; the code is right, and what's left is about evidence and the worklist row.

Locks and references. Every path through pull now pairs: Lock2D OK then Unlock2D; Lock2D failing (with the 2D interface held) falls back to a linear Lock that is unlocked on its own; both failing unlocks nothing; and a null pointer or a pitch <= 0 still unlocks before the release. b2 and b are released once each, and the own-sample path drops the sample if AddBuffer fails. In mf_av1_decode a failed or null Lock no longer reaches memcpy, and the SetCurrentLength/AddBuffer failures release b and s once, with no double release (the sample owns nothing yet there). Good.

What "rejected" means here. It's only a null pointer or a pitch <= 0 from a successful lock. A size change or a new output type mid-stream is the MF_E_TRANSFORM_STREAM_CHANGE path, which locks nothing and is untouched. With NV12 out of the AV1 decoder the pitch is positive, so the rejected path is close to unreachable with a real decoder, and the input lock only fails on a system-memory buffer when memory runs out. So this is hardening, with no symptom in the game today. That's fine as a change; I'd just like the PR and the row to say so (see the inline note).

Reach. On Windows, Media Foundation is the fallback: Windows builds now ship dav1d (HOOKSHOT_DAV1D, the "dav1d for Windows" job), and nn_video.c tries dav1d first. MF only runs when a build lacks dav1d or dav1d fails to open. Linux never compiles mf_av1.c and uses FFmpeg/libdav1d, so it is unaffected. #2682 (merged today) only touches nn_video.c. Your base already has it, and the drain at the stream's end (mf_av1_decode(NULL) then pull) goes through the same fixed unlock, so I see no interaction.

The test extracts the real pull/mf_av1_decode bodies and counts locks, unlocks and references through HRESULT-injected stand-ins. That models the pairing well. It can't model a decoder-owned D3D sample (where an unbalanced lock would really hurt), but your native MFCreate2DMediaBuffer run covers that side. It's declared through RUN_ALL_ARGS and builds as C under the {cxx} driver with -x c, which is fine.

To take it out of draft (our in-game rule): one Windows run on the Media Foundation path, i.e. a build without dav1d, so the log (HOS_VIDEO_LOG=1) shows video: AV1 on Media Foundation: <decoder>. Play the intro movie and one movie to its end (that end is where #2682 changed things), and say whether the decoder was hardware or software and which GPU. A short note that the movies look and end the same as before is enough. No numbers are needed, since nothing claims a speed or memory change. Then fix the two worklist points below, and it's good to go from my side.

CI on this head hasn't run yet: every job is still pending.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 17:24 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2859#pullrequestreview-5878992837) Thanks, this is a careful one. I read the change and the test against main; the code is right, and what's left is about evidence and the worklist row. **Locks and references.** Every path through `pull` now pairs: Lock2D OK then Unlock2D; Lock2D failing (with the 2D interface held) falls back to a linear Lock that is unlocked on its own; both failing unlocks nothing; and a null pointer or a pitch <= 0 still unlocks before the release. `b2` and `b` are released once each, and the own-sample path drops the sample if `AddBuffer` fails. In `mf_av1_decode` a failed or null Lock no longer reaches `memcpy`, and the `SetCurrentLength`/`AddBuffer` failures release `b` and `s` once, with no double release (the sample owns nothing yet there). Good. **What "rejected" means here.** It's only a null pointer or a pitch <= 0 from a successful lock. A size change or a new output type mid-stream is the `MF_E_TRANSFORM_STREAM_CHANGE` path, which locks nothing and is untouched. With NV12 out of the AV1 decoder the pitch is positive, so the rejected path is close to unreachable with a real decoder, and the input lock only fails on a system-memory buffer when memory runs out. So this is hardening, with no symptom in the game today. That's fine as a change; I'd just like the PR and the row to say so (see the inline note). **Reach.** On Windows, Media Foundation is the fallback: Windows builds now ship dav1d (`HOOKSHOT_DAV1D`, the "dav1d for Windows" job), and `nn_video.c` tries dav1d first. MF only runs when a build lacks dav1d or dav1d fails to open. Linux never compiles `mf_av1.c` and uses FFmpeg/libdav1d, so it is unaffected. #2682 (merged today) only touches `nn_video.c`. Your base already has it, and the drain at the stream's end (`mf_av1_decode(NULL)` then `pull`) goes through the same fixed unlock, so I see no interaction. **The test** extracts the real `pull`/`mf_av1_decode` bodies and counts locks, unlocks and references through HRESULT-injected stand-ins. That models the pairing well. It can't model a decoder-owned D3D sample (where an unbalanced lock would really hurt), but your native `MFCreate2DMediaBuffer` run covers that side. It's declared through `RUN_ALL_ARGS` and builds as C under the `{cxx}` driver with `-x c`, which is fine. **To take it out of draft** (our in-game rule): one Windows run on the Media Foundation path, i.e. a build without dav1d, so the log (`HOS_VIDEO_LOG=1`) shows `video: AV1 on Media Foundation: <decoder>`. Play the intro movie and one movie to its end (that end is where #2682 changed things), and say whether the decoder was hardware or software and which GPU. A short note that the movies look and end the same as before is enough. No numbers are needed, since nothing claims a speed or memory change. Then fix the two worklist points below, and it's good to go from my side. CI on this head hasn't run yet: every job is still pending. <!-- github-sync:r:5878992837 -->
github-sync commented 2026-11-12 18:00:08 +00:00
Author
Owner

@Arkex-ai commented on docs/worklist.md line 33 on GitHub · 2026-11-12 17:57 UTC · original

Restored docs/worklist.md byte-for-byte to the base in bc5078ac, including the unrelated encoding damage. No MF source or test bytes changed in this followup.

> [@Arkex-ai](https://github.com/Arkex-ai) commented on `docs/worklist.md` line 33 on GitHub · 2026-11-12 17:57 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2859#discussion_r4554275282) Restored docs/worklist.md byte-for-byte to the base in bc5078ac, including the unrelated encoding damage. No MF source or test bytes changed in this followup. <!-- github-sync:rc:4554275282 -->
github-sync commented 2026-11-12 18:00:08 +00:00
Author
Owner

@Arkex-ai commented on docs/worklist.md line 42 on GitHub · 2026-11-12 17:57 UTC · original

Removed the Windows-wide reach/priority row in bc5078ac and corrected #2835 and the PR summary: this is MF fallback hardening with no observed gameplay symptom, not a change to the normal dav1d path or Linux. The native MFCreate2DMediaBuffer control is an authored rejected RGB buffer, not a decoder-owned D3D sample or actual AV1 playback. The draft gate remains an explicitly MF-selected Windows build and decoder log, intro plus a movie through end/drain, with the actual decoding mode/GPU/driver/OS reported. That run has not been done.

> [@Arkex-ai](https://github.com/Arkex-ai) commented on `docs/worklist.md` line 42 on GitHub · 2026-11-12 17:57 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2859#discussion_r4554275584) Removed the Windows-wide reach/priority row in bc5078ac and corrected #2835 and the PR summary: this is MF fallback hardening with no observed gameplay symptom, not a change to the normal dav1d path or Linux. The native MFCreate2DMediaBuffer control is an authored rejected RGB buffer, not a decoder-owned D3D sample or actual AV1 playback. The draft gate remains an explicitly MF-selected Windows build and decoder log, intro plus a movie through end/drain, with the actual decoding mode/GPU/driver/OS reported. That run has not been done. <!-- github-sync:rc:4554275584 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Hookshot!2859
No description provided.