Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset the queue state between each command buffer on queue submit #3589

Merged

Conversation

jleibs
Copy link
Contributor

@jleibs jleibs commented Mar 14, 2023

Additionally reset the state of SAMPLE_ALPHA_TO_COVERAGE as part of a queue reset.

The move of reset_state from outside to inside the loop matches the previous behavior here: https://github.com/gfx-rs/gfx/blob/master/src/backend/gl/src/queue.rs#L1142

Given the command encoder assumes a default state this seems to be the only safe behavior to avoiding leaking state between buffers.

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories

Description
This is necessary to avoid leaking of alpha-to-coverage between command buffers.

Testing
Confirmed alpha-to-coverage set on one pipeline was no longer leaked to the next in our application.

@jleibs jleibs marked this pull request as draft March 14, 2023 17:47
@jleibs jleibs force-pushed the jleibs/gles_alpha_to_coverage_reset_state branch from ad10306 to cb889eb Compare March 14, 2023 18:14
@jleibs jleibs marked this pull request as ready for review March 14, 2023 18:17
@jleibs jleibs changed the title Reset the state of SAMPLE_ALPHA_TO_COVERAGE on queue submit Reset the queue state between each command buffer on queue submit Mar 15, 2023
@emilk
Copy link
Contributor

emilk commented Mar 16, 2023

Build error is an unrelated:


error[vulnerability]: Race Condition Enabling Link Following and Time-of-check Time-of-use (TOCTOU)
    ┌─ /github/workspace/Cargo.lock:212:1
    │
212 │ remove_dir_all 0.5.3 registry+https://github.com/rust-lang/crates.io-index
    │ -------------------------------------------------------------------------- security vulnerability detected
    │
    = ID: RUSTSEC-2023-0018
    = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0018
    = The remove_dir_all crate is a Rust library that offers additional features over the Rust
      standard library fs::remove_dir_all function.

@cwfitzgerald cwfitzgerald added the PR: needs back-porting PR with a fix that needs to land on crates label Mar 20, 2023
@Wumpf
Copy link
Member

Wumpf commented Mar 20, 2023

needs back-porting

But this is the backport 😄. The non-backport is #3593
@jleibs can you maybe adjust title to signal that and put a link in? thanks!

@Wumpf
Copy link
Member

Wumpf commented Mar 22, 2023

bump @cwfitzgerald can haz merge? :)

@cwfitzgerald cwfitzgerald merged commit 63ede07 into gfx-rs:v0.15 Mar 22, 2023
@cwfitzgerald cwfitzgerald removed the PR: needs back-porting PR with a fix that needs to land on crates label Mar 22, 2023
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.

4 participants