Summary: | [Web GPU] Indexed drawing and GPUCommandEncoder crash prevention | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Fan <justin_fan> | ||||||
Component: | WebGPU | Assignee: | Justin Fan <justin_fan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, commit-queue, dino, esprehn+autocc, ews-watchlist, graouts, kondapallykalyan, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Justin Fan
2019-04-09 19:00:32 PDT
Created attachment 367096 [details]
Patch
Comment on attachment 367096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367096&action=review > Source/WebCore/ChangeLog:7 > + Please give a summary of what you did. > Source/WebCore/Modules/webgpu/WebGPUCommandEncoder.cpp:171 > + if (!m_commandBuffer || m_commandBuffer->isEncodingPass()) { > LOG(WebGPU, "WebGPUCommandEncoder::finish(): Invalid operation!"); I wonder if we should give a better error message for this case. e.g. "Unable to finish encoder because it's still encoding" or something. Also, can you write a test for this case? > Source/WebCore/Modules/webgpu/WebGPURenderPassEncoder.cpp:130 > + for (auto& buffer : buffers) { Why can't this be const? > Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:98 > + if (isEncodingPass() || !src->isTransferSource() || !dst->isTransferDestination()) { Again, it would be nice to have a more clear message here, as well as tests. > Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:254 > + if (offset >= buffer.byteLength() || offset % 4) { > + LOG(WebGPU, "GPURenderPassEncoder::setIndexBuffer(): Invalid offset!"); > + return; > + } Another case where we should have separate messages and tests. > Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:528 > + Nit: blank line > LayoutTests/webgpu/draw-indexed-triangles.html:51 > + // float4 xyzw, float g > + -1, 1, 0, 1, 1, > + -1, -1, 0, 1, 1, > + 1, 1, 0, 1, 1, > + 1, -1, 0, 1, 1 > + ]); You could put a few non-green values in there to enhance the test (or do it in a separate test). e.g. 1, 1, 0, 1, 0 > LayoutTests/webgpu/draw-indexed-triangles.html:59 > +const indexBufferOffset = 2048; > +const indexOffset = -9001; Can you write some comments explaining what you're trying to test here? > LayoutTests/webgpu/draw-indexed-triangles.html:65 > + const indexArray = new Uint32Array([0, 1, 2, 1, 2, 3]); > + indexArray.forEach((value, index) => { > + indexArray[index] = value - indexOffset; > + }); I guess this works, but seems a bit scary - changing values from inside an iterator. It might be better as something like: const indexArray = new Uint32Array([0, 1, 2, 1, 2, 3].map(v => { v - indexOffset; })); Created attachment 367150 [details]
Patch for landing
Comment on attachment 367150 [details] Patch for landing Clearing flags on attachment: 367150 Committed r244147: <https://trac.webkit.org/changeset/244147> All reviewed patches have been landed. Closing bug. |