-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Reject fragment shader output location
s > max_color_attachments
limit
#8316
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
3c0eeda
2e2d9b5
5f9d9bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,4 +7,5 @@ mod device; | |
mod experimental; | ||
mod external_texture; | ||
mod instance; | ||
mod render_pipeline; | ||
mod texture; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
//! Tests of [`wgpu::RenderPipeline`] and related. | ||
|
||
use wgpu_hal::MAX_COLOR_ATTACHMENTS; | ||
use wgpu_test::fail; | ||
|
||
#[test] | ||
fn reject_fragment_shader_output_over_max_color_attachments() { | ||
let (device, _queue) = wgpu::Device::noop(&Default::default()); | ||
|
||
// NOTE: Vertex shader is a boring quad. The fragment shader is the interesting part. | ||
let source = format!( | ||
"\ | ||
@vertex | ||
fn vert(@builtin(vertex_index) vertex_index : u32) -> @builtin(position) vec4f {{ | ||
var pos = array<vec2f, 3>( | ||
vec2(0.0, 0.5), | ||
vec2(-0.5, -0.5), | ||
vec2(0.5, -0.5) | ||
); | ||
return vec4f(pos[vertex_index], 0.0, 1.0); | ||
}} | ||
|
||
@fragment | ||
fn frag() -> @location({MAX_COLOR_ATTACHMENTS}) vec4f {{ | ||
return vec4(1.0, 0.0, 0.0, 1.0); | ||
}} | ||
" | ||
); | ||
|
||
let module = device.create_shader_module(wgpu::ShaderModuleDescriptor { | ||
label: None, | ||
source: wgpu::ShaderSource::Wgsl(source.into()), | ||
}); | ||
let module = &module; | ||
|
||
fail( | ||
&device, | ||
|| { | ||
device.create_render_pipeline(&wgpu::RenderPipelineDescriptor { | ||
layout: None, | ||
label: None, | ||
vertex: wgpu::VertexState { | ||
module, | ||
entry_point: None, | ||
compilation_options: Default::default(), | ||
buffers: &[], | ||
}, | ||
fragment: Some(wgpu::FragmentState { | ||
module, | ||
entry_point: None, | ||
compilation_options: Default::default(), | ||
targets: &[Some(wgpu::ColorTargetState { | ||
format: wgpu::TextureFormat::Rgba8Unorm, | ||
blend: None, | ||
write_mask: Default::default(), | ||
})], | ||
}), | ||
primitive: Default::default(), | ||
depth_stencil: None, | ||
multisample: Default::default(), | ||
multiview: None, | ||
cache: None, | ||
}) | ||
}, | ||
Some(concat!( | ||
"Location[8] Float32x4 interpolated as Some(Perspective) ", | ||
"with sampling Some(Center)'s index exceeds `max_color_attachments` (8)" | ||
)), | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,6 +313,11 @@ pub enum StageError { | |
MultipleEntryPointsFound, | ||
#[error(transparent)] | ||
InvalidResource(#[from] InvalidResourceError), | ||
#[error( | ||
"Location[{location}] {var}'s index exceeds `max_color_attachments` ({})", | ||
hal::MAX_COLOR_ATTACHMENTS | ||
)] | ||
ColorAttachmentLocationExceedsLimit { location: u32, var: InterfaceVar }, | ||
} | ||
|
||
impl WebGpuError for StageError { | ||
|
@@ -334,7 +339,8 @@ impl WebGpuError for StageError { | |
| Self::TooManyVaryings { .. } | ||
| Self::MissingEntryPoint(..) | ||
| Self::NoEntryPointFound | ||
| Self::MultipleEntryPointsFound => return ErrorType::Validation, | ||
| Self::MultipleEntryPointsFound | ||
| Self::ColorAttachmentLocationExceedsLimit { .. } => return ErrorType::Validation, | ||
}; | ||
e.webgpu_error_type() | ||
} | ||
|
@@ -1317,29 +1323,54 @@ impl Interface { | |
} | ||
} | ||
|
||
if shader_stage == naga::ShaderStage::Vertex { | ||
for output in entry_point.outputs.iter() { | ||
//TODO: count builtins towards the limit? | ||
inter_stage_components += match *output { | ||
Varying::Local { ref iv, .. } => iv.ty.dim.num_components(), | ||
Varying::BuiltIn(_) => 0, | ||
}; | ||
|
||
if let Some( | ||
cmp @ wgt::CompareFunction::Equal | cmp @ wgt::CompareFunction::NotEqual, | ||
) = compare_function | ||
{ | ||
if let Varying::BuiltIn(naga::BuiltIn::Position { invariant: false }) = *output | ||
match shader_stage { | ||
naga::ShaderStage::Vertex => { | ||
for output in entry_point.outputs.iter() { | ||
//TODO: count builtins towards the limit? | ||
inter_stage_components += match *output { | ||
Varying::Local { ref iv, .. } => iv.ty.dim.num_components(), | ||
Varying::BuiltIn(_) => 0, | ||
}; | ||
|
||
if let Some( | ||
cmp @ wgt::CompareFunction::Equal | cmp @ wgt::CompareFunction::NotEqual, | ||
) = compare_function | ||
{ | ||
log::warn!( | ||
"Vertex shader with entry point {entry_point_name} outputs a @builtin(position) without the @invariant \ | ||
attribute and is used in a pipeline with {cmp:?}. On some machines, this can cause bad artifacting as {cmp:?} assumes \ | ||
the values output from the vertex shader exactly match the value in the depth buffer. The @invariant attribute on the \ | ||
@builtin(position) vertex output ensures that the exact same pixel depths are used every render." | ||
); | ||
if let Varying::BuiltIn(naga::BuiltIn::Position { invariant: false }) = | ||
*output | ||
{ | ||
log::warn!( | ||
concat!( | ||
"Vertex shader with entry point {} outputs a ", | ||
"@builtin(position) without the @invariant attribute and ", | ||
"is used in a pipeline with {cmp:?}. On some machines, ", | ||
"this can cause bad artifacting as {cmp:?} assumes the ", | ||
"values output from the vertex shader exactly match the ", | ||
"value in the depth buffer. The @invariant attribute on the ", | ||
"@builtin(position) vertex output ensures that the exact ", | ||
"same pixel depths are used every render." | ||
), | ||
entry_point_name, | ||
cmp = cmp | ||
); | ||
} | ||
} | ||
} | ||
} | ||
naga::ShaderStage::Fragment => { | ||
for output in &entry_point.outputs { | ||
let &Varying::Local { location, ref iv } = output else { | ||
continue; | ||
}; | ||
if location >= self.limits.max_color_attachments { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is true today that this will always be equal to There's also a question whether it is an error for an output that is not consumed by the pipeline to be placed at a location above |
||
return Err(StageError::ColorAttachmentLocationExceedsLimit { | ||
location, | ||
var: iv.clone(), | ||
}); | ||
} | ||
} | ||
} | ||
_ => (), | ||
} | ||
|
||
if inter_stage_components > self.limits.max_inter_stage_shader_components { | ||
|
@@ -1357,6 +1388,7 @@ impl Interface { | |
Varying::BuiltIn(_) => None, | ||
}) | ||
.collect(); | ||
|
||
Ok(outputs) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name bikeshedding: I would prefer
...TooLarge
or...OutOfBounds
(strings that we already use in error variants) overExceedsLimit
. (Although I do see we haveExceededLimit...
appearing in one place.)I also might be inclined to make the name general enough to cover the case that the location lands on a gap between defined color targets (e.g.
InvalidColorAttachmentLocation
), although the primary responsibility for checking that seems to be later increate_render_pipeline
(where it presently only generates alog::warn!
), so maybe it's not important.