-
Notifications
You must be signed in to change notification settings - Fork 330
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
Conversation
Previews, as seen when this build job started (37303fa): |
There was a problem hiding this 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.
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 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? |
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: We should note these in the computation of maxTextureDimension2D on D3D12: gpuweb/correspondence/index.bs Line 121 in 254de56
Even though it doesn't make any difference since all of the D3D12 limits are hardcoded anyway. |
Thanks for locating that! I tried searching for Added related values to the correspondence doc. |
2 * seems a bit limiting on second though
Notes from today's call:
|
@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 |
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
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 |
Great, I think that means we can merge this! @toji would you mind addressing the two open comments for the correspondence doc first though? |
0c78f60
to
37303fa
Compare
Correspondence changes made, thanks! PTAL, I think this should be ready to land now. |
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>
GPU Web WG 2025-01-29 Atlantic-time
|
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.