Bug 192134 - [WebGPU] WebGPURenderPassEncoder::setPipeline, draw, and endPass prototypes
Summary: [WebGPU] WebGPURenderPassEncoder::setPipeline, draw, and endPass prototypes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-28 20:45 PST by Justin Fan
Modified: 2018-11-30 10:38 PST (History)
9 users (show)

See Also:


Attachments
Patch (29.05 KB, patch)
2018-11-28 21:30 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (29.06 KB, patch)
2018-11-29 12:31 PST, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2018-11-28 20:45:32 PST
[WebGPU] WebGPURenderPassEncoder::setPipeline, draw, and endPass prototypes
Comment 1 Radar WebKit Bug Importer 2018-11-28 20:46:36 PST
<rdar://problem/46331555>
Comment 2 Justin Fan 2018-11-28 21:30:17 PST
Created attachment 355975 [details]
Patch
Comment 3 Dean Jackson 2018-11-29 11:36:49 PST
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.
Comment 4 Justin Fan 2018-11-29 12:24:11 PST
(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.
Comment 5 Justin Fan 2018-11-29 12:25:54 PST
Bug above is <rdar://problem/46331451>
Comment 6 Justin Fan 2018-11-29 12:31:19 PST
Created attachment 356035 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2018-11-29 13:09:17 PST
Comment on attachment 356035 [details]
Patch for landing

Clearing flags on attachment: 356035

Committed r238687: <https://trac.webkit.org/changeset/238687>
Comment 8 WebKit Commit Bot 2018-11-29 13:09:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Frédéric Wang (:fredw) 2018-11-29 23:29:50 PST
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.
Comment 10 Justin Fan 2018-11-30 09:11:59 PST
(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.
Comment 11 Frédéric Wang (:fredw) 2018-11-30 10:38:32 PST
(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!