[webkit-reviews] review granted: [Bug 196758] [Web GPU] Indexed drawing and GPUCommandEncoder crash prevention : [Attachment 367096] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 10 11:49:33 PDT 2019
Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 196758: [Web GPU] Indexed drawing and GPUCommandEncoder crash prevention
https://bugs.webkit.org/show_bug.cgi?id=196758
Attachment 367096: Patch
https://bugs.webkit.org/attachment.cgi?id=367096&action=review
--- Comment #2 from Dean Jackson <dino at apple.com> ---
Comment on attachment 367096
--> https://bugs.webkit.org/attachment.cgi?id=367096
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; }));
More information about the webkit-reviews
mailing list