From 3545a4fdd5e2b7e86552e0ca127063b8466a3031 Mon Sep 17 00:00:00 2001 From: Boris-Chengbiao Zhou Date: Wed, 17 Nov 2021 05:08:03 +0100 Subject: [PATCH] Introduce SamplerBindingType enum and fix bug in validation (#2181) * Introduce SamplerBindingType enum and fix bug in validation This matches the WebGPU spec more closely and also lets us implement the validation as it's written in the spec[1]. This fixes a bug which previously prevented the "shadow" example from running in Firefox. Previously the validation would check whether the filtering was appropiate even if it was a comparison sampler. This didn't matter when running the shadow example natively since the example was setting `filtering: true` in addition to `comparison: true`. But this failed when running through a browser since there the used WebIDL has the proper enum representation and eventually resulted in `filtering: false` being passed to wgpu-core which would then fail validation.[2] [1]: https://gpuweb.github.io/gpuweb/#bind-group-creation [2]: https://github.com/mozilla/gecko-dev/blob/674b6582bafedc11fba6dc537769c88e9ed921d3/dom/webgpu/ipc/WebGPUChild.cpp#L502-L513 * Fix remaining examples * Fix deno_webgpu --- deno_webgpu/src/binding.rs | 31 ++------------------ wgpu-core/src/device/mod.rs | 35 +++++++++++++---------- wgpu-core/src/validation.rs | 24 ++++++++-------- wgpu-hal/examples/halmark/main.rs | 5 +--- wgpu-types/src/lib.rs | 30 ++++++++++++------- wgpu/examples/bunnymark/main.rs | 5 +--- wgpu/examples/conservative-raster/main.rs | 5 +--- wgpu/examples/shadow/main.rs | 5 +--- wgpu/examples/skybox/main.rs | 5 +--- wgpu/examples/texture-arrays/main.rs | 5 +--- wgpu/examples/water/main.rs | 5 +--- wgpu/src/backend/web.rs | 19 ++++++------ wgpu/src/lib.rs | 15 +++++----- 13 files changed, 80 insertions(+), 109 deletions(-) diff --git a/deno_webgpu/src/binding.rs b/deno_webgpu/src/binding.rs index 8ed6447466..68915c3531 100644 --- a/deno_webgpu/src/binding.rs +++ b/deno_webgpu/src/binding.rs @@ -56,34 +56,7 @@ impl From for wgpu_types::BufferBindingType { #[derive(Deserialize)] #[serde(rename_all = "camelCase")] struct GpuSamplerBindingLayout { - r#type: GpuSamplerBindingType, -} - -#[derive(Deserialize)] -#[serde(rename_all = "kebab-case")] -enum GpuSamplerBindingType { - Filtering, - NonFiltering, - Comparison, -} - -impl From for wgpu_types::BindingType { - fn from(binding_type: GpuSamplerBindingType) -> Self { - match binding_type { - GpuSamplerBindingType::Filtering => wgpu_types::BindingType::Sampler { - filtering: true, - comparison: false, - }, - GpuSamplerBindingType::NonFiltering => wgpu_types::BindingType::Sampler { - filtering: false, - comparison: false, - }, - GpuSamplerBindingType::Comparison => wgpu_types::BindingType::Sampler { - filtering: true, - comparison: true, - }, - } - } + r#type: wgpu_types::SamplerBindingType, } #[derive(Deserialize)] @@ -170,7 +143,7 @@ impl TryFrom for wgpu_types::BindingType { has_dynamic_offset: buffer.has_dynamic_offset, min_binding_size: std::num::NonZeroU64::new(buffer.min_binding_size), }, - GpuBindingType::Sampler(sampler) => sampler.r#type.into(), + GpuBindingType::Sampler(sampler) => wgpu_types::BindingType::Sampler(sampler.r#type), GpuBindingType::Texture(texture) => wgpu_types::BindingType::Texture { sample_type: texture.sample_type.into(), view_dimension: texture.view_dimension, diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index c67e925010..50f9ce8f00 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -1521,31 +1521,36 @@ impl Device { } Br::Sampler(id) => { match decl.ty { - wgt::BindingType::Sampler { - filtering, - comparison, - } => { + wgt::BindingType::Sampler(ty) => { let sampler = used .samplers .use_extend(&*sampler_guard, id, (), ()) .map_err(|_| Error::InvalidSampler(id))?; - // Check the actual sampler to also (not) be a comparison sampler - if sampler.comparison != comparison { + // Allowed sampler values for filtering and comparison + let (allowed_filtering, allowed_comparison) = match ty { + wgt::SamplerBindingType::Filtering => (None, false), + wgt::SamplerBindingType::NonFiltering => (Some(false), false), + wgt::SamplerBindingType::Comparison => (None, true), + }; + + if let Some(allowed_filtering) = allowed_filtering { + if allowed_filtering != sampler.filtering { + return Err(Error::WrongSamplerFiltering { + binding, + layout_flt: allowed_filtering, + sampler_flt: sampler.filtering, + }); + } + } + + if allowed_comparison != sampler.comparison { return Err(Error::WrongSamplerComparison { binding, - layout_cmp: comparison, + layout_cmp: allowed_comparison, sampler_cmp: sampler.comparison, }); } - // Check the actual sampler to be non-filtering, if required - if sampler.filtering && !filtering { - return Err(Error::WrongSamplerFiltering { - binding, - layout_flt: filtering, - sampler_flt: sampler.filtering, - }); - } let res_index = hal_samplers.len(); hal_samplers.push(&sampler.raw); diff --git a/wgpu-core/src/validation.rs b/wgpu-core/src/validation.rs index 7d1f8a3210..afbc26d42a 100644 --- a/wgpu-core/src/validation.rs +++ b/wgpu-core/src/validation.rs @@ -391,11 +391,8 @@ impl Resource { allowed_usage } ResourceType::Sampler { comparison } => match entry.ty { - BindingType::Sampler { - filtering: _, - comparison: cmp, - } => { - if cmp == comparison { + BindingType::Sampler(ty) => { + if (ty == wgt::SamplerBindingType::Comparison) == comparison { GlobalUse::READ } else { return Err(BindingError::WrongSamplerComparison); @@ -536,10 +533,11 @@ impl Resource { has_dynamic_offset: false, min_binding_size: Some(size), }, - ResourceType::Sampler { comparison } => BindingType::Sampler { - filtering: true, - comparison, - }, + ResourceType::Sampler { comparison } => BindingType::Sampler(if comparison { + wgt::SamplerBindingType::Comparison + } else { + wgt::SamplerBindingType::Filtering + }), ResourceType::Texture { dim, arrayed, @@ -1031,9 +1029,11 @@ impl Interface { sample_type: wgt::TextureSampleType::Float { filterable }, .. } => match sampler_layout.ty { - wgt::BindingType::Sampler { - filtering: true, .. - } if !filterable => Some(FilteringError::NonFilterable), + wgt::BindingType::Sampler(wgt::SamplerBindingType::Filtering) + if !filterable => + { + Some(FilteringError::NonFilterable) + } _ => None, }, wgt::BindingType::Texture { diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index 45b9b3ba86..30f0982110 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -177,10 +177,7 @@ impl Example { wgt::BindGroupLayoutEntry { binding: 2, visibility: wgt::ShaderStages::FRAGMENT, - ty: wgt::BindingType::Sampler { - filtering: true, - comparison: false, - }, + ty: wgt::BindingType::Sampler(wgt::SamplerBindingType::Filtering), count: None, }, ], diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 9f1983a4f4..dafa8d1cc1 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -3194,6 +3194,25 @@ pub enum StorageTextureAccess { ReadWrite, } +/// Specific type of a sampler binding. +/// +/// WebGPU spec: +#[repr(C)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] +#[cfg_attr(feature = "trace", derive(Serialize))] +#[cfg_attr(feature = "replay", derive(Deserialize))] +#[cfg_attr(feature = "serde", serde(rename_all = "kebab-case"))] +pub enum SamplerBindingType { + /// The sampling result is produced based on more than a single color sample from a texture, + /// e.g. when bilinear interpolation is enabled. + Filtering, + /// The sampling result is produced based on a single color sample from a texture. + NonFiltering, + /// Use as a comparison sampler instead of a normal sampler. + /// For more info take a look at the analogous functionality in OpenGL: . + Comparison, +} + /// Specific type of a binding. /// /// WebGPU spec: the enum of @@ -3227,16 +3246,7 @@ pub enum BindingType { /// layout(binding = 0) /// uniform sampler s; /// ``` - Sampler { - /// The sampling result is produced based on more than a single color sample from a texture, - /// e.g. when bilinear interpolation is enabled. - /// - /// A filtering sampler can only be used with a filterable texture. - filtering: bool, - /// Use as a comparison sampler instead of a normal sampler. - /// For more info take a look at the analogous functionality in OpenGL: . - comparison: bool, - }, + Sampler(SamplerBindingType), /// A texture binding. /// /// Example GLSL syntax: diff --git a/wgpu/examples/bunnymark/main.rs b/wgpu/examples/bunnymark/main.rs index b99cc7fdad..b7aa71a78a 100644 --- a/wgpu/examples/bunnymark/main.rs +++ b/wgpu/examples/bunnymark/main.rs @@ -77,10 +77,7 @@ impl framework::Example for Example { wgpu::BindGroupLayoutEntry { binding: 2, visibility: wgpu::ShaderStages::FRAGMENT, - ty: wgpu::BindingType::Sampler { - filtering: true, - comparison: false, - }, + ty: wgpu::BindingType::Sampler(wgpu::SamplerBindingType::Filtering), count: None, }, ], diff --git a/wgpu/examples/conservative-raster/main.rs b/wgpu/examples/conservative-raster/main.rs index b6467e71fe..40683fdb91 100644 --- a/wgpu/examples/conservative-raster/main.rs +++ b/wgpu/examples/conservative-raster/main.rs @@ -183,10 +183,7 @@ impl framework::Example for Example { wgpu::BindGroupLayoutEntry { binding: 1, visibility: wgpu::ShaderStages::FRAGMENT, - ty: wgpu::BindingType::Sampler { - filtering: false, - comparison: false, - }, + ty: wgpu::BindingType::Sampler(wgpu::SamplerBindingType::NonFiltering), count: None, }, ], diff --git a/wgpu/examples/shadow/main.rs b/wgpu/examples/shadow/main.rs index 8a22c8fe99..325e9d36a8 100644 --- a/wgpu/examples/shadow/main.rs +++ b/wgpu/examples/shadow/main.rs @@ -580,10 +580,7 @@ impl framework::Example for Example { wgpu::BindGroupLayoutEntry { binding: 3, visibility: wgpu::ShaderStages::FRAGMENT, - ty: wgpu::BindingType::Sampler { - comparison: true, - filtering: true, - }, + ty: wgpu::BindingType::Sampler(wgpu::SamplerBindingType::Comparison), count: None, }, ], diff --git a/wgpu/examples/skybox/main.rs b/wgpu/examples/skybox/main.rs index 03d3cc702b..cb0108d453 100644 --- a/wgpu/examples/skybox/main.rs +++ b/wgpu/examples/skybox/main.rs @@ -166,10 +166,7 @@ impl framework::Example for Skybox { wgpu::BindGroupLayoutEntry { binding: 2, visibility: wgpu::ShaderStages::FRAGMENT, - ty: wgpu::BindingType::Sampler { - comparison: false, - filtering: true, - }, + ty: wgpu::BindingType::Sampler(wgpu::SamplerBindingType::Filtering), count: None, }, ], diff --git a/wgpu/examples/texture-arrays/main.rs b/wgpu/examples/texture-arrays/main.rs index b126b7a8ad..ef2ab2add6 100644 --- a/wgpu/examples/texture-arrays/main.rs +++ b/wgpu/examples/texture-arrays/main.rs @@ -188,10 +188,7 @@ impl framework::Example for Example { wgpu::BindGroupLayoutEntry { binding: 1, visibility: wgpu::ShaderStages::FRAGMENT, - ty: wgpu::BindingType::Sampler { - comparison: false, - filtering: true, - }, + ty: wgpu::BindingType::Sampler(wgpu::SamplerBindingType::Filtering), count: None, }, ], diff --git a/wgpu/examples/water/main.rs b/wgpu/examples/water/main.rs index 81fc24497c..8b1bd57a5c 100644 --- a/wgpu/examples/water/main.rs +++ b/wgpu/examples/water/main.rs @@ -391,10 +391,7 @@ impl framework::Example for Example { wgpu::BindGroupLayoutEntry { binding: 3, visibility: wgpu::ShaderStages::FRAGMENT, - ty: wgpu::BindingType::Sampler { - comparison: false, - filtering: true, - }, + ty: wgpu::BindingType::Sampler(wgpu::SamplerBindingType::Filtering), count: None, }, ], diff --git a/wgpu/src/backend/web.rs b/wgpu/src/backend/web.rs index 42cbb73556..be06debff4 100644 --- a/wgpu/src/backend/web.rs +++ b/wgpu/src/backend/web.rs @@ -1283,15 +1283,18 @@ impl crate::Context for Context { }); mapped_entry.buffer(&buffer); } - wgt::BindingType::Sampler { - comparison, - filtering, - } => { + wgt::BindingType::Sampler(ty) => { let mut sampler = web_sys::GpuSamplerBindingLayout::new(); - sampler.type_(match (comparison, filtering) { - (false, false) => web_sys::GpuSamplerBindingType::NonFiltering, - (false, true) => web_sys::GpuSamplerBindingType::Filtering, - (true, _) => web_sys::GpuSamplerBindingType::Comparison, + sampler.type_(match ty { + wgt::SamplerBindingType::NonFiltering => { + web_sys::GpuSamplerBindingType::NonFiltering + } + wgt::SamplerBindingType::Filtering => { + web_sys::GpuSamplerBindingType::Filtering + } + wgt::SamplerBindingType::Comparison => { + web_sys::GpuSamplerBindingType::Comparison + } }); mapped_entry.sampler(&sampler); } diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 6bb078bc01..495a722ae7 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -32,13 +32,14 @@ pub use wgt::{ DynamicOffset, Extent3d, Face, Features, FilterMode, FrontFace, ImageDataLayout, ImageSubresourceRange, IndexFormat, Limits, MultisampleState, Origin3d, PipelineStatisticsTypes, PolygonMode, PowerPreference, PresentMode, PrimitiveState, - PrimitiveTopology, PushConstantRange, QueryType, RenderBundleDepthStencil, SamplerBorderColor, - ShaderLocation, ShaderModel, ShaderStages, StencilFaceState, StencilOperation, StencilState, - StorageTextureAccess, SurfaceConfiguration, SurfaceStatus, TextureAspect, TextureDimension, - TextureFormat, TextureFormatFeatureFlags, TextureFormatFeatures, TextureSampleType, - TextureUsages, TextureViewDimension, VertexAttribute, VertexFormat, VertexStepMode, - COPY_BUFFER_ALIGNMENT, COPY_BYTES_PER_ROW_ALIGNMENT, MAP_ALIGNMENT, PUSH_CONSTANT_ALIGNMENT, - QUERY_RESOLVE_BUFFER_ALIGNMENT, QUERY_SET_MAX_QUERIES, QUERY_SIZE, VERTEX_STRIDE_ALIGNMENT, + PrimitiveTopology, PushConstantRange, QueryType, RenderBundleDepthStencil, SamplerBindingType, + SamplerBorderColor, ShaderLocation, ShaderModel, ShaderStages, StencilFaceState, + StencilOperation, StencilState, StorageTextureAccess, SurfaceConfiguration, SurfaceStatus, + TextureAspect, TextureDimension, TextureFormat, TextureFormatFeatureFlags, + TextureFormatFeatures, TextureSampleType, TextureUsages, TextureViewDimension, VertexAttribute, + VertexFormat, VertexStepMode, COPY_BUFFER_ALIGNMENT, COPY_BYTES_PER_ROW_ALIGNMENT, + MAP_ALIGNMENT, PUSH_CONSTANT_ALIGNMENT, QUERY_RESOLVE_BUFFER_ALIGNMENT, QUERY_SET_MAX_QUERIES, + QUERY_SIZE, VERTEX_STRIDE_ALIGNMENT, }; use backend::{BufferMappedRange, Context as C};