Bug 194406 – [Web GPU] Update GPUSwapChainDescriptor, GPUSwapChain and implement GPUCanvasContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Justin Fan]

GPUSwapChainDescriptor has been simplified in the sketch IDL (https://github.com/gpuweb/gpuweb/blob/master/design/sketch.webidl#L630). For example, height and width of the swap chain are now gleaned from the canvas. In addition, GPUTextureUsageFlags has been implemented and should be taken into account when creating the swap chain.

[Radar WebKit Bug Importer]

<rdar://problem/47892466>

[Justin Fan]

This also involves providing the GPUDevice to Web GPU during context/swapChain creation (canvas.getContext()) rather than by configuring it on the swap chain object.

[Justin Fan]

Created attachment 364125 [details]
Patch

[Justin Fan]

Comment on attachment 364125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364125&action=review

> Source/WebCore/Modules/webgpu/DOMWindowWebGPU.idl:31
> +    [Replaceable, SameObject] readonly attribute WebGPU gpu;

This isn't permanent, as the API now exposes "gpu" as just a namespace on window rather than as an object. I made this change while trying to fix Inspector.

[Justin Fan]

Patch is still WIP as ChangeLogs have to be elaborated on, but tossing it at EWS to see what sticks.

[EWS Watchlist]

Attachment 364125 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 364125 [details]
Patch

Attachment 364125 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11438496

New failing tests:
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 364129 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Justin Fan]

Created attachment 364235 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 364235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364235&action=review

> Source/WebCore/Modules/webgpu/WebGPUDevice.idl:36
> +    GPUTextureUsageFlags usage = 16; // FIXME: How to set this to GPUTextureUsage.OUTPUT_ATTACHMENT in IDL?

Might want to add a comment in GPUTextureUsage so we know if we modify those flags we have to modify this value too.

Also would be good to file a new bug about this.

[Myles C. Maxfield]

Comment on attachment 364235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364235&action=review

> Source/WebCore/Modules/webgpu/GPUCanvasContext.cpp:71
> +    if (m_swapChain && m_swapChain->swapChain())
> +        m_swapChain->swapChain()->present();

I don't quite understand this. Maybe Dean can help here.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:170
> +    useAttachments(buffer, WTFMove(descriptor));

Cool.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUSwapChainMetal.mm:66
> +static RetainPtr<WebGPULayer> tryCreateSwapLayer(MTLDevice *device, MTLPixelFormat format, OptionSet<GPUTextureUsage::Flags> usage)

"Create Swap"? "Swap Layer"? This could be named better.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUSwapChainMetal.mm:109
> +    setLayerShape(swapLayer.get(), width, height);

Shouldn't we be hooking this up to get called when the canvas backing store changes size?

[Justin Fan]

(In reply to Myles C. Maxfield from comment #11)
> Comment on attachment 364235 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364235&action=review
> 
> > Source/WebCore/Modules/webgpu/GPUCanvasContext.cpp:71
> > +    if (m_swapChain && m_swapChain->swapChain())
> > +        m_swapChain->swapChain()->present();
> 
> I don't quite understand this. Maybe Dean can help here.

Yeah, I'm not entirely sure if this implementation is necessary either.

> > Source/WebCore/platform/graphics/gpu/cocoa/GPUSwapChainMetal.mm:109
> > +    setLayerShape(swapLayer.get(), width, height);
> 
> Shouldn't we be hooking this up to get called when the canvas backing store
> changes size?

This should happen because GPUCanvasContext::reshape() also calls setLayerShape().

[Justin Fan]

Created attachment 364314 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364314 [details]
Patch for landing

Clearing flags on attachment: 364314

Committed r242759: <https://trac.webkit.org/changeset/242759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Devin Rousso]

Comment on attachment 364314 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=364314&action=review

> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:53
> +    'webgpu': 'WebGPU',  # Canvas.ContextType.gpu

NIT: `Canvas.ContextType.webgpu` to match Canvas.json.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:253
> +        return toJS(&state, deprecatedGlobalObjectForPrototype(&state), downcast<GPUCanvasContext>(context));

You'd also want to update `WebCore::canvasRenderingContext` in PageConsoleClient.cpp with the same type of thing.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:415
> +    WebGPU: "gpu",

We keep these values in sync with the ones in Canvas.json, so it should be "webgpu".

[Devin Rousso]

(In reply to Devin Rousso from comment #16)
> Comment on attachment 364314 [details]
> Patch for landing
Dunno why this didn't save with my other comments, but here it is.

Thanks for adding some inspector support for this!  In the future, please reach out (or just CC) one of us on the bug so we can provide feedback (if needed).  I noticed a few things while reading the inspector parts.  I've commented on them inline.

[Devin Rousso]

Comment on attachment 364314 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=364314&action=review

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:253
>> +        return toJS(&state, deprecatedGlobalObjectForPrototype(&state), downcast<GPUCanvasContext>(context));
> 
> You'd also want to update `WebCore::canvasRenderingContext` in PageConsoleClient.cpp with the same type of thing.

Actually, I take this back.  We don't support recording Web GPU contexts right now, so you wouldn't want to add it there.  My mistake!

[Justin Fan]

(In reply to Devin Rousso from comment #17)
> (In reply to Devin Rousso from comment #16)
> > Comment on attachment 364314 [details]
> > Patch for landing
> Dunno why this didn't save with my other comments, but here it is.
> 
> Thanks for adding some inspector support for this!  In the future, please
> reach out (or just CC) one of us on the bug so we can provide feedback (if
> needed).  I noticed a few things while reading the inspector parts.  I've
> commented on them inline.

Thanks for looking it over! I apologize for not roping you guys in as I really only made the changes to Inspector because my renaming had caused it to keep crashing. In the future we can work together on improved Inspector support and I can clean up the naming (unfortunately it still isn't final).