Make sure to unset current context in wgl Surface::configure/present #5087
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Imberflur
force-pushed
the
wgl-fix-current
branch
from
690ac23
to
9d03125
Compare
…esent and Surface::configure of wgl backend. So that we don't fail to set the context on other threads.
Imberflur
force-pushed
the
wgl-fix-current
branch
from
9d03125
to
dd2229a
Compare
Imberflur
force-pushed
the
wgl-fix-current
branch
from
dd2229a
to
50ddff9
Compare
cwfitzgerald
added
the
PR: needs back-porting
cwfitzgerald
pushed a commit
to cwfitzgerald/wgpu
that referenced
this pull request
Jan 21, 2024
cwfitzgerald
removed
the
PR: needs back-porting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
None known
Description
When re-building pipelines in background threads on the wgl backend we encounted this panic:
which is produced by the unwrap when making a context current fails here
wgpu/wgpu-hal/src/gles/wgl.rs
Line 76 in 7774f31
I tracked this down to Surface::configure and Surface::present not unsetting the current context after setting it even though they release the associated lock used for synchronizing this.
To fix this I took advantage of the existing guard type and added a new locking make current method (called
lock_with_dcthat allows passing in the DC that Surface::configure/Surface::present were previously passing directly tomake_current.Note that with this guard type, if unmake_current fails it will panic. I wasn't completely sure if this was appropriate for Surface::configure/present since they return errors instead of panicking when make_current fails. However, if unmake_current fails it seems like later calls to make_current (if called on a different thread) would also fail causing
AdapterContext::lockto panic anyway? I'm not really familiar with what would cause unmake_current to fail though and what the behavior would be in terms of whether make_current can succeed later after such a failure.Note, another small behavior change is that this uses the same
.try_lock_for(Duration::from_secs(CONTEXT_LOCK_TIMEOUT_SECS))pattern instead of just a.lock().Testing
To reproduce this I spawned an extra thread in the examples
framework.rsthat calledAdapter::get_texture_format_featuresin a loop. This consistently produced panics when trying to make the gl context current to that thread. After this change, no panics were observed with this testing setup. Additionally, the original issue when re-building pipelines seems to be resolved.Checklist
cargo fmt.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.