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

Fragment output <-> attachment interface matching rules def #2013

Closed
shrekshao opened this issue Aug 3, 2021 · 21 comments · Fixed by #3408
Closed

Fragment output <-> attachment interface matching rules def #2013

shrekshao opened this issue Aug 3, 2021 · 21 comments · Fixed by #3408
Assignees
Labels
copyediting Pure editorial stuff (copyediting, bikeshed, etc.)
Projects
Milestone

Comments

@shrekshao
Copy link
Contributor

shrekshao commented Aug 3, 2021

Right now the WebGPU spec and WGSL spec are vague about the fragment output and attachment interface matching rules.

https://www.w3.org/TR/webgpu/#fragment-state

The pipeline output type must be compatible with colorState.format.

The rule should have two parts.

  • For BaseType: The scalar type (f32, i32, or u32) must match the sample type of the format. (Pretty clear).
  • For componentCounts: here are results shown by some tests (when the component count of output and channel count of fragment color attachment don't match)
Metal D3D12 Vulkan(Win)
vec4<f32> <-> R8Unorm ✔️ ✔️ ✔️
f32 <-> RGBA8Unorm ❌Error ❌ Undefined value for unwritten channels (Error with backend validation) ❌ Undefined value ✔️(seems happy, pad with 0)

Here are some related spec text I found (please help add refs)

We have 2 options here:

  • State in the spec that component count of the fragment color output should be equal or bigger than the channel count of the colorState.format. The extra components are discarded. Otherwise there would be a validation error.
  • Do a transform of the user-input WGSL shader in our implementations to make sure the output is always expanded to vec4<T>, without validating the componentCount.

WDYT

@kainino0x kainino0x added this to Needs Discussion in Main Aug 3, 2021
@kainino0x kainino0x added this to the MVP milestone Aug 3, 2021
@kainino0x
Copy link
Contributor

kainino0x commented Aug 3, 2021

FWIW, reading the Vulkan spec I just realized why it's always allowed to output too many components: fragment outputs don't go directly into the attachment, they go to the blending unit. The blending unit may use the output alpha value even if the attachment doesn't have an alpha channel.

We need to determine what the blending unit sees in the alpha channel if the shader doesn't output it. If it's an undefined value (edit: see next comment), I think we have to choose one of these options:

  1. Pad all outputs to vec4 (on such backends)
  2. Validate outputs are vec4
  3. Validate outputs are vec4 if they use blending
  4. Validate outputs are vec4 if they use blending modes that read the alpha channel

If it's defined, then option 1 (the first option Shrek posted) is additionally still valid.


Aside: I found this in the Vulkan spec section you linked:

Any value that cannot be represented in the attachment’s format is undefined.

I guess this means there are technically undefined values if you write outside of the range of a unorm/snorm attachment type. That seems like a pretty minor hazard but I'm surprised by it.

@kainino0x
Copy link
Contributor

kainino0x commented Aug 3, 2021

Oh, finally found the relevant sentence in Vulkan (emphasis added):

The input values to blending or color attachment writes are undefined for components which do not correspond to a fragment shader output.

@kvark
Copy link
Contributor

kvark commented Aug 3, 2021

Injecting shader code is always extra work, and also incompatible with the idea of generating MTLLibrary ahead of time. So we should play safe and say that for now the shader output has to:

  1. have at least as many components as the target format
  2. be a vec4 if any of the blend factors for this attachment include one of the following:
    - "src-alpha"
    - "one-minus-src-alpha"
    - "src-alpha-saturated"

@kainino0x
Copy link
Contributor

kainino0x commented Aug 3, 2021

note: I don't think it's incompatible with generating ahead of time. We don't have to pad to the size of the format, we can always pad to vec4 unconditionally.

@kainino0x
Copy link
Contributor

Resolution: Add the precise validation rules (option 5, the one kvark described just above)

@kainino0x kainino0x moved this from Needs Discussion to Needs Specification in Main Aug 9, 2021
@kainino0x
Copy link
Contributor

kainino0x commented Aug 9, 2021

Adding to editor agenda to clarify a corner case: what if the format has alpha, but blending is used and the fragment output alpha actually isn't used?

@kainino0x
Copy link
Contributor

Editors: That should also be valid. Unless the validation layers complain about that.

Buuut then what if the write mask prevents any actual undefined values? Then we should also allow that. Unless the validation layers complain about that. @shrekshao do you have things set up so you could check what the Metal and d3d12 debug layers think about these things?

@shrekshao
Copy link
Contributor Author

shrekshao commented Aug 9, 2021

With fragment output set to f32, attachment format RGBA8Unorm, writeMask GPUColorWrite::Red, D3D12 still treats it as invalid.

So seems that we don't have to consider write mask state at validation

@kainino0x
Copy link
Contributor

Good to know! Could you also check if D3D12 considers the blend factors in its debug layer?

@shrekshao
Copy link
Contributor Author

shrekshao commented Aug 9, 2021

For blending, at least D3D12 doesn't care if blending is null, or enabled but srcFactor/dstFactor doesn't include src-alpha, when writing vec3<f32> to RGBA8Unorm. It simply said it's unmatched. This actually makes implementing the validation a bit easier.

ID3D12Device::CreateGraphicsPipelineState: Pixel Shader output 'SV_Target0' is writing to 3 components, while the corresponding RenderTarget slot [0] has format (R8G8B8A8_UNORM) with 4 component(s). This results in undefined contents in the unwritten channels of the render target. [ STATE_CREATION WARNING #974: CREATEGRAPHICSPIPELINESTATE_PS_OUTPUT_RT_OUTPUT_MISMATCH]

@kainino0x
Copy link
Contributor

kainino0x commented Aug 10, 2021

Hmm, that seems like there might be a deficiency in the D3D12 debug layer. Am I correct in understanding it allows the following case? Which should be invalid.

  • output vec3
  • attachment format is rg
  • blending is enabled, factors include src-alpha

In any case, we will make that invalid, because it's undefined according to the Vulkan spec. So I think the rules might be:

  1. have at least as many components as the target format
  2. be a vec4 if any of the [color] blend factors for this attachment include one of the following:
    • "src-alpha"
    • "one-minus-src-alpha"
    • "src-alpha-saturated"
  1. be a vec4 if:
    • the target format has alpha, and
    • [edited] the .alpha.srcFactor is not 'zero' OR the .alpha.dstFactor includes src or src-alpha

Plus whatever is needed for #2025.

@shrekshao
Copy link
Contributor Author

(Sorry I misunderstood here. I tested against attachment format rgba first)

For

  • output vec3
  • attachment format is rg
  • blending is enabled, factors include src-alpha

D3D12 debug layer (--enable-backend-validation in dawn) let it passed. We should validate this since the alpha channel would be undefined value.

The conclusion seems good.

@kainino0x
Copy link
Contributor

(Sorry I misunderstood here. I tested against attachment format rgba first)

No misunderstanding, I just thought of this when I read your latest result at the time. :)

@kainino0x
Copy link
Contributor

  • any of the alpha blend factors for this attachment include src or src-alpha.

Realized this is wrong, I think it should say

  • the .alpha.srcFactor is not 'zero' OR the .alpha.dstFactor includes src or src-alpha

@kainino0x
Copy link
Contributor

kainino0x commented Aug 12, 2021

  • the .alpha.srcFactor is not 'zero'

reading/thinking about this YET FURTHER, this may also be wrong. If src.alpha is undefined, then multiplying it by zero is still undefined, e.g. if src.alpha was NaN.


Unfortunately I can't find a concrete answer to either issue (#2013 or #2025) in the D3D11 functional spec - all language seems to ignore the possibility that the blend factor uses an alpha value that wasn't output by the shader. However I did find some interesting sections that very explicitly call out the use of write masks to protect against undefined results:

https://microsoft.github.io/DirectX-Specs/d3d/archive/D3D11_3_FunctionalSpec.htm#17.15%20Output%20Write%20Masks

Failure to provide sufficient data to the Output Merger for all of the RenderTarget(s)/component(s) enabled with the write masks results in undefined values being written out.

https://microsoft.github.io/DirectX-Specs/d3d/archive/D3D11_3_FunctionalSpec.htm#OutputWrites

Partial writes to a given o# output register (writing a nonempty proper subset of the declared components) will produce undefined results in the unwritten component(s) that were declared for output. i.e. Declaring o0.rga but only writing o0.r means the RenderTarget location for o0.ga will be written with undefined values. However the application can take advantage of the write-enable masks to prevent undefined values from being written out and thus vary outputs with flow control in a Shader, as long as the condition doesn't vary within a given Draw*() (since the write-enable masks can only be updated between Draw*() calls).

@kainino0x
Copy link
Contributor

kainino0x commented Aug 12, 2021

Ahh the addition I made to the rules earlier (rule 3) is actually redundant: For any format that has an alpha channel, the shader must output vec4 anyway due to rule 1.

(edited) So the first 2 rules in #2013 (comment) were correct, but revising rule 1 here for generality/preciseness:

Shader output must:

  1. have a superset of the components of the target format
  2. be a vec4 if any of the color blend factors for this attachment include one of the following:
    • "src-alpha"
    • "one-minus-src-alpha"
    • "src-alpha-saturated"

@shrekshao
Copy link
Contributor Author

Does the revised rule without rule 3 covers this case:
fragment output: vec3<f32>
attachemnt format: rg8unorm
blending:
alpha.dstFactor: src-alpha

The attachment doesn't actually have an alpha channel, so the src-alpha although reading from something undefined but the value is discarded anyway.

So seems the revised rule is correct...

@kainino0x
Copy link
Contributor

My mental model from reading the D3D spec is that blending for all 4 channels always runs, what matters is whether undefined values make it into the output. So in this case, the alpha channel blending reads dst=undef, multiplies it with dstFactor=undef, resulting in a final value of undef, but that doesn't get written so it doesn't matter.

@kainino0x
Copy link
Contributor

Ohh yes sorry, the revised rules I posted aren't actually the original 2 rules. They are still changed to say "color blend factors" instead of "blend factors"

@Kangz
Copy link
Contributor

Kangz commented Dec 14, 2021

What still needs to happen to be able to close this issue?

@kainino0x
Copy link
Contributor

I think we figured out what the spec needs to say, so just spec work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
copyediting Pure editorial stuff (copyediting, bikeshed, etc.)
Projects
No open projects
Main
Needs Specification
Development

Successfully merging a pull request may close this issue.

4 participants