Bringup red triangle - #5785

Why the removal of WGPUBufferUsage_CopyDst & WGPUBufferUsage_MapRead constraints?

The previous validation didn't allow for MapWrite | Vertex or other valid
combinations. I relaxed restrictions to only prevent buffers mapped as both
read and write which is invalid according to the specification.

I couldn't find where in the spec it says Read is only valid with CopyDst, does that exist? And if so, how would the buffers get bound as vertex / index / uniform / etc buffers?

https://gpuweb.github.io/gpuweb/#buffer-usage

If descriptor.usage contains MAP_READ: descriptor.usage contains no other flags except COPY_DST.

If descriptor.usage contains MAP_WRITE: descriptor.usage contains no other flags except COPY_SRC.

The idea is that authors have to copy data in/out of all mappable buffers. I've tried many times to relax this restriction.

gpuweb/gpuweb#2926 (comment)

gpuweb/gpuweb#2388

Ditto

`Fully' is redundant.

`Fully' is redundant.

Use C++ raw string literal?

return String::fromUTF8(R"(
#include <metal_stdlib>
using namespace metal;
struct Vertex {
    float4 position [[position]];
    float4 color;
    ...
)");

oh that is much nicer, I'll do that, thank you

The linter did not like that:

ERROR: Source/WebGPU/WebGPU/ShaderModule.mm:86:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]

I'm not reviewing these very closely. Someone that knows GPUProcess should review these.

@kkinnunen-apple is this a reasonable approach or should I do something else? These constructors get called on the WebGPU work queue but startReceivingMessages needs to be called from the main queue at the moment, right?

On the surface level this looks incorrect.
If this is the intention, this class of changes look like they could perhaps be submitted separately.
Submitting to main runloop is done with callOnMainRunLoop, but this really doesn't look like the correct approach.

IIRC the WebGPU, like WebGL is using the "main" IPC::Connection as their out of stream connection.
If I understand correctly, you are running into an assertion? I think the solution is to start using the "dedicated connection", similar to what RemoteRenderingBackend uses. This change could be submitted separately.

wgslSource

@litherum not sure if this change is correct / i.e., lifetime is guaranteed to be ok. But the RemoteDeviceProxy instance which is passed to RemoteQueueProxy's constructor comes from RemoteDeviceProxy.cpp:56 and there is no call to adoptRef(...) so using Ref<...> here crashes.

But based on RemoteDeviceProxy's usage of RemoteQueueProxy here it seems like the lifetime is ok

I think we can actually just delete this member. It doesn't appear to be used for anything, and the ownership model of queues is a bit tricky.

I tried to remove m_parent but it seems to be used in a few places via these member functions in the header:

    RemoteDeviceProxy& parent() { return m_parent; }
    RemoteGPUProxy& root() { return m_parent.root(); }

I think we should not do this until it's ready for people to play around with. I don't think the implementation is at a point where it's good for it to show up in the experimental features menu.

WebGPU (no space)

Maybe this is a silly question, but why does the abstraction layer need to be at the WebGPU layer? Why not just have WebGPUSurface (not WebGPUSurfaceCocoa) and have the objects inside it be platform-specific? I believe this is the design we use for compositing IIRC.

The reasoning from Kiet was to allow non-Apple platforms have a platform specific implementation of WebGPUSurface that uses something other than IOSurfaces.

Do you think it is worth keeping that abstraction or should the platform specific objects be placed inside WebGPUSurface allowing WebGPUSurfaceCocoa to be removed?

I think it's better to keep all the #PLATFORM(COCOA) yuckiness contained inside a single file, rather than sprayed around to all the callers who need to use surfaces.

Another option could be something like

#if PLATFORM(COCOA)
using WebGPUSurface = WebGPUSurfaceCocoa;
#else
using WebGPUSurface = WebGPUNullSurface;
#endif

if all the functions will have the same signatures (which seems likely?)

Why copy?

Looks like the intent is to copy into m_currentConfiguration. This should probably be an r-value reference instead for clarity, I will change it, however it will probably end up having the same effect:

so:

void GPUCanvasContext::configure(GPUCanvasConfiguration&&)

It might be worth adding a comment to indicate in which situations this statement would be reached.

(ditto)

In order to better be able to reason about object lifetime, I think we should pull out the data we need from the render pipeline and retain that, instead of retaining the render pipeline itself.

Completely agree. I'll change that

Nit: The "maybe" is already indicated by the return type

I kind of think the getters shouldn't get internal Metal types, but instead should get the WebGPU type, and we can add a helper in a utilities file to convert. Returning Metal types feels kind of like a layering violation.

This is easy for primitiveType, frontFace, and cullMode, but I don't know if its so simple for renderPipelineState which is used in the RenderPassEncoder.

Do you see a feasible way to achieve this for renderPipelineState? I will also see if I can break the coupling somehow between RenderPipeline and RenderPassEncoder

Can we not actually commit this? How harmful would it be to maintain this in a local checkout until we get something better?

Certainly not harmful, just nothing will show on the screen until the compiler outputs a basic shader like this one

On the surface level this looks like some sort of corruption.

I'm investigating why this is occurring. It occurs during the call to RemoteGPUProxy::RequestAdapter:

auto sendResult = sendSync(Messages::RemoteGPU::RequestAdapter(*convertedOptions, identifier));

On the surface level this looks incorrect.
If this is the intention, this class of changes look like they could perhaps be submitted separately.
Submitting to main runloop is done with callOnMainRunLoop, but this really doesn't look like the correct approach.

IIRC the WebGPU, like WebGL is using the "main" IPC::Connection as their out of stream connection.
If I understand correctly, you are running into an assertion? I think the solution is to start using the "dedicated connection", similar to what RemoteRenderingBackend uses. This change could be submitted separately.

@litherum would it be acceptable to keep the flag in WebPreferences but behind an otherwise undefined variable that I can add to my LocalOverrides.xcconfig? This avoid some recompilation since for some reason modifying WebPreferences.yaml triggers a rebuild of a large number of files

Yes, this solves the problem nicely.

@litherum are you ok with this to avoid checking in the metal source file?

Even once we have the compiler outputting a basic shader, it is likely there will be disconnects between the pipeline and compiler where the pipeline may want to test features not available in the compiler. Having an option to use custom metal source allows development on the pipeline to continue in parallel with the compiler.

I'm a little sad that this is necessary, but I do see the rationale why it is significantly helpful for development.

I don't quite understand the ENABLE(WEBGPU) check, because this is inside WebGPU.framework. It doesn't make a lot of sense to say "build WebGPU.framework but disable WebGPU inside it"

Can we at least use something like

#define TEMPORARY_MSL_HACK_PLEASE_DELETE_ONCE_WGSL_COMPILER_IS_HOOKED_UP 1

or something?

Nit: Can we use WebCore::IOSurface instead of a raw IOSurfaceRef?

We could use WebCore::IOSurface but createIOSurfaceBackedTexture creates a WGPUTextureDescriptorIOSurfaceBacking which is defined in WebGPUExt.h and also cast to a WGPUChainedStruct so I believe we could not use unique_ptr inside the WGPUTextureDescriptorIOSurfaceBacking as WebGPUExt.h is a C-header.

Should we use WebCore::IOSurface in this function even though we need to retain the IOSurfaceRef longer than the lifetime of the WebCore::IOSurface object?

Nit: final

it's already a final class: class WebGPUSurfaceCocoa final: public WebGPUSurface {

do we place final on the member function for redundancy / in case the class is changed from no longer being final? Since final on a member function of a final class does not change anything as far as I know, right?

Oh, yeah, that makes sense. https://webkit.org/code-style-guidelines/#override-methods seems a little ambiguous here. Maybe this is worth a webkit-dev email about style?

I don't think this will work with IOKit sandboxing. This code is expected to run in the Web process, right? I don't think the web process is allowed to create IOSurfaces.

This should be likely created by the GPU process, right?

I think this is a UAF. m_configuration doesn't retain the device. (Nor should it! I'm not sure this is the right design, but I need to gather my thoughts before making a more conclusive comment.)

I'm also not super sure this is the right design. I kind of feel:

1. The Surface is conceptually the equivalent of a CAMetalLayer. It shouldn't need to retain anything WebGPU-specific inside it.
2. The Surface owns 3 IOSurfaces, so that part matches CAMetalLayer. So that's good
3. But the 3 IOSurfaces are created lazily, and sent to the GPU Process one-by-one, which I don't think makes a lot of sense. I think it's a better design to package up all 3 IOSurfaces and send them in a single package to the GPUP (perhaps inside GPUCanvasContext.configure(), or maybe perhaps earlier).
4. I'm also not sure that this belongs in platform/. This class is about interfacing between canvas, the compositor, and webgpu.
5. I'm also not sure if the name is right. I'm considering the possibility that we may want to build browser features with WebGPU in the future (like a compositor, or filters). Those wouldn't be tied to canvas, and presumably would instead have their own subclass of WebGPUSurface, right? Both subclasses would be compiled on Cocoa, though.
6. Is it really right that there's no concept of swap chain here? The GPUP doesn't seem to have any notion of this Surface object either, which I think is kind of contrary to the API design of WebGPU.framework. I kind of think we should try to have WebGPU.framework have a single usage pattern here for both CAMetalLayer and for WebKit, and try to make them as conceptually similar as possible. The current patch seems to be sidestepping this design by creating a new API-exposed WGPUTextureDescriptorIOSurfaceBacking object to create a texture directly from an IOSurface, without using the Surface/Swapchain machinery. I kind of think that's backwards.

I'm going to break this part out into a separate PR

I don't understand how this causes the rendered results to be presented to the screen. Is that really what this function is trying to do?

I guess this is setting up WebGPUSurfaceCocoa::display() to perform the present next time it's called. But surely present() needs to do some invalidation logic to make sure it gets called soon?

How do we know the GPU is done rendering the frame at this point? I don't see any back pressure from the GPUP to the Web process.

I'll take a look at this

Correct void WebGPUSurfaceCocoa::display(PlatformCALayer& layer) causes the contents to appear on the screen.

which on macOS is called from -[WebSimpleLayer display] and then GraphicsLayerCA::platformCALayerLayerDisplay

I think the invalidation logic is handled by GPUCanvasContext::m_compositingResultsNeedsUpdating