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] mip level validation is not handled correctly in copyBuffer/TextureToBuffer/Texture #11727

Closed
wants to merge 1 commit into from
Closed

[WebGPU] mip level validation is not handled correctly in copyBuffer/TextureToBuffer/Texture #11727

wants to merge 1 commit into from

Conversation

mwyrzykowski
Copy link
Contributor

@mwyrzykowski mwyrzykowski commented Mar 20, 2023 •
edited by webkit-early-warning-system

77d269e

[WebGPU] mip level validation is not handled correctly in copyBuffer/TextureToBuffer/Texture
https://bugs.webkit.org/show_bug.cgi?id=254175
<radar://106958194>

Reviewed by NOBODY (OOPS!).

The correct sizes were already computed in logicalMiplevelSpecificTextureExtent
but they were not used for validation.

* Source/WebGPU/WebGPU/CommandEncoder.mm:
(WebGPU::CommandEncoder::copyBufferToTexture):
(WebGPU::CommandEncoder::copyTextureToBuffer):
* Source/WebGPU/WebGPU/Queue.mm:
(WebGPU::Queue::writeTexture):
3D textures with rowBytes > 2048 will assert in Metal in debug
and release.

* Source/WebGPU/WebGPU/Texture.mm:
(WebGPU::Texture::validateLinearTextureData):
Metal requires the copy size to be a multiple of the block size.

77d269e

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@mwyrzykowski mwyrzykowski self-assigned this Mar 20, 2023
@mwyrzykowski mwyrzykowski added the WebGPU For bugs in WebGPU label Mar 20, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 986ca43)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
⏳ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

mwyrzykowski
mwyrzykowski commented Mar 20, 2023
View reviewed changes
Source/WebGPU/WebGPU/CommandEncoder.mm
|| destination.origin.x + widthForMetal > logicalSize.width
|| destination.origin.y + heightForMetal > logicalSize.height
|| destination.origin.z + depthForMetal > logicalSize.depthOrArrayLayers
|| sourceBytesPerRow > 2048) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe 2048 should be from one of the device limits?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't find a device limit, please document with a FIXME to come back and fix it up later.

djg
djg approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@djg djg left a comment

Choose a reason for hiding this comment

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

:shipit:

mwyrzykowski
mwyrzykowski commented Mar 21, 2023
View reviewed changes
Source/WebGPU/WebGPU/CommandEncoder.mm
if (!widthForMetal || !heightForMetal)
return;

if (widthForMetal * heightForMetal > destinationBuffer.length) {
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 think this is wrong, at least it causes a regression in compressed texture formats copyTextureToTexture cases

@mwyrzykowski
[WebGPU] mip level validation is not handled correctly in copyBuffer/…
77d269e 
…TextureToBuffer/Texture

https://bugs.webkit.org/show_bug.cgi?id=254175
<radar://106958194>

Reviewed by NOBODY (OOPS!).

The correct sizes were already computed in logicalMiplevelSpecificTextureExtent
but they were not used for validation.

* Source/WebGPU/WebGPU/CommandEncoder.mm:
(WebGPU::CommandEncoder::copyBufferToTexture):
(WebGPU::CommandEncoder::copyTextureToBuffer):
* Source/WebGPU/WebGPU/Queue.mm:
(WebGPU::Queue::writeTexture):
3D textures with rowBytes > 2048 will assert in Metal in debug
and release.

* Source/WebGPU/WebGPU/Texture.mm:
(WebGPU::Texture::validateLinearTextureData):
Metal requires the copy size to be a multiple of the block size.
@mwyrzykowski mwyrzykowski force-pushed the eng/WebGPU-mip-level-validation-is-not-handled-correctly-in-copyBufferTextureToBufferTexture branch from 986ca43 to 77d269e Compare April 18, 2023 22:15
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 77d269e. Live statuses available at the PR page, #11727

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 77d269e)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 19, 2023
@mwyrzykowski
Copy link
Contributor Author

Closing since some of this validation is incorrect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djg djg djg approved these changes

@litherum litherum Awaiting requested review from litherum

@tadeuzagallo tadeuzagallo Awaiting requested review from tadeuzagallo tadeuzagallo is a code owner

Assignees

@mwyrzykowski mwyrzykowski

Labels
merging-blocked Applied to prevent a change from being merged WebGPU For bugs in WebGPU
Projects
None yet
Milestone
No milestone
4 participants
@mwyrzykowski @webkit-early-warning-system @djg @webkit-ews-buildbot