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.

Sign up for GitHub

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

Jump to bottom

[wgsl-in] Ensure textureSampleLevel's level argument is an integer for depth textures #6529

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
+102 −15
Open

[wgsl-in] Ensure textureSampleLevel's level argument is an integer for depth textures #6529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion naga/src/back/spv/image.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,43 @@ impl<'w> BlockContext<'w> {
depth_id,
);

let lod_id = self.cached[lod_handle];
let mut lod_id = self.cached[lod_handle];
// SPIR-V expects the LOD to be a float for all image classes.
// lod_id, however, will be an integer for depth images,
// therefore we must do a conversion.
if matches!(
self.ir_module.types[image_type].inner,
crate::TypeInner::Image {
class: crate::ImageClass::Depth { .. },
..
}
) {
let lod_f32_id = self.gen_id();
let f32_type_id = self.get_type_id(LookupType::Local(LocalType::Numeric(
NumericType::Scalar(crate::Scalar::F32),
)));
let convert_op = match *self.fun_info[lod_handle]
.ty
.inner_with(&self.ir_module.types)
{
crate::TypeInner::Scalar(crate::Scalar {
kind: crate::ScalarKind::Uint,
width: 4,
}) => spirv::Op::ConvertUToF,
crate::TypeInner::Scalar(crate::Scalar {
kind: crate::ScalarKind::Sint,
width: 4,
}) => spirv::Op::ConvertSToF,
_ => unreachable!(),
};
block.body.push(Instruction::unary(
convert_op,
f32_type_id,
lod_f32_id,
lod_id,
));
lod_id = lod_f32_id;
teoxoy marked this conversation as resolved.
Show resolved Hide resolved
}
mask |= spirv::ImageOperands::LOD;
inst.add_operand(mask.bits());
inst.add_operand(lod_id);
Expand Down
27 changes: 27 additions & 0 deletions naga/src/front/spv/image.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ impl<'function> super::BlockContext<'function> {
match self.expressions[handle] {
crate::Expression::GlobalVariable(handle) => Ok(self.global_arena[handle].ty),
crate::Expression::FunctionArgument(i) => Ok(self.arguments[i as usize].ty),
crate::Expression::Access { base, .. } => Ok(self.get_image_expr_ty(base)?),
ref other => Err(Error::InvalidImageExpression(other.clone())),
}
}
Expand Down Expand Up @@ -460,6 +461,7 @@ impl<I: Iterator<Item = u32>> super::Frontend<I> {
} else {
None
};
let span = self.span_from_with_op(start);

let mut image_ops = if words_left != 0 {
words_left -= 1;
Expand All @@ -486,9 +488,34 @@ impl<I: Iterator<Item = u32>> super::Frontend<I> {
let lod_lexp = self.lookup_expression.lookup(lod_expr)?;
let lod_handle =
self.get_expr_handle(lod_expr, lod_lexp, ctx, emitter, block, body_idx);

let is_depth_image = {
let image_lexp = self.lookup_sampled_image.lookup(sampled_image_id)?;
let image_ty = ctx.get_image_expr_ty(image_lexp.image)?;
matches!(
ctx.type_arena[image_ty].inner,
crate::TypeInner::Image {
class: crate::ImageClass::Depth { .. },
..
}
)
};

level = if options.compare {
log::debug!("Assuming {:?} is zero", lod_handle);
crate::SampleLevel::Zero
} else if is_depth_image {
log::debug!(
"Assuming level {:?} converts losslessly to an integer",
lod_handle
);
let expr = crate::Expression::As {
expr: lod_handle,
kind: crate::ScalarKind::Sint,
convert: Some(4),
};
Comment on lines +508 to +516
Copy link
Member

@teoxoy teoxoy Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that if we round it first this will behave the same as the sampler doing it. Thoughts?

Copy link
Contributor Author

@jamienicol jamienicol Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My assumption was that a fractional lod interpolated between two mip levels. But I can't see anything in the spec which says that (but nor that it rounds the lod). Do you happen to know what the spec says?

Copy link
Member

Choose a reason for hiding this comment

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

Depth textures can only be paired with a non-filtering (or comparison) sampler which means that the mipmapFilter field of the sampler descriptor can't be linear, it must be nearest which sounds like it rounds instead.

A few links:

https://www.w3.org/TR/webgpu/#ref-for-dom-gpusamplerbindingtype-filtering%E2%91%A2
https://www.w3.org/TR/webgpu/#dom-gpusamplerbindingtype-non-filtering
https://www.w3.org/TR/webgpu/#ref-for-dom-gpusampler-isfiltering-slot
https://www.w3.org/TR/webgpu/#enumdef-gpumipmapfiltermode

let s32_lod_handle = ctx.expressions.append(expr, span);
crate::SampleLevel::Exact(s32_lod_handle)
} else {
crate::SampleLevel::Exact(lod_handle)
};
Expand Down
25 changes: 19 additions & 6 deletions naga/src/valid/expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ pub enum ExpressionError {
InvalidGatherComponent(crate::SwizzleComponent),
#[error("Gather can't be done for image dimension {0:?}")]
InvalidGatherDimension(crate::ImageDimension),
#[error("Sample level (exact) type {0:?} is not a scalar float")]
#[error("Sample level (exact) type {0:?} has an invalid type")]
InvalidSampleLevelExactType(Handle<crate::Expression>),
#[error("Sample level (bias) type {0:?} is not a scalar float")]
InvalidSampleLevelBiasType(Handle<crate::Expression>),
Expand Down Expand Up @@ -530,11 +530,24 @@ impl super::Validator {
crate::SampleLevel::Auto => ShaderStages::FRAGMENT,
crate::SampleLevel::Zero => ShaderStages::all(),
crate::SampleLevel::Exact(expr) => {
match resolver[expr] {
Ti::Scalar(Sc {
kind: Sk::Float, ..
}) => {}
_ => return Err(ExpressionError::InvalidSampleLevelExactType(expr)),
match class {
crate::ImageClass::Depth { .. } => match resolver[expr] {
Ti::Scalar(Sc {
kind: Sk::Sint | Sk::Uint,
..
}) => {}
_ => {
return Err(ExpressionError::InvalidSampleLevelExactType(expr))
}
},
Comment on lines +534 to +542
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would modify the GLSL & SPIR-V frontends as well to not regress. I think we need to round the lod then cast it to a u32 so that the behavior is in line with a non-filtering sampler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think any GLSL command can be converted to a depth texture textureSampleLevel(). GLSL's textureLod() with a shadow texture takes a vector with an extra component for the coords, and this component is used as the compare value when translated to textureSampleCompareLevel().

textureSampleCompareLevel always has a LOD of zero, so the LOD specified in the GLSL also appears to be lost in translation. I think that occurs here. So as I can tell I don't think there's anything to do for GLSL, but this is all very new to me so could be wrong!

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the GLSL frontend, that seems accurate. The SPIR-V frontend can produce a textureSampleLevel() with a depth texture though.

_ => match resolver[expr] {
Ti::Scalar(Sc {
kind: Sk::Float, ..
}) => {}
_ => {
return Err(ExpressionError::InvalidSampleLevelExactType(expr))
}
},
}
ShaderStages::all()
}
Expand Down
4 changes: 3 additions & 1 deletion naga/tests/in/image.wgsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,9 @@ fn gather() -> @location(0) vec4<f32> {
@fragment
fn depth_no_comparison() -> @location(0) vec4<f32> {
let tc = vec2<f32>(0.5);
let level = 1;
let s2d = textureSample(image_2d_depth, sampler_reg, tc);
let s2d_gather = textureGather(image_2d_depth, sampler_reg, tc);
return s2d + s2d_gather;
let s2d_level = textureSampleLevel(image_2d_depth, sampler_reg, tc, level);
return s2d + s2d_gather + s2d_level;
}
3 changes: 2 additions & 1 deletion naga/tests/out/hlsl/image.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,5 +369,6 @@ float4 depth_no_comparison() : SV_Target0
float2 tc_3 = (0.5).xx;
float s2d_1 = image_2d_depth.Sample(sampler_reg, tc_3);
float4 s2d_gather = image_2d_depth.Gather(sampler_reg, tc_3);
return ((s2d_1).xxxx + s2d_gather);
float s2d_level = image_2d_depth.SampleLevel(sampler_reg, tc_3, 1);
teoxoy marked this conversation as resolved.
Show resolved Hide resolved
return (((s2d_1).xxxx + s2d_gather) + (s2d_level).xxxx);
}
3 changes: 2 additions & 1 deletion naga/tests/out/msl/image.msl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,5 +265,6 @@ fragment depth_no_comparisonOutput depth_no_comparison(
metal::float2 tc_3 = metal::float2(0.5);
float s2d_1 = image_2d_depth.sample(sampler_reg, tc_3);
metal::float4 s2d_gather = image_2d_depth.gather(sampler_reg, tc_3);
return depth_no_comparisonOutput { metal::float4(s2d_1) + s2d_gather };
float s2d_level = image_2d_depth.sample(sampler_reg, tc_3, metal::level(1));
return depth_no_comparisonOutput { (metal::float4(s2d_1) + s2d_gather) + metal::float4(s2d_level) };
}
14 changes: 10 additions & 4 deletions naga/tests/out/spv/image.spvasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
; SPIR-V
; Version: 1.1
; Generator: rspirv
; Bound: 520
; Bound: 526
OpCapability Shader
OpCapability Image1D
OpCapability Sampled1D
Expand Down Expand Up @@ -687,8 +687,14 @@ OpBranch %512
%515 = OpCompositeExtract %7 %514 0
%516 = OpSampledImage %428 %511 %510
%517 = OpImageGather %23 %516 %280 %198
%518 = OpCompositeConstruct %23 %515 %515 %515 %515
%519 = OpFAdd %23 %518 %517
OpStore %508 %519
%518 = OpSampledImage %428 %511 %510
%520 = OpConvertSToF %7 %29
%519 = OpImageSampleExplicitLod %23 %518 %280 Lod %520
%521 = OpCompositeExtract %7 %519 0
%522 = OpCompositeConstruct %23 %515 %515 %515 %515
%523 = OpFAdd %23 %522 %517
%524 = OpCompositeConstruct %23 %521 %521 %521 %521
%525 = OpFAdd %23 %523 %524
OpStore %508 %525
OpReturn
OpFunctionEnd
3 changes: 2 additions & 1 deletion naga/tests/out/wgsl/image.wgsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,5 +235,6 @@ fn depth_no_comparison() -> @location(0) vec4<f32> {
const tc_3 = vec2(0.5f);
let s2d_1 = textureSample(image_2d_depth, sampler_reg, tc_3);
let s2d_gather = textureGather(image_2d_depth, sampler_reg, tc_3);
return (vec4(s2d_1) + s2d_gather);
let s2d_level = textureSampleLevel(image_2d_depth, sampler_reg, tc_3, 1i);
return ((vec4(s2d_1) + s2d_gather) + vec4(s2d_level));
}