Since WebGPU has a similar vertex/fragment shader concept, the UI would likely be the same as for WebGL/WebGL2 (e.g. split view vertex and fragment). Allowing live editing and recompilation would also be nice :)
Created attachment 378639 [details] [Patch] WIP This patch makes WebGPU shader pipelines (both render and compute) visible for the corresponding `WebGPUDevice`. I still need to get editing working (including modifying the `entryPoint`, if possible), but this is already pretty great 😁 For compute shader pipelines, we don't need a split view as there's only one shader, so we only show a single content view with "Compute Shader" in the header. This also needs tests -.-
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 378640 [details] [Image] After Patch is applied
Comment on attachment 378639 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=378639&action=review > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:65 > textEditor.mimeType = "x-shader/x-vertex"; We should probably create another MIME type for compute shaders (e.g. "x-shader/x-compute"), but that isn't supported by CodeMirror right now (not to mention that "x-shader/x-vertex" and "x-shader/x-fragment" don't have proper syntax highlighting for WHLSL).
Created attachment 378718 [details] [Patch] WIP Reworked some things and added further scaffolding for editing. The last bit remaining is to actually hook up editing (`GPUComputePipeline::recompile` and `GPURenderPipeline::recompile`).
Created attachment 378836 [details] [Patch] WIP
Created attachment 378840 [details] [Patch] WIP
Created attachment 378862 [details] Patch
Comment on attachment 378862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378862&action=review r=me Nice! Great tests. The WebGPU Pipeline stuff went over my head so you might want to get a nod from a WebGPU expert. WK1 tests seem to have failed, maybe WebGPU is not enabled there?: CONSOLE MESSAGE: line 83: Unhandled Promise Rejection: TypeError: undefined is not an object (evaluating 'navigator.gpu.requestAdapter') > Source/WebCore/inspector/InspectorShaderProgram.cpp:120 > return nullptr; Seems we should also ASSERT_NOT_REACHED for WebGL here. > Source/WebCore/inspector/InspectorShaderProgram.cpp:231 > + if (entryPoint) > + shader.value().entryPoint = *entryPoint; Is there any concern here about a malicious kind of entryPoint string? > Source/WebCore/platform/graphics/gpu/GPUComputePipeline.h:70 > + // Prserved for Web Inspector recompilation. Typo: "Prserved" => "Preserved", This happens a few times. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:45 > + static _contextTypeSupportsProgramType(contextType, programType) These are static, might as well not give them an underscore prefix. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:118 > + if (this._canvas.contextType === WI.Canvas.ContextType.WebGPU) > + return; > + > + console.assert(this._programType === ShaderProgram.ProgramType.Render); > + console.assert(this._canvas.contextType === WI.Canvas.ContextType.WebGL || this._canvas.contextType === WI.Canvas.ContextType.WebGL2); Maybe move the assertions above the early return? That would help catch if we ever did this for a WebGPU context among other issues. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:152 > + console.assert((!!entryPoint) === (this._canvas.contextType === WI.Canvas.ContextType.WebGPU)); Style: Could drop the parenthesis on the left side. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:63 > + textEditor.mimeType = "x-shader/x-vertex"; // Treat this like a vertex shader for CodeMirror. You could probably add something to CodeMirrorAdditions.js: // FIXME: Add a WHLSL specific mode. // Treat a computer shader like a vertex shader. CodeMirror.defineMIME("x-shader/x-compute", "x-shader/x-vertex"); And keep this code rather clean. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:195 > + case WI.Canvas.ContextType.WebGPU: > + extension = WI.unlocalizedString(".whlsl"); Is this the preferred extension? Perhaps `.wsl`? I was unable to find any concrete examples. whlsl seems find to me! > Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:55 > + this.element.addEventListener("mouseover", this._handleMouseOver.bind(this)); > + this.element.addEventListener("mouseout", this._handleMouseOut.bind(this)); I assume we will want to / should be able to do this for WebGPU in the future. So maybe add a FIXME. > LayoutTests/ChangeLog:11 > + * inspector/canvas/requestShaderSource-expected.txt: It kind stinks when we have: inspector/foo/bar.html inspector/foo/bar-a.html inspector/foo/bar-b.html That sorting gets ugly for: inspector/foo/bar-a-expected.html inspector/foo/bar-b-expected.html inspector/foo/bar-expected.html Maybe we should rename this common tests to something like: inspector/canvas/requestShaderSource-invalid.html inspector/canvas/requestShaderSource-common.html > LayoutTests/ChangeLog:12 > + * inspector/canvas/updateShader.html: Ditto > LayoutTests/inspector/canvas/requestShaderSource-webgl.html:63 > + InspectorTest.log(source); `InspectorTest.ShaderProgram.logSource` instead of just `InspectorTest.log`? > LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgpu.js:113 > + InspectorTest.log("`" + source + "`"); Maybe "```\n" + source + "```"? That is more Markdown like output for generic code. > LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu-expected.txt:15 > +PASS: Added "compute ShaderProgram. Nit: Unbalanced quote > LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu.html:29 > + InspectorTest.expectThat(program instanceof WI.ShaderProgram, `Added "${programType} ShaderProgram.`); Style: Unbalanced quote around ${programType}. > LayoutTests/inspector/canvas/updateShader-webgpu.html:96 > + test({ > + name: "Canvas.updateShader.WebGPU.Fragment.Invalid", > + programType: WI.ShaderProgram.ProgramType.Render, > + shaderType: WI.ShaderProgram.ShaderType.Fragment, > + source: "INVALID", > + entryPoint: "INVALID", > + shouldThrow: true, > + }); What happens if you attempt to update with only an invalid or missing entryPoint?
Comment on attachment 378862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378862&action=review >> Source/WebCore/inspector/InspectorShaderProgram.cpp:120 >> return nullptr; > > Seems we should also ASSERT_NOT_REACHED for WebGL here. I'd rather not, given that this is basically ingesting straight from the frontend, and there's nothing preventing an arbitrary frontend from sending "compute" for a WebGL program. It is technically reachable. >> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:152 >> + console.assert((!!entryPoint) === (this._canvas.contextType === WI.Canvas.ContextType.WebGPU)); > > Style: Could drop the parenthesis on the left side. I would find that harder to read 😅 >> LayoutTests/inspector/canvas/requestShaderSource-webgl.html:63 >> + InspectorTest.log(source); > > `InspectorTest.ShaderProgram.logSource` instead of just `InspectorTest.log`? That's only in the `-webgpu.js`. I was trying to avoid modifying the expected results of the existing tests. >> LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgpu.js:113 >> + InspectorTest.log("`" + source + "`"); > > Maybe "```\n" + source + "```"? That is more Markdown like output for generic code. Lol yup that'd be better :P >> LayoutTests/inspector/canvas/updateShader-webgpu.html:96 >> + }); > > What happens if you attempt to update with only an invalid or missing entryPoint? I'm going to remove the ability to edit the `entryPoint`, as it's not really a "good" process and is likely not really something that the developer is going to change often. I'd rather have a "Shader" details sidebar that has information about the shader program/pipeline, especially since WebGPU shader pipelines have a LOT more configuration than WebGL shader programs.
Comment on attachment 378862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378862&action=review > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.h:-41 > -// Hide the contents of the ShaderModule. Clients of the compiler shouldn't care what's inside it. The details of the ShaderModule need to be hidden from WebCore. We expect to replace this string with some other compiler-dependent stuff. Instead, a better solution would be to create a wrapper object that contains a ShaderModule and a string. In the future, the string in the ShaderModule will be gone, but the other string can persist. (Even if there are two strings, they will both point to the same String Impl so the won't cause memory bloat.) > Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:59 > + // Prserved for Web Inspector recompilation. typo
I'm still in the middle of reviewing this.
Comment on attachment 378862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378862&action=review >> Source/WebCore/inspector/InspectorShaderProgram.cpp:231 >> + shader.value().entryPoint = *entryPoint; > > Is there any concern here about a malicious kind of entryPoint string? You mean, shader code? This string here is just used to match the contents of the shader; any shader code that can run will first be vetted by the compiler.
Comment on attachment 378862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378862&action=review >> LayoutTests/ChangeLog:11 >> + * inspector/canvas/requestShaderSource-expected.txt: > > It kind stinks when we have: > > inspector/foo/bar.html > inspector/foo/bar-a.html > inspector/foo/bar-b.html > > That sorting gets ugly for: > > inspector/foo/bar-a-expected.html > inspector/foo/bar-b-expected.html > inspector/foo/bar-expected.html > > Maybe we should rename this common tests to something like: > > inspector/canvas/requestShaderSource-invalid.html > inspector/canvas/requestShaderSource-common.html I agree that the sorting isn't ideal, but I don't like adding a `-common`/`-invalid` suffix. I'd rather the common things be in the file that matches the protocol name so it's the most straightforward to find, and then any "specializations" have a `-*` suffix at the end.
Created attachment 378927 [details] Patch Reworked shader module source logic to not expose the `WHLSL::ShaderModule`.
Comment on attachment 378927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378927&action=review this looks LGTM to me > Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:50 > + Optional<WebGPUPipeline::ShaderData> computeShader() const { return m_computeShader; } I wonder if this should just be shader()? > Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:52 > + bool recompile(const WebGPUDevice&); Why not just compile(..)? > Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:60 > + Optional<WebGPUPipeline::ShaderData> m_computeShader; m_shader; > Source/WebCore/Modules/webgpu/WebGPUPipeline.cpp:48 > +Lock& WebGPUPipeline::instancesMutex() WebGPUPipeline::mutex() ?
Comment on attachment 378927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378927&action=review >> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:50 >> + Optional<WebGPUPipeline::ShaderData> computeShader() const { return m_computeShader; } > > I wonder if this should just be shader()? I like keeping the `compute*` prefix, so it's clear that the shader is expected to be a compute shader. Also, many other values are also prefixed with `compute*` (e.g. `m_computePipeline`). >> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:52 >> + bool recompile(const WebGPUDevice&); > > Why not just compile(..)? I wanted some distinction/indication that this doesn't need to be called after construction, and is instead a way of basically "updating" the pipeline. Perhaps `update` would be better? >> Source/WebCore/Modules/webgpu/WebGPUPipeline.cpp:48 >> +Lock& WebGPUPipeline::instancesMutex() > > WebGPUPipeline::mutex() ? This matches `WebGLProgram::instancesMutex` and `CanvasRenderingContext::instancesMutex` and `WebGPUDevice::instancesMutex`.
Comment on attachment 378927 [details] Patch I spoke with Myles offline, and he said to go ahead and land this.
The commit-queue encountered the following flaky tests while processing attachment 378927 [details]: media/modern-media-controls/compact-media-controls/compact-media-controls-constructor.html bug 193587 (author: graouts@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 378927 [details] Patch Clearing flags on attachment: 378927 Committed r250114: <https://trac.webkit.org/changeset/250114>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55543450>
It looks like the changes in https://trac.webkit.org/changeset/250114/webkit has broken ~16 tests on Mojave Release wk2. Build: https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/6794 I was able to reproduce these crashes locally by just running the webgpu/ directory on 250113 which passed and 250114 which failed. Results: https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r250114%20(6794)/results.html
This also appears to have caused flaky crashes on WebKit1, see bug 202038 and bug 202046. Seems like we should do an emergency rollback due to impact on EWS and commit queue.
Reverted r250114 for reason: Broke ~16 webgpu/ tests on Mojave wk2 Committed r250139: <https://trac.webkit.org/changeset/250139>
Created attachment 379387 [details] Patch Remove some invalid `ASSERT` (the `layout` of a pipeline descriptor can be left out).
Comment on attachment 379387 [details] Patch Oops, this still has some debug logging :(
Created attachment 379393 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 379393 [details]: inspector/canvas/resolveContext-bitmaprenderer.html bug 202038 (author: drousso@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 379393 [details] Patch Clearing flags on attachment: 379393 Committed r250258: <https://trac.webkit.org/changeset/250258>
This is causing crashes again, see bug 202186.