Sitelet https://github.com/gpuweb/gpuweb/pull/4621
Skip to content

Add optional feature dual_source_blending - #4621

Merged
kainino0x merged 43 commits into
gpuweb:mainfrom
Jiawei-Shao:add-dual-source-blending
Jun 18, 2024
Merged

Add optional feature dual_source_blending#4621
kainino0x merged 43 commits into
gpuweb:mainfrom
Jiawei-Shao:add-dual-source-blending

Conversation

@Jiawei-Shao

@Jiawei-Shao Jiawei-Shao commented May 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

This patch adds dual_source_blending to WebGPU and WGSL SPEC.

  • Add dual_source_blending as an optional feature in both WebGPU and WGSL.
  • Support @blend_src in WGSL when dual_source_blending is enabled.
  • Add all the blend factors that use src1 to WebGPU SPEC.

Fixed: #4283

alan-baker
alan-baker reviewed May 6, 2024
View reviewed changes
Comment thread wgsl/index.bs Outdated
@alan-baker alan-baker added wgsl WebGPU Shading Language Issues api WebGPU API labels May 6, 2024
@github-actions

github-actions Bot commented May 7, 2024 •
edited by mehmetoguzderin
Loading

Copy link
Copy Markdown
Contributor

Previews, as seen when this build job started (b98a4b0):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

kainino0x
kainino0x reviewed May 7, 2024
View reviewed changes
Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
alan-baker
alan-baker reviewed May 7, 2024
View reviewed changes
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
kainino0x
kainino0x reviewed May 8, 2024
View reviewed changes

@kainino0x kainino0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still some issues in the validation algorithm; it was too complicated to try to explain what should be done so I've pushed a commit. Please review my changes!

Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
Comment thread spec/index.bs Outdated
@Jiawei-Shao

Jiawei-Shao commented May 8, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @kainino0x and @alan-baker !

I've merged the main branch into the PR and fixed several nits in Kai's comment. PTAL ,thanks!

@kainino0x
kainino0x requested a review from toji May 8, 2024 17:59
@kainino0x

kainino0x commented May 8, 2024

Copy link
Copy Markdown
Contributor

fixed several nits in Kai's comment

Thanks for the fixes!

API text LGTM (except that I wrote part of it) but I haven't checked if it matches the validation in the backends. Will need tests.

toji
toji reviewed May 8, 2024
View reviewed changes

@toji toji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One request for the WebGPU spec, but overall it's looking good! I'm not in a good position to evaluate the WGSL spec changes.

Comment thread spec/index.bs
@kainino0x kainino0x added this to the Milestone 1 milestone May 8, 2024
@toji

toji commented May 9, 2024

Copy link
Copy Markdown
Member

Thanks for adding the feature column to the blend factor table! LGTM, though I'll wait for someone on the WGSL spec side to give final approval.

alan-baker
alan-baker reviewed May 10, 2024
View reviewed changes
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
Comment thread wgsl/index.bs Outdated
@Jiawei-Shao
Require all the @location(0) @blend_src(N) outputs have same type
df42b25 
As is required by Metal Shading Language:
```
5.2.3.5 Fragment Function Output Attributes
Multiple elements in the fragment function return type that use the
same color attachment index for blending needs to be declared with
the same data type.
```
teoxoy
teoxoy reviewed May 29, 2024
View reviewed changes
Comment thread spec/index.bs
@Jiawei-Shao

Jiawei-Shao commented Jun 4, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @alan-baker!

@kainino0x could you take a look?

dneto0
dneto0 requested changes Jun 4, 2024
View reviewed changes

@dneto0 dneto0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall looks good.

I have one suggestion: forbid use of blend_src without also having a location attribute.

Comment thread wgsl/index.bs Outdated
teoxoy
teoxoy approved these changes Jun 4, 2024
View reviewed changes

@teoxoy teoxoy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks consistent with what we previously agreed on. 👍

@kdashg

kdashg commented Jun 4, 2024

Copy link
Copy Markdown
Contributor
WGSL 2024-06-04 Minutes
  • (KG: Are we happy with the WGSL half of the PR?)
  • DN: Was okay with this. Requested to restrict blend-source so that’s only usable when you have a location to use with it. Let’s make a rule to make sure people don’t think they’re getting blend-source when they can’t: forbid it when it’s not meaningful.
  • AB: That’s restating rules that appear later on in the entry points section, but I don’t mind the rules appearing in the attribute section too. Otherwise I signed off on this after some review
  • KG: Teo approved this too.
  • DS: So are we going to land?
  • JB: Ok to land from our perspective
  • DS: Need CTS?
  • KG: Nice to have CTS, but don’t block on it. (Expect it won’t change due to CTS).
  • MARK AS WGSL RESOLVED. LAND after API side happy with it.

@kdashg kdashg added the wgsl resolved Resolved - waiting for a change to the WGSL specification label Jun 4, 2024
kainino0x
kainino0x approved these changes Jun 7, 2024
View reviewed changes

@kainino0x kainino0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Comment thread wgsl/index.bs Outdated
@kainino0x
kainino0x requested a review from dneto0 June 7, 2024 22:27
dneto0
dneto0 approved these changes Jun 13, 2024
View reviewed changes

@dneto0 dneto0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Sorry for the delay.

@Jiawei-Shao

Jiawei-Shao commented Jun 18, 2024

Copy link
Copy Markdown
Contributor Author

@kainino0x can we merge this PR right now?

@Kangz

Kangz commented Jun 18, 2024

Copy link
Copy Markdown
Contributor

Yes we can merge.

@kainino0x
kainino0x merged commit 6eeebff into gpuweb:main Jun 18, 2024
@kainino0x kainino0x mentioned this pull request Jun 20, 2024
Merged
@Kangz

Kangz commented Jun 25, 2024

Copy link
Copy Markdown
Contributor
GPU Web WG 2024-05-29 Atlantic-time
  • JS: discussed this feature last year. Some issues while creating the PR. Kai raised a blocking issue.
  • KN: trying to figure out validation rules in the WGSL spec. If you put blend_src on anything, you need exactly 2 locations? Blend sources 0 and 1, on the entry point? And different locations, can't use blend_src at all? Validate this eagerly in WGSL? Trying to figure out how to actually write this down.
  • JS: agree, no big issues, just spec wording.
  • KG: anything you wanted feedback from the group on?
  • JS: only updated the PR, found some new info on Mac/Metal - outputs have to have the same type when doing dual-source blending, or it will error. Simplifies this PR a lot, actually.
  • KG: need approvals on the PR.
  • KN: needs editorial work; we'll bring this back here, maybe don't need to wait for a Pacific time meeting.

juj added a commit to juj/wasm_webgpu that referenced this pull request Sep 6, 2024
@juj
IDL: Add support for dual source blending. gpuweb/gpuweb#4621. gpuweb…
0611911 
…/gpuweb#4710
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dj2 dj2 dj2 left review comments
@toji toji toji left review comments
@kainino0x kainino0x kainino0x approved these changes
@dneto0 dneto0 dneto0 approved these changes
@teoxoy teoxoy teoxoy approved these changes
@alan-baker alan-baker alan-baker approved these changes
+1 more reviewer
@Devwanabe Devwanabe Devwanabe left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api WebGPU API wgsl resolved Resolved - waiting for a change to the WGSL specification wgsl WebGPU Shading Language Issues

Projects

None yet

Milestone

Milestone 1

Development

Successfully merging this pull request may close these issues.

Dual Source Blending Investigation + Feature Proposal

10 participants

@Jiawei-Shao @kainino0x @toji @Kangz @kdashg @dj2 @dneto0 @teoxoy @alan-baker @Devwanabe