Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[webgpu] Add Surface API #21939

Merged
merged 14 commits into from
May 23, 2024
Merged

[webgpu] Add Surface API #21939

merged 14 commits into from
May 23, 2024

Conversation

beaufortfrancois
Copy link
Contributor

FIX #21745

@beaufortfrancois beaufortfrancois marked this pull request as draft May 14, 2024 11:33
@beaufortfrancois beaufortfrancois mentioned this pull request May 14, 2024
Closed
@beaufortfrancois beaufortfrancois force-pushed the surface branch 2 times, most recently from 533d5f6 to d6a8639 Compare May 14, 2024 14:41
@sbc100 sbc100 requested a review from kainino0x May 14, 2024 15:06
@sbc100 sbc100 changed the title Add Surface API [webgpu] Add Surface API May 14, 2024
@beaufortfrancois beaufortfrancois force-pushed the surface branch 2 times, most recently from 139b253 to 5a31c92 Compare May 15, 2024 06:29
kainino0x
kainino0x reviewed May 16, 2024
View reviewed changes
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry haven't finished reviewing but here are some comments

src/library_webgpu.js Outdated Show resolved Hide resolved
src/library_webgpu.js Outdated Show resolved Hide resolved
src/library_webgpu.js Outdated
'premultiplied',
'unpremultiplied',
'inherit',
],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed (see emscripten_no_enum_table in dawn.json)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, this is needed. I saw that it wasn't used and didn't think about the fact that it should be used.

However not all of the entries should be there, it should be something like:

    CompositeAlphaMode: [
      'opaque',
      'opaque',
      'premultiplied',
    ],

src/library_webgpu.js Outdated Show resolved Hide resolved
kainino0x
kainino0x reviewed May 16, 2024
View reviewed changes
src/library_webgpu.js Outdated Show resolved Hide resolved
src/library_webgpu.js Outdated Show resolved Hide resolved
@beaufortfrancois
Copy link
Contributor Author

Sorry haven't finished reviewing but here are some comments

Thanks for starting the review @kainino0x.
I've addressed all your feedback.

There are still some TODOs I'd like to solve such as presentModes, alphaModes, and the error returned by getCurrentTexture.

@beaufortfrancois beaufortfrancois marked this pull request as ready for review May 16, 2024 09:26
kainino0x
kainino0x reviewed May 17, 2024
View reviewed changes
src/library_webgpu.js Show resolved Hide resolved
system/lib/webgpu/webgpu.cpp Outdated Show resolved Hide resolved
test/webgpu_basic_rendering.cpp Outdated Show resolved Hide resolved
test/webgpu_basic_rendering.cpp Outdated Show resolved Hide resolved
system/lib/webgpu/webgpu.cpp Outdated Show resolved Hide resolved
system/lib/webgpu/webgpu.cpp Outdated Show resolved Hide resolved
system/lib/webgpu/webgpu.cpp Show resolved Hide resolved
system/lib/webgpu/webgpu.cpp Outdated Show resolved Hide resolved
kainino0x
kainino0x reviewed May 17, 2024
View reviewed changes
src/library_webgpu.js Outdated Show resolved Hide resolved
src/library_webgpu.js Outdated Show resolved Hide resolved
kainino0x
kainino0x reviewed May 17, 2024
View reviewed changes
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed all my own comments, so LGTM now, but @beaufortfrancois PTAL before I land it.

@beaufortfrancois
Copy link
Contributor Author

I addressed all my own comments, so LGTM now, but @beaufortfrancois PTAL before I land it.

@kainino0x I've just merged main branch and I've tried locally all your latest changes. LGTM. Thank you!

@beaufortfrancois beaufortfrancois mentioned this pull request May 21, 2024
Merged
@juj
Copy link
Collaborator

juj commented May 21, 2024

Is there an IDL or a spec available to this Surface API? It doesn't seem to exist in the WebGPU spec at least.

@beaufortfrancois
Copy link
Contributor Author

beaufortfrancois commented May 21, 2024 •
edited
Loading

Is there an IDL or a spec available to this Surface API? It doesn't seem to exist in the WebGPU spec at least.

I believe webgpu-native/webgpu-headers#164 and webgpu-native/webgpu-headers#203 are where things got defined.

@kainino0x
Copy link
Collaborator

Is there an IDL or a spec available to this Surface API? It doesn't seem to exist in the WebGPU spec at least.

The Web API only concerns itself with HTML Canvas, whereas Surface is a C-API-only abstraction over both HTML Canvas and native window system integration. The Surface API is very close to GPUCanvasContext though.

@juj
Copy link
Collaborator

juj commented May 21, 2024

What got me confused is that the JS side code configures a GPUCanvasContext using a presentMode parameter here: https://github.com/emscripten-core/emscripten/pull/21939/files#diff-f2ba55d634491c159f57989d779458c308f77253f42039012674780ff95894daR2716

but there is no such parameter defined in the IDL: https://www.w3.org/TR/webgpu/#dictdef-gpucanvasconfiguration (and no presentMode mentioned in the spec page). Is that something that is not yet defined in the WebGPU spec, or maybe a typo leaking over from the Dawn side?

@kainino0x
Copy link
Collaborator

Ah, that's not supposed to be there. presentMode is native-only, Chrome doesn't support it in Blink, it just gets dropped on the floor because that's how WebIDL works.

kainino0x
kainino0x reviewed May 22, 2024
View reviewed changes
src/library_webgpu.js Outdated Show resolved Hide resolved
src/library_webgpu.js Outdated
'premultiplied',
'unpremultiplied',
'inherit',
],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, this is needed. I saw that it wasn't used and didn't think about the fact that it should be used.

However not all of the entries should be there, it should be something like:

    CompositeAlphaMode: [
      'opaque',
      'opaque',
      'premultiplied',
    ],

src/library_webgpu.js
"usage": {{{ gpu.makeGetU32('config', C_STRUCTS.WGPUSurfaceConfiguration.usage) }}},
// viewFormatCount
// viewFormats
"alphaMode": "opaque",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be either "opaque" or "premultiplied" depending on the configuration, and there should be an assert that it's one of the values that's valid in JS (auto, opaque, premultiplied). (Just look it up in the table above and then assert that the result is not undefined.)

Alternatively, assert the alphaMode is Opaque for now with a TODO, and implement this later if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've assert alphaMode is Opaque with a TODO.

src/library_webgpu.js Outdated Show resolved Hide resolved
kainino0x
kainino0x approved these changes May 23, 2024
View reviewed changes
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one last thing

test/webgpu_basic_rendering.cpp Outdated Show resolved Hide resolved
src/library_webgpu.js Outdated Show resolved Hide resolved
src/library_webgpu.js Show resolved Hide resolved
@kainino0x kainino0x enabled auto-merge (squash) May 23, 2024 01:33
@kainino0x kainino0x merged commit 579da6c into emscripten-core:main May 23, 2024
29 checks passed
@beaufortfrancois beaufortfrancois deleted the surface branch May 23, 2024 07:41
@beaufortfrancois
Copy link
Contributor Author

@sbc100 Is there any chance we can publish next Emscripten release (3.1.61 I believe) when Chrome 126 hits stable channel (June 10th) so that developers don't have to use the ToT version of Emscripten to use the Surface API in WebGPU?

@sbc100
Copy link
Collaborator

sbc100 commented May 23, 2024

Sure, we can basically cut releases as we please.

@sbc100
Copy link
Collaborator

sbc100 commented May 23, 2024

(assuming the tree is green and there are no known issues)

@beaufortfrancois
Copy link
Contributor Author

Sure, we can basically cut releases as we please.

That's great to hear!

@beaufortfrancois beaufortfrancois mentioned this pull request May 27, 2024
Merged
copybara-service bot pushed a commit to google/dawn that referenced this pull request May 27, 2024
@beaufortfrancois
Set surfaceConfiguration alphaMode default value to auto
78a1ef6 
As raised in [1], the default value for surfaceConfiguration alphaMode
should be "auto", not "opaque".

[1]: emscripten-core/emscripten#21939 (comment)

Bug: dawn:2320
Change-Id: I4201f6ab623ee62a2d206121d0fe2ae1eece67cc
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/190080
Reviewed-by: Corentin Wallez <[email protected]>
Commit-Queue: Fr <[email protected]>
kripken pushed a commit that referenced this pull request May 31, 2024
@beaufortfrancois
[WebGPU] Set surfaceConfiguration alphaMode default value to auto (#2…
afa0b8b 
…2006)

As raised in #21939 (comment), the default value for surfaceConfiguration alphaMode
should be "auto", not "opaque".

Relevant Dawn CL: https://dawn-review.googlesource.com/c/dawn/+/190080
@beaufortfrancois
Copy link
Contributor Author

@sbc100 Can we schedule a new release next week?

@sbc100
Copy link
Collaborator

sbc100 commented May 31, 2024

We are working on right now. But we have hit a few snags.

@beaufortfrancois
Copy link
Contributor Author

Thanks!

@beaufortfrancois
Copy link
Contributor Author

Thanks for releasing https://github.com/emscripten-core/emscripten/releases/tag/3.1.61 @sbc100!

msorvig pushed a commit to msorvig/emscripten that referenced this pull request Jun 4, 2024
@beaufortfrancois
[WebGPU] Set surfaceConfiguration alphaMode default value to auto (em…
f079d79 
…scripten-core#22006)

As raised in emscripten-core#21939 (comment), the default value for surfaceConfiguration alphaMode
should be "auto", not "opaque".

Relevant Dawn CL: https://dawn-review.googlesource.com/c/dawn/+/190080
@kainino0x kainino0x mentioned this pull request Sep 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kainino0x kainino0x kainino0x approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WebGPU new Surface API
4 participants
@beaufortfrancois @juj @kainino0x @sbc100