Bug 294223 – WebGPU 1D texture sampling bug

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Steven Wittens]

Created attachment 475516 [details]
example misaligned palette texture (off by 4 texels).. there should be no green

In the following WebGPU demo from Use.GPU , a 1D texture is used to store and sample the palette for the voxels:

https://usegpu.live/demo/geometry/voxel

(format: rgba8unorm-srgb, texture_1d<f32>)

In Safari Tech Preview, the palette is randomly corrupt on re-loading the page, though it remains constant during a particular run. You can also trigger this by pressing Next+Previous at the bottom (which does not reload the page, but reloads the texture).

Tweaking the shader suggests that instead of accessing texel 'i' it is sometimes accessing texel 'i - 4', 'i - 8' or 'i - 12' and does so consistently for a given texture. i.e. It looks like a misalignment issue when uploading the texture to the GPU.

There is also this 6 year old bug that suggests all non-2D textures are broken, however I am not sure it's true, given that the voxel data is a 3D texture (texture_3d(u32), r8uint) and it works fine.
https://bugs.webkit.org/show_bug.cgi?id=201384

[Radar WebKit Bug Importer]

<rdar://problem/152880664>

[Mike Wyrzykowski]

https://bugs.webkit.org/show_bug.cgi?id=201384 is only applicable potentially on Tier1 devices, on macOS Sequoia no such devices exist is my understanding.

[Mike Wyrzykowski]

Checking the gputrace, if you'd like to try as well here are the instructions:
``````
(Launch Safari or Safari Technology Preview) __XPC_METAL_CAPTURE_ENABLED=1 open /Applications/Safari.app 
(Capture a trace) notifyutil -p com.apple.WebKit.WebGPU.CaptureFrame
(Find where the trace was saved - I usually use Console.app and search for “gputrace")

````
By default it captures one MTLCommandBuffer, you can change it via:
```
(Set frame count to 10) notifyutil -s com.apple.WebKit.WebGPU.CaptureFrame 10
(Capture a trace) notifyutil -p com.apple.WebKit.WebGPU.CaptureFrame
```

[Mike Wyrzykowski]

The issue seems to be in the blit command encoder path.

Forcing the replaceRegion path:
https://github.com/WebKit/WebKit/blob/66fc5e188133f37c2990801c5c665825dbe85243/Source/WebGPU/WebGPU/Queue.mm#L1006

and the issue is not reproducible.

Presumably we have an issue around this line:
https://github.com/WebKit/WebKit/blob/66fc5e188133f37c2990801c5c665825dbe85243/Source/WebGPU/WebGPU/Queue.mm#L1147

[Mike Wyrzykowski]

Forcing the no copy path (set noCopy = true) and the issue is similarly not reproducible:
https://github.com/WebKit/WebKit/blob/66fc5e188133f37c2990801c5c665825dbe85243/Source/WebGPU/WebGPU/Queue.mm#L1109

[Mike Wyrzykowski]

The bug appears to be in buffer sub allocation in https://github.com/WebKit/WebKit/blob/66fc5e188133f37c2990801c5c665825dbe85243/Source/WebGPU/WebGPU/Queue.mm#L586

[Mike Wyrzykowski]

aligning to 64 bytes instead of 16 resolves the issue but I don't know if I'm just working around the issue

[Mike Wyrzykowski]

diff --git a/Source/WebGPU/WebGPU/Queue.mm b/Source/WebGPU/WebGPU/Queue.mm
index 228a7915f865..a7af1da907c7 100644
--- a/Source/WebGPU/WebGPU/Queue.mm
+++ b/Source/WebGPU/WebGPU/Queue.mm
@@ -600,7 +601,7 @@ std::pair<id<MTLBuffer>, uint64_t> Queue::newTemporaryBufferWithBytes(std::span<
     }
 
     auto priorOffset = m_temporaryBufferOffset;
-    m_temporaryBufferOffset += WTF::roundUpToMultipleOf(16, dataSize);
+    m_temporaryBufferOffset += WTF::roundUpToMultipleOf(64, dataSize);
     memcpySpan(span(m_temporaryBuffer).subspan(priorOffset), dataSpan);
     return std::make_pair(m_temporaryBuffer, priorOffset);
 }

[Mike Wyrzykowski]

Docs says it should only need to be pixel aligned - https://developer.apple.com/documentation/metal/mtlblitcommandencoder/copy(from:sourceoffset:sourcebytesperrow:sourcebytesperimage:sourcesize:to:destinationslice:destinationlevel:destinationorigin:options:)?language=objc

Largest pixel should by 16 bytes afaik

[Mike Wyrzykowski]

Pull request: https://github.com/WebKit/WebKit/pull/46615

[Mike Wyrzykowski]

I need to make a tiny test case in C++ to prove this is a Metal issue.

[Mike Wyrzykowski]

So it appears passing 0 to sourceBytesPerRow is problematic.

[Mike Wyrzykowski]

Passing the correct value for sourceBytesPerRow and we don't need to align to 64 bytes

[Mike Wyrzykowski]

I would have expected a Metal validation error if this is a requirement, perhaps missing in the API validation layer

[EWS]

Committed 296260@main (1b6e6ca8f0c2): <https://commits.webkit.org/296260@main>

Reviewed commits have been landed. Closing PR #46615 and removing active labels.