Skip to content

Loosen viewport validation requirements #5025

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.

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

Merged
merged 3 commits into from
Jan 31, 2025
Merged

Conversation

toji
Copy link
Member

@toji toji commented Dec 17, 2024

Fixes #373

Simplifies the Vulkan model a bit by ignoring the fact that the maximum viewport size may be larger than the largest possible 2D texture size. So the logic becomes that the viewports must be no larger than maxTextureDimension2D and cannot be extend past -maxTextureDimension2D * 2 or (maxTextureDimension2D * 2) - 1 on either axis.

Which should be enough for anybody!*

*There's definitely no way that this statement will come back to haunt us. Nope.

@toji toji requested review from kainino0x and jimblandy December 17, 2024 19:35
Copy link
Contributor

github-actions bot commented Dec 17, 2024

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

Copy link
Contributor

@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.

Spec LGTM but we should verify that these limits are going to work in Metal and D3D12. I'm pretty sure it was determined on the bug that Metal would be fine, but I don't think anyone has looked at D3D. The answer can probably be found in the D3D11 functional spec though.

@toji
Copy link
Member Author

toji commented Dec 18, 2024

D3D11 does have limits on the viewport size, as noted in the D3D11_VIEWPORT struct docs. Specifically it says the x and y coords must lie between D3D11_VIEWPORT_BOUNDS_MIN and D3D11_VIEWPORT_BOUNDS_MAX.

However, the D3D12 spec has no such limits mentioned in the D3D12_VIEWPORT struct docs, and I can't find any other docs that mention a limit in D3D12. As such, I'm inclined to assume that there is no limit, similar to Metal, but I would love confirmation from someone at Microsoft on that point. @RafaelCintron?

@kainino0x
Copy link
Contributor

kainino0x commented Dec 19, 2024

It's probably a safe guess that D3D12 has the same limits as D3D11, and in any case, it does have these constants, so presumably they're correct:
https://github.com/microsoft/DirectX-Headers/blob/06d1185aa7ba650f0f5ae25b9cb39b8a85573bbe/include/directx/d3d12.h#L1374-L1376

We should note these in the computation of maxTextureDimension2D on D3D12:

<td>16384 = `min(D3D12_REQ_TEXTURE2D_U_OR_V_DIMENSION, D3D12_REQ_TEXTURECUBE_DIMENSION)`

Even though it doesn't make any difference since all of the D3D12 limits are hardcoded anyway.

@toji
Copy link
Member Author

toji commented Dec 19, 2024

Thanks for locating that! I tried searching for D3D12_VIEWPORT_BOUNDS_MAX directly on Google and it simply didn't show up. 🙄.

Added related values to the correspondence doc.

@kainino0x kainino0x added this to the Milestone 1 milestone Jan 21, 2025
mwyrzykowski
mwyrzykowski previously approved these changes Jan 29, 2025
@kainino0x kainino0x added the api WebGPU API label Jan 29, 2025
@mwyrzykowski mwyrzykowski dismissed their stale review January 29, 2025 20:06

2 * seems a bit limiting on second though

@kainino0x kainino0x added the api resolved Resolved - waiting for a change to the API specification label Jan 29, 2025
@toji
Copy link
Member Author

toji commented Jan 29, 2025

Notes from today's call:

  • Group feels like this is OK to accept.
  • It would be good to do testing across more hardware to see if there are any common driver bugs in this area.
    • If so, polyfilling the behavior should be investigated, which may be complicated.
  • There may be some considerations for compat mode, though it's worth noting that this does NOT introduce the ability to specify negative viewport widths or heights.
  • It is worth investigating how viewports interact with foveated/variable rate shading. It was pointed out that the common pattern in Metal may require a significantly larger viewport than the render target in some cases. If that's the case, however, this change should still strictly be an improvement, with additional loosening being considered in the future.

@mwyrzykowski
Copy link

mwyrzykowski commented Jan 29, 2025

@toji here is the reference to the Metal VRR map documentation https://developer.apple.com/documentation/metal/mtlrasterizationratemap?language=objc#Configuring-the-Rate-Map and https://developer.apple.com/documentation/metal/mtlrasterizationratemap/physicalcoordinates(screencoordinates:layer:)?language=objc

This is probably a separate topic, but at the framework level the viewport in Metal can be arbitrary sized based on the VRR map. Hiding this fact at the WebGPU level is probably wrong too since then the viewport region of size W x H would map to N x M pixels where W*H is not necessarily N*M, but as discussed this is overall an improvement over the existing spec. But perhaps this will not be sufficient in the future.

@greggman
Copy link
Contributor

I commented in the WG meeting I thought there were GL issues but I'm pretty sure I was mistaken. There are no GL issues

width >= 0 and width <= maxTextureDimenion2D
height >= 0 and height <= maxTextureDimenion2D

No restrictions on x and y listed.

To get into the weeds. It says you can query the max width,height from GL_MAX_VIEWPORT_DIMS but it requires the values returned to be effectively >= what's returned by querying GL_MAX_TEXTURE_SIZE

@kainino0x
Copy link
Contributor

Great, I think that means we can merge this! @toji would you mind addressing the two open comments for the correspondence doc first though?

@toji toji force-pushed the unlock-viewports branch from 0c78f60 to 37303fa Compare January 30, 2025 23:30
@toji
Copy link
Member Author

toji commented Jan 30, 2025

Correspondence changes made, thanks! PTAL, I think this should be ready to land now.

@kainino0x kainino0x merged commit 81f3cd7 into gpuweb:main Jan 31, 2025
3 of 4 checks passed
@toji toji deleted the unlock-viewports branch February 1, 2025 18:31
copybara-service bot pushed a commit to google/dawn that referenced this pull request Feb 13, 2025
Updates Dawn to match the new viewport validation introduced in
gpuweb/gpuweb#5025.

Bug: 390162929
Change-Id: I149d601a2e11a9ce50629a4e00b53d51042a8713
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/219974
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Brandon Jones <bajones@chromium.org>
@Kangz
Copy link
Contributor

Kangz commented Feb 18, 2025

GPU Web WG 2025-01-29 Atlantic-time
  • BJ: Original restriction was here because of Metal spec at the time, but it was wrong and was removed from Metal. Only complication here is that VK is wishywashy about what the maximums are: Bounds are (at least?) [-maxTex2dSize2,maxTex2dSize2-1]. Loosen viewport validation requirements #5025 matches that.
  • JB: Works on all backend APIs? How confident are we that that it works on real devices?
  • BJ: Started writing tests but wanted to discuss in WG first.
  • JB: Is this possible to polyfill if we encounter a buggy device where it doesn't work?
  • BJ: There was some discussion on the issue about polyfilling, I know it's not trivial, might be doable.
  • GT: GL doesn't seem to support this. Would need to add a compat restriction
  • SW: Talked about polyfilling it on GL but doesn't seem super easy.
  • GL: What's the restriction on GL?
  • GT: (reading some spec text)
  • GL: Negative width/height is a different thing.
  • BJ: Negative width/height not added in this PR.
    • AB: Vulkan allows negative height but not negative width.
  • GT: Will check outside the meeting, maybe it works.
  • MW: Considered how this interacts with foveated rendering or variable-rate rendering (VRR), texture size is much smaller than the viewport size.
  • BJ: Think viewport will act as usual - before rasterization -
  • MW: In Metal if you set a VRR map and the viewport is going to be larger than the texture size, it probably usually falls within these bounds but not necessarily. Say a 16Kx16K texture… with [a higher density region in the middle][?] viewport will be 32Kx32K
  • BJ: (something…) probably wouldn't function at all
  • KG: Think we had not considered it, but this change is strictly better than before. Only might need to further improve it later for VRR.
  • MW: Agree.
  • Approved, needs testing, need to check whether a new Compat restriction needs to be introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api resolved Resolved - waiting for a change to the API specification api WebGPU API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Viewport rect must lie entirely within the current attachment size?
5 participants