Merge #1331

1331: Validate filtering r=cwfitzgerald a=kvark

**Connections**
See gpuweb/gpuweb#1223

**Description**
This PR implements validation of sampler filtering (property in BGL), as well as the ability to filter non-filterable textures.

**Testing**
Untested

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>

wgpu-core/src/binding_model.rs

@@ -43,6 +43,8 @@ pub enum CreateBindGroupLayoutError {
     TooManyBindings(BindingTypeMaxCountError),
 }
 
+//TODO: refactor this to move out `enum BindingError`.
+
 #[derive(Clone, Debug, Error)]
 pub enum CreateBindGroupError {
     #[error(transparent)]
@@ -130,8 +132,18 @@ pub enum CreateBindGroupError {
         layout_format: wgt::TextureFormat,
         view_format: wgt::TextureFormat,
     },
-    #[error("the given sampler is/is not a comparison sampler, while the layout type indicates otherwise")]
-    WrongSamplerComparison,
+    #[error("sampler binding {binding} expects comparison = {layout_cmp}, but given a sampler with comparison = {sampler_cmp}")]
+    WrongSamplerComparison {
+        binding: u32,
+        layout_cmp: bool,
+        sampler_cmp: bool,
+    },
+    #[error("sampler binding {binding} expects filtering = {layout_flt}, but given a sampler with filtering = {sampler_flt}")]
+    WrongSamplerFiltering {
+        binding: u32,
+        layout_flt: bool,
+        sampler_flt: bool,
+    },
     #[error("bound texture views can not have both depth and stencil aspects enabled")]
     DepthStencilAspect,
     #[error("the adapter does not support simultaneous read + write storage texture access for the format {0:?}")]

wgpu-core/src/device/mod.rs

@@ -940,6 +940,9 @@ impl<B: GfxBackend> Device<B> {
             Some(wgt::SamplerBorderColor::OpaqueWhite) => hal::image::BorderColor::OpaqueWhite,
         };
 
+        let filtering = [desc.min_filter, desc.mag_filter, desc.mipmap_filter]
+            .contains(&wgt::FilterMode::Linear);
+
         let info = hal::image::SamplerDesc {
             min_filter: conv::map_filter(desc.min_filter),
             mag_filter: conv::map_filter(desc.mag_filter),
@@ -975,6 +978,7 @@ impl<B: GfxBackend> Device<B> {
             },
             life_guard: LifeGuard::new(desc.label.borrow_or_default()),
             comparison: info.comparison.is_some(),
+            filtering,
         })
     }
 
@@ -1412,7 +1416,7 @@ impl<B: GfxBackend> Device<B> {
                 Br::Sampler(id) => {
                     match decl.ty {
                         wgt::BindingType::Sampler {
-                            filtering: _,
+                            filtering,
                             comparison,
                         } => {
                             let sampler = used
@@ -1422,7 +1426,19 @@ impl<B: GfxBackend> Device<B> {
 
                             // Check the actual sampler to also (not) be a comparison sampler
                             if sampler.comparison != comparison {
-                                return Err(Error::WrongSamplerComparison);
+                                return Err(Error::WrongSamplerComparison {
+                                    binding,
+                                    layout_cmp: 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,
+                                });
                             }
 
                             SmallVec::from([hal::pso::Descriptor::Sampler(&sampler.raw)])
@@ -2233,12 +2249,15 @@ impl<B: GfxBackend> Device<B> {
 
             if let Some(ref interface) = shader_module.interface {
                 let provided_layouts = match desc.layout {
-                    Some(pipeline_layout_id) => Some(Device::get_introspection_bind_group_layouts(
-                        pipeline_layout_guard
+                    Some(pipeline_layout_id) => {
+                        let pipeline_layout = pipeline_layout_guard
                             .get(pipeline_layout_id)
-                            .map_err(|_| pipeline::CreateRenderPipelineError::InvalidLayout)?,
-                        &*bgl_guard,
-                    )),
+                            .map_err(|_| pipeline::CreateRenderPipelineError::InvalidLayout)?;
+                        Some(Device::get_introspection_bind_group_layouts(
+                            pipeline_layout,
+                            &*bgl_guard,
+                        ))
+                    }
                     None => None,
                 };
 

wgpu-core/src/resource.rs

@@ -446,6 +446,8 @@ pub struct Sampler<B: hal::Backend> {
     pub(crate) life_guard: LifeGuard,
     /// `true` if this is a comparison sampler
     pub(crate) comparison: bool,
+    /// `true` if this is a filtering sampler
+    pub(crate) filtering: bool,
 }
 
 #[derive(Clone, Debug, Error)]

wgpu-core/src/validation.rs

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use crate::{binding_model::BindEntryMap, FastHashMap};
+use crate::{binding_model::BindEntryMap, FastHashMap, FastHashSet};
 use naga::valid::GlobalUse;
 use std::{collections::hash_map::Entry, fmt};
 use thiserror::Error;
@@ -25,8 +25,8 @@ enum ResourceType {
 
 #[derive(Debug)]
 struct Resource {
-    group: u32,
-    binding: u32,
+    name: Option<String>,
+    bind: naga::ResourceBinding,
     ty: ResourceType,
     class: naga::StorageClass,
 }
@@ -102,6 +102,7 @@ struct EntryPoint {
     outputs: Vec<Varying>,
     resources: Vec<(naga::Handle<Resource>, GlobalUse)>,
     spec_constants: Vec<SpecializationConstant>,
+    sampling_pairs: FastHashSet<(naga::Handle<Resource>, naga::Handle<Resource>)>,
 }
 
 #[derive(Debug)]
@@ -184,6 +185,14 @@ pub enum BindingError {
     BadStorageFormat(wgt::TextureFormat),
 }
 
+#[derive(Clone, Debug, Error)]
+pub enum FilteringError {
+    #[error("integer textures can't be sampled")]
+    Integer,
+    #[error("non-filterable float texture")]
+    NonFilterable,
+}
+
 #[derive(Clone, Debug, Error)]
 pub enum InputError {
     #[error("input is not provided by the earlier stage in the pipeline")]
@@ -201,12 +210,14 @@ pub enum StageError {
     InvalidModule,
     #[error("unable to find entry point '{0:?}'")]
     MissingEntryPoint(String),
-    #[error("error matching global binding at index {binding} in group {group} against the pipeline layout")]
-    Binding {
-        group: u32,
-        binding: u32,
+    #[error("error matching global {0:?} against the pipeline layout")]
+    Binding(naga::ResourceBinding, #[source] BindingError),
+    #[error("unable to filter the texture ({texture:?}) by the sampler ({sampler:?})")]
+    Filtering {
+        texture: naga::ResourceBinding,
+        sampler: naga::ResourceBinding,
         #[source]
-        error: BindingError,
+        error: FilteringError,
     },
     #[error(
         "error matching the stage input at {location} ({var}) against the previous stage outputs"
@@ -751,8 +762,8 @@ impl Interface {
         let mut resources = naga::Arena::new();
         let mut resource_mapping = FastHashMap::default();
         for (var_handle, var) in module.global_variables.iter() {
-            let (group, binding) = match var.binding {
-                Some(ref br) => (br.group, br.binding),
+            let bind = match var.binding {
+                Some(ref br) => br.clone(),
                 _ => continue,
             };
             let ty = match module.types[var.ty].inner {
@@ -779,8 +790,8 @@ impl Interface {
                 }
             };
             let handle = resources.append(Resource {
-                group,
-                binding,
+                name: var.name.clone(),
+                bind,
                 ty,
                 class: var.class,
             });
@@ -845,12 +856,13 @@ impl Interface {
             .get(&pair)
             .ok_or(StageError::MissingEntryPoint(pair.1))?;
 
+        // check resources visibility
         for &(handle, usage) in entry_point.resources.iter() {
             let res = &self.resources[handle];
             let result = match given_layouts {
                 Some(layouts) => layouts
-                    .get(res.group as usize)
-                    .and_then(|map| map.get(&res.binding))
+                    .get(res.bind.group as usize)
+                    .and_then(|map| map.get(&res.bind.binding))
                     .ok_or(BindingError::Missing)
                     .and_then(|entry| {
                         if entry.visibility.contains(stage_bit) {
@@ -861,11 +873,11 @@ impl Interface {
                     })
                     .and_then(|entry| res.check_binding_use(entry, usage)),
                 None => derived_layouts
-                    .get_mut(res.group as usize)
+                    .get_mut(res.bind.group as usize)
                     .ok_or(BindingError::Missing)
                     .and_then(|set| {
                         let ty = res.derive_binding_type(usage)?;
-                        match set.entry(res.binding) {
+                        match set.entry(res.bind.binding) {
                             Entry::Occupied(e) if e.get().ty != ty => {
                                 return Err(BindingError::InconsistentlyDerivedType)
                             }
@@ -874,7 +886,7 @@ impl Interface {
                             }
                             Entry::Vacant(e) => {
                                 e.insert(BindGroupLayoutEntry {
-                                    binding: res.binding,
+                                    binding: res.bind.binding,
                                     ty,
                                     visibility: stage_bit,
                                     count: None,
@@ -885,14 +897,52 @@ impl Interface {
                     }),
             };
             if let Err(error) = result {
-                return Err(StageError::Binding {
-                    group: res.group,
-                    binding: res.binding,
-                    error,
-                });
+                return Err(StageError::Binding(res.bind.clone(), error));
+            }
+        }
+
+        // check the compatibility between textures and samplers
+        if let Some(layouts) = given_layouts {
+            for &(texture_handle, sampler_handle) in entry_point.sampling_pairs.iter() {
+                let texture_bind = &self.resources[texture_handle].bind;
+                let sampler_bind = &self.resources[sampler_handle].bind;
+                let texture_layout = &layouts[texture_bind.group as usize][&texture_bind.binding];
+                let sampler_layout = &layouts[sampler_bind.group as usize][&sampler_bind.binding];
+                assert!(texture_layout.visibility.contains(stage_bit));
+                assert!(sampler_layout.visibility.contains(stage_bit));
+
+                let error = match texture_layout.ty {
+                    wgt::BindingType::Texture {
+                        sample_type: wgt::TextureSampleType::Float { filterable },
+                        ..
+                    } => match sampler_layout.ty {
+                        wgt::BindingType::Sampler {
+                            filtering: true, ..
+                        } if !filterable => Some(FilteringError::NonFilterable),
+                        _ => None,
+                    },
+                    wgt::BindingType::Texture {
+                        sample_type: wgt::TextureSampleType::Sint,
+                        ..
+                    }
+                    | wgt::BindingType::Texture {
+                        sample_type: wgt::TextureSampleType::Uint,
+                        ..
+                    } => Some(FilteringError::Integer),
+                    _ => None, // unreachable, really
+                };
+
+                if let Some(error) = error {
+                    return Err(StageError::Filtering {
+                        texture: texture_bind.clone(),
+                        sampler: sampler_bind.clone(),
+                        error,
+                    });
+                }
             }
         }
 
+        // check inputs compatibility
         for input in entry_point.inputs.iter() {
             match *input {
                 Varying::Local { location, ref iv } => {