Bug 194406 - [Web GPU] Update GPUSwapChainDescriptor, GPUSwapChain and implement GPUCanvasContext
Summary: [Web GPU] Update GPUSwapChainDescriptor, GPUSwapChain and implement GPUCanvas...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-07 12:06 PST by Justin Fan
Modified: 2019-03-11 20:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (118.11 KB, patch)
2019-03-09 03:59 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.60 MB, application/zip)
2019-03-09 06:14 PST, Build Bot
no flags Details
Patch (120.59 KB, patch)
2019-03-10 22:58 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (120.59 KB, patch)
2019-03-11 17:07 PDT, 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 2019-02-07 12:06:06 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-02-07 12:06:29 PST
<rdar://problem/47892466>
Comment 2 Justin Fan 2019-02-07 15:34:08 PST
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.
Comment 3 Justin Fan 2019-03-09 03:59:52 PST
Created attachment 364125 [details]
Patch
Comment 4 Justin Fan 2019-03-09 04:04:02 PST
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.
Comment 5 Justin Fan 2019-03-09 04:04:40 PST
Patch is still WIP as ChangeLogs have to be elaborated on, but tossing it at EWS to see what sticks.
Comment 6 Build Bot 2019-03-09 04:05:20 PST
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.
Comment 7 Build Bot 2019-03-09 06:14:28 PST
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
Comment 8 Build Bot 2019-03-09 06:14:29 PST
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
Comment 9 Justin Fan 2019-03-10 22:58:52 PDT
Created attachment 364235 [details]
Patch
Comment 10 Myles C. Maxfield 2019-03-11 16:07:25 PDT
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.
Comment 11 Myles C. Maxfield 2019-03-11 16:30:56 PDT
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?
Comment 12 Justin Fan 2019-03-11 16:55:22 PDT
(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().
Comment 13 Justin Fan 2019-03-11 17:07:50 PDT
Created attachment 364314 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2019-03-11 17:46:47 PDT
Comment on attachment 364314 [details]
Patch for landing

Clearing flags on attachment: 364314

Committed r242759: <https://trac.webkit.org/changeset/242759>
Comment 15 WebKit Commit Bot 2019-03-11 17:46:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Devin Rousso 2019-03-11 19:18:56 PDT
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".
Comment 17 Devin Rousso 2019-03-11 19:21:20 PDT
(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.
Comment 18 Devin Rousso 2019-03-11 19:22:28 PDT
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!
Comment 19 Justin Fan 2019-03-11 20:13:00 PDT
(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).