Skip to content

fix(texture): call SetFrameEvent before SaveTexture/GetTextureData#207

Merged
BANANASJIM merged 11 commits intomasterfrom
fix/tex-export-d3d11-frame-event
Mar 31, 2026
Merged

fix(texture): call SetFrameEvent before SaveTexture/GetTextureData#207
BANANASJIM merged 11 commits intomasterfrom
fix/tex-export-d3d11-frame-event

Conversation

@BANANASJIM
Copy link
Copy Markdown
Owner

@BANANASJIM BANANASJIM commented Mar 28, 2026

Summary

  • _handle_tex_export and _handle_tex_raw were missing a _set_frame_event() call before invoking SaveTexture/GetTextureData
  • D3D11 replay requires SetFrameEvent(eid) to populate internal texture tracking maps (WrappedID3D11Texture*::m_TextureList); without it, SaveTexture always fails
  • Vulkan replay is not strict about ordering, so the bug only manifests on Windows D3D11 captures

Fixes #206. Reported by @Junlin-Yin.

Test plan

  • Existing unit tests pass (2777 passed)
  • Manual verification on Windows D3D11 capture: rdc texture <id> -o out.png should succeed

Summary by CodeRabbit

  • Bug Fixes

    • Texture export and raw operations now validate entity IDs and return error code -32002 for out-of-range values, improving error handling.
  • Tests

    • Added unit tests asserting proper error responses for out-of-range entity ID values in texture operations.
  • Refactor

    • Internal replay/structured-file retrieval logic refactored with no user-facing behavior changes.
  • Chores

    • CI security audit adjusted to ignore a specific known vulnerability during scanning.

…e_push to AUR git job

Curly braces in jq examples inside <pre><code> were parsed as Astro JSX
expressions, causing esbuild to fail with "triangles is not defined" and
"{}" parse errors. Wrap affected jq object literals in {"..."} string
expressions. Also add force_push: true to aur-git job to fix non-fast-forward
push rejections from AUR.
pixi run rdc previously fell back to /usr/bin/rdc (stale system
install) because no explicit task existed. Adding `rdc = "uv run rdc"`
ensures the development version is always used.
D3D11 replay requires SetFrameEvent(eid) to populate internal texture
tracking maps before SaveTexture or GetTextureData can succeed. Without
this call, WrappedID3D11Texture*::m_TextureList remains empty and
SaveTexture always fails.

Vulkan replay does not enforce this ordering, so the bug only manifests
on Windows D3D11 captures. Fixes #206.
Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bf204b44-87f1-46b6-939d-d4b265b886cc

📥 Commits

Reviewing files that changed from the base of the PR and between 4d27c7e and 1b6a5e4.

📒 Files selected for processing (1)
  • src/rdc/daemon_server.py

📝 Walkthrough

Walkthrough

Handlers for texture export/raw now accept an optional eid (defaulting to state.current_eid), convert it to int, call _set_frame_event(state, eid) before texture operations, and return JSON-RPC error -32002 if frame-event validation fails. Structured file retrieval now uses the adapter.

Changes

Cohort / File(s) Summary
Texture Export / Raw Handlers
src/rdc/handlers/texture.py
Handlers _handle_tex_export and _handle_tex_raw now read an eid param (default state.current_eid), convert to int, call _set_frame_event(state, eid) before saving, and return error -32002 on failure.
Daemon structured-file adapter
src/rdc/daemon_server.py
Replaced direct GetStructuredData() usage with state.adapter.get_structured_file() after RenderDocAdapter initialization in _load_replay and _load_remote_replay.
Unit Tests
tests/unit/test_binary_daemon.py
Added test_eid_out_of_range to TestTexExport and TestTexRaw asserting handlers return error -32002 for eid=99999.
CI Workflow
.github/workflows/ci.yml
Updated pip-audit invocation in security job to ignore CVE-2026-4539 via --ignore-vuln CVE-2026-4539.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Daemon as Daemon/RPC
    participant Handler as TextureHandler
    participant Adapter as RenderDocAdapter
    participant Replay as RenderDoc/Replay

    Client->>Daemon: tex_export / tex_raw request (params include optional eid)
    Daemon->>Handler: dispatch request
    Handler->>Handler: parse eid (or use state.current_eid) and int()
    Handler->>Adapter: _set_frame_event(state, eid)
    Adapter-->>Handler: ok / error
    alt ok
        Handler->>Adapter: controller.save_texture/write_raw(...)
        Adapter->>Replay: SaveTexture / WriteTextureData
        Replay-->>Adapter: result/data
        Adapter-->>Handler: saved / written
        Handler-->>Daemon: success response
        Daemon-->>Client: success
    else error
        Handler-->>Daemon: error (-32002)
        Daemon-->>Client: error (-32002)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through frames to find the right eid,
Set the event so each pixel could be freed,
Now exports land where they ought to be—
A rabbit's patch of textured glee! 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The CI workflow change (CVE-2026-4539 exclusion) is out of scope relative to the texture export fix objective and issue #206. Remove the CVE-2026-4539 exclusion from the CI workflow or address it in a separate pull request dedicated to security updates.
Docstring Coverage ⚠️ Warning Docstring coverage is 52.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title precisely describes the primary code change: adding SetFrameEvent calls before SaveTexture/GetTextureData operations in texture handlers.
Linked Issues check ✅ Passed All code changes directly address issue #206's root cause: SetFrameEvent is now called before texture operations to populate D3D11 texture tracking maps.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/tex-export-d3d11-frame-event

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@BANANASJIM
Copy link
Copy Markdown
Owner Author

Note: this fix can't be verified locally (no Windows D3D11 environment). @Junlin-Yin could you test with your setup and confirm?

@BANANASJIM
Copy link
Copy Markdown
Owner Author

Can't verify locally — no Windows/D3D11 setup. @Junlin-Yin could you test this?

Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)

59-60: The comment already includes an explicit removal trigger; consider adding a tracking issue or monitoring schedule for additional clarity.

The code correctly documents the suppression condition ("ignore until upstream ships a patch"). Verification confirms no patched release exists as of now, so the suppression is necessary. To prevent this from lingering indefinitely, consider adding a link to the upstream issue or a specific check-in date (e.g., "revisit 2026-06-01") in the comment so the team actively monitors for the next Pygments release.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 59 - 60, Update the CI suppression
comment for the pip-audit step that ignores CVE-2026-4539 (the commented line
and the run: uv run --with pip-audit pip-audit --ignore-vuln CVE-2026-4539) to
include a tracking link or revisit date; specifically, append either an upstream
Pygments issue/PR URL or a "revisit YYYY-MM-DD" note (e.g., revisit 2026-06-01)
to the existing comment so reviewers know when to re-evaluate the suppression
and where to monitor for a patch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 59-60: Update the CI suppression comment for the pip-audit step
that ignores CVE-2026-4539 (the commented line and the run: uv run --with
pip-audit pip-audit --ignore-vuln CVE-2026-4539) to include a tracking link or
revisit date; specifically, append either an upstream Pygments issue/PR URL or a
"revisit YYYY-MM-DD" note (e.g., revisit 2026-06-01) to the existing comment so
reviewers know when to re-evaluate the suppression and where to monitor for a
patch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f38df029-68fc-476e-8894-b801f1b5c9dc

📥 Commits

Reviewing files that changed from the base of the PR and between 66eb533 and 4d27c7e.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml

On Windows D3D11, cap.GetStructuredData() creates a temporary
WrappedID3D11Device whose destructor clears the static m_TextureList,
causing all subsequent SaveTexture calls to fail.

Use controller.GetStructuredFile() instead — same return type, no
destructive side effect.

Reported-by: Junlin-Yin
Fixes #206
Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@BANANASJIM BANANASJIM merged commit 72801bc into master Mar 31, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: SaveTexture always fails when executing "rdc texture tid -o /path/to/picture.png"

1 participant