Bug 192134

<rdar://problem/46331555>

Created attachment 355975 [details] Patch

Comment on attachment 355975 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355975&action=review > Source/WebCore/Modules/webgpu/WebGPUProgrammablePassEncoder.cpp:46 > +Ref<WebGPUCommandBuffer> WebGPUProgrammablePassEncoder::endPass() > +{ > + passEncoder().endPass(); > + return m_commandBuffer.copyRef(); > +} I know the IDL specifies it, but I wonder why endPass returns a command buffer. Surely the author would have created it and held a reference? Maybe this is an issue for the spec? > Source/WebCore/Modules/webgpu/WebGPUProgrammablePassEncoder.idl:34 > + // FIXME: Only support render pipelines for demo. s/demo/prototype/ > Source/WebCore/Modules/webgpu/WebGPURenderPassEncoder.cpp:52 > +void WebGPURenderPassEncoder::draw( > + unsigned long vertexCount, > + unsigned long instanceCount, > + unsigned long firstVertex, > + unsigned long firstInstance > + ) Put all this on one line. > Source/WebCore/Modules/webgpu/WebGPURenderPassEncoder.cpp:54 > +{ > + m_passEncoder->draw(vertexCount, instanceCount, firstVertex, firstInstance); Could you put a // FIXME in here saying we need to do some validation. e.g. all values are > 0, etc. > Source/WebCore/platform/graphics/gpu/GPURenderPassEncoder.h:58 > + ~GPURenderPassEncoder() { endPass(); } // The MTLCommandEncoder must end encoding before it is released. Remember that this file will ultimately be used by backends other than Metal. So maybe the comment should be "Ensure that the encoding has ended before destruction." > Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:104 > +void GPURenderPassEncoder::draw( > + unsigned long vertexCount, > + unsigned long instanceCount, > + unsigned long firstVertex, > + unsigned long firstInstance > + ) One line. > Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:111 > + [m_platformRenderPassEncoder > + drawPrimitives:primitiveTypeForGPUPrimitiveTopology(m_pipeline->primitiveTopology()) > + vertexStart:firstVertex > + vertexCount:vertexCount > + instanceCount:instanceCount > + baseInstance:firstInstance]; Either everything on a single line, or the 2nd+ lines should be indented. > LayoutTests/webgpu/render-command-encoding.html:12 > +let canvas, commandBuffer, renderPassEncoder; canvas is only used inside the promise test. I also wonder if the other variables should be passed into the functions as parameters.

(In reply to Dean Jackson from comment #3) > Comment on attachment 355975 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=355975&action=review > > I also wonder if the other variables should be passed into the functions as > parameters. I've made a new bug to go back and refactor all of the tests and basic.js to use WPT without relying on js-test-pre, so I'll take care of this in that patch.

Bug above is <rdar://problem/46331451>

Created attachment 356035 [details] Patch for landing

Comment on attachment 356035 [details] Patch for landing Clearing flags on attachment: 356035 Committed r238687: <https://trac.webkit.org/changeset/238687>

All reviewed patches have been landed. Closing bug.

Comment on attachment 356035 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=356035&action=review > Source/WebCore/Modules/webgpu/WebGPUTexture.h:35 > +class GPUTexture; This change caused some build error for me when UnifiedSource rotates (see https://bugs.webkit.org/attachment.cgi?id=356043&action=review). Unfortunately, the error happens in generated JS*.cpp files so I'm not sure how easy it is to properly include "GPUTexture.h". If there is not better suggestion, I would just revert the change to WebGPUTexture.h. In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource106.cpp:1: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderPassEncoder.cpp:25: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderPassEncoder.h:25: In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/bindings/js/JSDOMWrapper.h:24: In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/bindings/js/JSDOMGlobalObject.h:29: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/WebCoreJSBuiltinInternals.h:39: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMBindingInternalsBuiltins.h:34: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Identifier.h:23: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/PrivateName.h:28: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/text/SymbolImpl.h:28: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/text/UniquedStringImpl.h:28: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/text/StringImpl.h:32: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/Vector.h:37: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/VectorTraits.h:23: /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/Ref.h:60:37: error: member access into incomplete type 'WebCore::GPUTexture' PtrTraits::unwrap(m_ptr)->deref(); ^ In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource106.cpp:4: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderingContext.cpp:25: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderingContext.h:26: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPUSwapChain.h:27: In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/webgpu/WebGPUSwapChain.h:33: /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/webgpu/WebGPUTexture.h:38:7: note: in instantiation of member function 'WTF::Ref<WebCore::GPUTexture, WTF::DumbPtrTraits<WebCore::GPUTexture> >::~Ref' requested here class WebGPUTexture : public RefCounted<WebGPUTexture> { ^ In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource106.cpp:1: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderPassEncoder.cpp:25: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderPassEncoder.h:25: In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/bindings/js/JSDOMWrapper.h:24: In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/bindings/js/JSDOMGlobalObject.h:29: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/WebCoreJSBuiltinInternals.h:39: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMBindingInternalsBuiltins.h:34: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Identifier.h:23: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/PrivateName.h:28: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/text/SymbolImpl.h:28: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/text/UniquedStringImpl.h:28: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/text/StringImpl.h:32: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/Vector.h:37: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/VectorTraits.h:23: /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/Ref.h:60:39: note: in instantiation of member function 'WTF::RefCounted<WebCore::WebGPUTexture>::deref' requested here PtrTraits::unwrap(m_ptr)->deref(); ^ In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource106.cpp:8: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPUSwapChain.cpp:37: /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPUTexture.h:81:183: note: in instantiation of member function 'WTF::Ref<WebCore::WebGPUTexture, WTF::DumbPtrTraits<WebCore::WebGPUTexture> >::~Ref' requested here inline JSC::JSValue toJSNewlyCreated(JSC::ExecState* state, JSDOMGlobalObject* globalObject, RefPtr<WebGPUTexture>&& impl) { return impl ? toJSNewlyCreated(state, globalObject, impl.releaseNonNull()) : JSC::jsNull(); } ^ In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource106.cpp:4: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderingContext.cpp:25: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPURenderingContext.h:26: In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSWebGPUSwapChain.h:27: In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/webgpu/WebGPUSwapChain.h:31: /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/gpu/GPUSwapChain.h:38:7: note: forward declaration of 'WebCore::GPUTexture' class GPUTexture; ^ 1 error generated.

(In reply to Frédéric Wang (:fredw) from comment #9) > Comment on attachment 356035 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356035&action=review > > > Source/WebCore/Modules/webgpu/WebGPUTexture.h:35 > > +class GPUTexture; > > This change caused some build error for me when UnifiedSource rotates (see > https://bugs.webkit.org/attachment.cgi?id=356043&action=review). > Unfortunately, the error happens in generated JS*.cpp files so I'm not sure > how easy it is to properly include "GPUTexture.h". If there is not better > suggestion, I would just revert the change to WebGPUTexture.h. Hi Frédéric, I also ran into this so I have reverted the change in https://bugs.webkit.org/show_bug.cgi?id=192213.

(In reply to Justin Fan from comment #10) > (In reply to Frédéric Wang (:fredw) from comment #9) > > Comment on attachment 356035 [details] > > Patch for landing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=356035&action=review > > > > > Source/WebCore/Modules/webgpu/WebGPUTexture.h:35 > > > +class GPUTexture; > > > > This change caused some build error for me when UnifiedSource rotates (see > > https://bugs.webkit.org/attachment.cgi?id=356043&action=review). > > Unfortunately, the error happens in generated JS*.cpp files so I'm not sure > > how easy it is to properly include "GPUTexture.h". If there is not better > > suggestion, I would just revert the change to WebGPUTexture.h. > > > Hi Frédéric, I also ran into this so I have reverted the change in > https://bugs.webkit.org/show_bug.cgi?id=192213. OK, thanks!