Add pipeline shader module error enumeration. - #646
Conversation
kvark
left a comment
There was a problem hiding this comment.
We talked about this and figured out the compilation errors should be moved to GPUShaderModule.
At least that's the plan for our implementation. Does Tint and Apple's WGSL parser expect to have the errors at a different time?
|
I'm pretty sure we can have the parser errors in createShaderModule. But what about more complex errors? What if a non-parser error could be detected in createShaderModule but it's not immediately obvious whether it should be there or in createPipeline? Or what if an error is not 100% clear whether it's during parsing or later? For testing I think it's important that it's reliable/portable (unlike in GL). |
|
|
||
| [Serializable] | ||
| interface GPUCompilationMessage { | ||
| GPUCompilationMessageType type; |
There was a problem hiding this comment.
I’m pretty surprised that all these fields are present. Do we expect applications to use these fields at runtime to patch up their shaders dynamically?
There was a problem hiding this comment.
This looks like a structured representation of a standard error message to me. How would you see them being used for patching?
|
@kainino0x In my understanding, most of the errors need to be detected at the
The result - the shader module - is basically a set of ready to use entry points with defined interfaces, i.e. all the input vertex attributes, output colors, used storage and uniform buffers, etc. The only validation errors left to |
| DOMString? mappedFileName; | ||
| unsigned long long mappedLineNum; | ||
| unsigned long long mappedLinePos; | ||
| }; |
There was a problem hiding this comment.
Is there a message string as well? I'm probably missing something, but I can't see anything that says what the actual error or warning might be.
|
From meeting:
|
|
Reading back the minutes from yesterday, I'm surprised we came to the conclusion that compilation errors should be on the pipeline object and not the shader module. Talking with @dneto0 and @dj2 it seems that all the WGSL compilation errors should be caught at Errors left at pipeline creation are mismatches between the WGSL interface and the interface declared in the descriptor. Or backend compilation errors, but I don't know that we should expose them to the user because it seems more like an issue in the browser's translator. Interface mismatch errors are not actionable in the same way as WGSL compilation errors and could be surfaced through push/popErrorScope. So instead, what do you think of having the following? partial interface GPUShaderModule {
[SameObject] Promise<sequence<GPUCompilationMessage>> compilationMessages();
}; |
|
From that discussion this morning I've decided that I support having errors on ShaderModule. In particular, we would provide structured errors for ShaderModule creation and non-structured (for now) errors for pipeline creation. Reducing that functionality duplication makes it more appealing to me. Pipeline creation errors would be limited as much as possible to compatibility issues (layout compatibility, interface between stages, etc.) David also shared some feedback from the Vulkan ecosystem saying that this choice in Vulkan has been annoying for users, which is a useful signal. |
* Add `GPUCompilationMessage.message` (oops).
| it concurrently. Since {{GPUShaderModule}} is immutable, there are no race | ||
| conditions. | ||
|
|
||
| If no source-map was provided for the |GPUShaderModule|: |
There was a problem hiding this comment.
this should follow-up after #645 , if it's not about to land
|
Is anything missing (apart from the removal of the mention of source maps) before merging this? |
|
|
||
| [Serializable] | ||
| interface GPUShaderModule { | ||
| [SameObject] Promise<GPUCompilationInfo> compilationInfo(); |
There was a problem hiding this comment.
The [SameObject] extended attribute must not be used on anything other than a read only attribute whose type is an interface type or object.
|
Fixed up some uninteresting issues, and merging. |
* Add pipeline shader module error enumeration. * Move compilationMessages to GPUShaderModule. * Add `GPUCompilationMessage.message` (oops). * Remove sourcemaps references for now, and return info iface that has seq<message>. * Apply suggestions from code review * IDL syntax fixes Co-authored-by: Kai Ninomiya <kainino@chromium.org>
* Add pipeline shader module error enumeration. * Move compilationMessages to GPUShaderModule. * Add `GPUCompilationMessage.message` (oops). * Remove sourcemaps references for now, and return info iface that has seq<message>. * Apply suggestions from code review * IDL syntax fixes Co-authored-by: Kai Ninomiya <kainino@chromium.org>
* And add a finishAndSubmit() helper. With the fixes, this should ~eliminate cases where we had multiple (endPass, finish, submit) calls inside one validation error scope and couldn't make sure the errors came from the right functions. This also substantially simplifies the finish/submit in most tests. Folds two other helpers into this one so that all of the CommandBufferMakers are centralized. Tested locally, and all of the changed tests pass (except for existing known issues). * address comments * lint fix
Related to #645 shader-maps.
Preview | Diff