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

Don't crash if a texture is destroyed before queue submission #5028

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

nical
Copy link
Contributor

@nical nical commented Jan 9, 2024

Connections

Another crash found while running the CTS.

Description

If texture.destroy() is called between the recording of commands and queue.submit() it causes the memory initialization code to produce an error which is unwraped and panics.
This PR addresses it by checking for the DestroyedTexture error specifically after clearing the texture and propagating it. Other errors are still considered unexpected.

Testing

covered by the CTS.

Checklist

  • Run cargo fmt.
  • Run cargo clippy.
  • Run cargo xtask test to run tests.

@nical nical requested a review from a team as a code owner January 9, 2024 14:29
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Code looks good - it would be nice to get a test that tests this for buffers and textures. While it's covered by the cts, this feels super easy to accidentally break by adding some other code that doesn't properly expect the destroyed state

@nical
Copy link
Contributor Author

nical commented Jan 9, 2024

A test is indeed easy to add

#[gpu_test]
static TEXTURE_DESTROY_BEFORE_SUBMIT: GpuTestConfiguration = GpuTestConfiguration::new().run_sync(|ctx| {
    let descriptor = wgpu::TextureDescriptor {
        label: None,
        size: wgpu::Extent3d {
            width: 128,
            height: 128,
            depth_or_array_layers: 1,
        },
        mip_level_count: 1,
        sample_count: 1, // multisampling is not supported for clear
        dimension: wgpu::TextureDimension::D2,
        format: wgpu::TextureFormat::Rgba8Snorm,
        usage: wgpu::TextureUsages::COPY_DST | wgpu::TextureUsages::COPY_SRC | wgpu::TextureUsages::TEXTURE_BINDING,
        view_formats: &[],
    };

    let texture_1 = ctx.device.create_texture(&descriptor);
    let texture_2 = ctx.device.create_texture(&descriptor);

    let mut encoder = ctx.device.create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
    encoder.copy_texture_to_texture(
        wgpu::ImageCopyTexture {
            texture: &texture_1,
            mip_level: 0,
            origin: wgpu::Origin3d::ZERO,
            aspect: wgpu::TextureAspect::All,
        },
        wgpu::ImageCopyTexture {
            texture: &texture_2,
            mip_level: 0,
            origin: wgpu::Origin3d::ZERO,
            aspect: wgpu::TextureAspect::All,            
        },
        wgpu::Extent3d {
            width: 128,
            height: 128,
            depth_or_array_layers: 1,
        }
    );

    texture_1.destroy();
    texture_2.destroy();

    fail(&ctx.device, || {
        ctx.queue.submit([encoder.finish()]);
    });
});

However queue.submit at the wgpu level panics on errors. The API would need to be changed to return a Result<SubmissionIndex, SomeError> or Option<SubmissionIndex> .

How does that sound?

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jan 9, 2024

This is a byproduct of the whole submission index thing being a bad design. To be consistent we need to make the submission index hold an Option<u64> on the direct backend, make poll a no-op if the submission index is None, then send all submit errors down the error sync instead of calling fatal_error

This sucks, but the futures api is a better one, so will be resolved when we can figure that out.

Could we also get a test for buffers, if you don't mind?

@nical
Copy link
Contributor Author

nical commented Jan 10, 2024

Alright, I filed #5031 as a followup so we can take the crash fix now. I hope to get to it soonish.

@nical nical merged commit f27bb44 into gfx-rs:trunk Jan 10, 2024
27 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.

2 participants