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

Make it possible to filter labels out ahead of wgpu-hal #4246

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

nical
Copy link
Contributor

@nical nical commented Oct 16, 2023

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

This is intended as a hardening feature. If we don't trust the author of labels (for example web content) and don't trust drivers to handle long or non-ascii strings, an instance flags can be set to filter out hal labels. When the flag is set, wgpu-core can still hold on to labels but they are not passed down to GPU drivers.

Most people should not enable this flag. Labels are pretty useful for debugging with renderdoc and similar tools.

@nical
Copy link
Contributor Author

nical commented Oct 16, 2023

There are still a couple of internal labels that are missed (because the flag was not easily reachable). They are less important for hardening since they short and ascii-only, but we can look into filtering them as well for consistency.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

I believe this PR is missing fixes for the following labels:

  • wgpu_hal::RenderPassDescriptor::label, passed in wgpu_core::render::RenderPassInfo::start
  • wgpu_types::QuerySetDescriptor::label, passed in wgpu_core::Device::create_query_set - although this is not currently used by Firefox (GPUDevice.createQuerySet is commented out in the WebIDL), we probably should address it now
  • wgpu_types::RenderBundleDescriptor::label, which RenderBundle::execute passes to wgpu_hal::CommandEncoder::begin_debug_marker
  • It seems like wgpu_hal::CommandEncoder::begin_encoding gets passed wgpu_core::command::CommandEncoder::label, which it seems can be set from wgpu_types::CommandEncoderDescriptor::label.
  • Global::command_encoder_run_render_pass_impl takes a BasePassRef as an argument whose label field is passed to wgpu_core::command::CommandEncoder::open_pass, which passes it to wgpu_hal::CommandEncoder::begin_encoding.
  • wgpu_hal::CommandEncoder::insert_debug_marker seems to take labels from compute pass encoders, and is also called from Gloal::command_encoder_insert_debug_marker. I don't know why we have two ways of doing this, but they both seem reachable from Firefox.
  • It seems like wgpu_hal::CommandEncoder::begin_debug_marker is also reachable.

wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

jimblandy commented Oct 16, 2023

I marked this as "changes requested", but the PR as it stands is a step in the right direction. If you'd like to address the cases I found in a follow-up, that's fine with me.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

I came across a few missed labels, and some problems with string processing in render passes and compute passes. I've added a fixup commit; please look it over.

wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/render.rs Outdated Show resolved Hide resolved
@nical nical merged commit 8c03aa8 into gfx-rs:trunk Oct 18, 2023
23 checks passed
@nical nical deleted the labels2 branch October 18, 2023 15:46
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.

3 participants