Skip to content

webgpu: step 1 in fixing test_Scissor by fixing WebGPUDriver::readPixels with mipmap #9629

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

Open
yrcloud wants to merge 3 commits into
base: main
Choose a base branch
from
+19 −10
Open

webgpu: step 1 in fixing test_Scissor by fixing WebGPUDriver::readPixels with mipmap #9629

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
863379b
wgpu: fix the readPixels function so test_Scissor can draw the triangle
yrcloud Jan 23, 2026
cf694a3
wgpu: add TextureUsage::SAMPLEABLE to the depth texture in test_Scissor,
yrcloud Jan 23, 2026
0ad90d1
wgpu: code clean up based on feedback
yrcloud Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions filament/backend/src/webgpu/WebGPUDriver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1472,6 +1472,7 @@ void WebGPUDriver::readPixels(Handle<HwRenderTarget> sourceRenderTargetHandle, c
const auto srcTarget{ handleCast<WebGPURenderTarget>(sourceRenderTargetHandle) };
assert_invariant(srcTarget);

uint32_t srcMipLevel = 0;
wgpu::Texture srcTexture{ nullptr };
if (srcTarget->isDefaultRenderTarget()) {
assert_invariant(mSwapChain);
Expand All @@ -1482,6 +1483,7 @@ void WebGPUDriver::readPixels(Handle<HwRenderTarget> sourceRenderTargetHandle, c
// TODO we are currently assuming the first attachment is the desired texture.
if (colorAttachmentInfos[0].handle) {
auto texture = handleCast<WebGPUTexture>(colorAttachmentInfos[0].handle);
srcMipLevel = colorAttachmentInfos[0].level;
if (texture) {
srcTexture = texture->getTexture();
}
Expand All @@ -1494,15 +1496,21 @@ void WebGPUDriver::readPixels(Handle<HwRenderTarget> sourceRenderTargetHandle, c
return;
}
const uint32_t srcWidth {srcTexture.GetWidth()};
const uint32_t srcHeight{srcTexture.GetHeight()};
const uint32_t srcHeight {srcTexture.GetHeight()};
const uint32_t srcMipLevelCount {srcTexture.GetMipLevelCount()};

const uint32_t mipLevelImageScale = 1 << srcMipLevel;
const uint32_t mipLeveledSrcWidth = srcWidth / mipLevelImageScale;
const uint32_t mipLeveledSrcHeight = srcHeight / mipLevelImageScale;
assert_invariant(mipLeveledSrcWidth > 0 && mipLeveledSrcHeight > 0);

// Clamp read region to texture bounds
if (UTILS_UNLIKELY(x >= srcWidth || y >= srcHeight)) {
if (UTILS_UNLIKELY(x >= mipLeveledSrcWidth || y >= mipLeveledSrcHeight)) {
scheduleDestroy(std::move(pixelBufferDescriptor));
return;
}
auto actualWidth{ std::min(width, srcWidth - x) };
auto actualHeight{ std::min(height, srcHeight - y)};
auto actualWidth{ std::min(width, mipLeveledSrcWidth - x) };
auto actualHeight{ std::min(height, mipLeveledSrcHeight - y)};
Comment on lines +1511 to +1512
Copy link
Copy Markdown
Collaborator

@pixelflinger pixelflinger Jan 26, 2026

Choose a reason for hiding this comment

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

I don't understand why the -x and -y here?

Copy link
Copy Markdown
Collaborator Author

@yrcloud yrcloud Jan 29, 2026

Choose a reason for hiding this comment

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

The x and y are offsets, so subtracting them from the entire texture's size (at the requested mipmap level) gives you the remaining maximum size that can be copied, from the beginning offsetted corner to the diagonally opposite corner of the entire texture.

if (UTILS_UNLIKELY(actualWidth == 0 || actualHeight == 0)) {
scheduleDestroy(std::move(pixelBufferDescriptor));
return;
Expand All @@ -1526,6 +1534,7 @@ void WebGPUDriver::readPixels(Handle<HwRenderTarget> sourceRenderTargetHandle, c
// If the source format is different from the destination (e.g. BGRA vs RGBA),
// we need to perform a conversion using an intermediate blit.
if (conversionNecessary(srcFormat, dstFormat, pixelBufferDescriptor.type)) {
// TODO: check if the blit process here is correct when mipmap level > 0
const wgpu::TextureDescriptor stagingDescriptor{
.label = "readpixels_staging_texture",
.usage = wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::RenderAttachment,
Expand All @@ -1536,7 +1545,7 @@ void WebGPUDriver::readPixels(Handle<HwRenderTarget> sourceRenderTargetHandle, c
.depthOrArrayLayers = 1,
},
.format = dstFormat,
.mipLevelCount = 1,
.mipLevelCount = srcMipLevelCount,
Copy link
Copy Markdown
Contributor

@z3moon z3moon Jan 26, 2026

Choose a reason for hiding this comment

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

reading code, I think you need to pass BlitArgs::Attachment::mipLevel for the blit operation below to correctly point to the level for the copy. It defaults to 0 and has only 1 operation atm. So I think only level 0 is copied even though srcMipLevelCount > 1.

I'm not sure if it's worth copying all levels for this. But if so, you probably need to iterate through all levels with the blit call below. Otherwise, you may as well revert this along with the other comment below.

Copy link
Copy Markdown
Collaborator Author

@yrcloud yrcloud Jan 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

So I plan to NOT resolve issues about blit in this change, reasons being:

  • The target test of this change, namely test_Scissor, does not need blit, i.e., conversionNecessary returns false
  • It seems that the current webgpu implementation does not consider mipmap at all, I suspect the implementation of blit may have many issues we'd have to look through
  • The test_Blit is broken and skipped on webgpu too, I plan to look at that after I fix this test_Scissor anyway
    So overall I suggest I revert these changes related to blit (whatever is inside conversionNecessary branch) and investigate it later when looking at test_Blit, I'll keep the TODO on line 1537.

.sampleCount = srcTexture.GetSampleCount(),
};
stagingTexture = mDevice.CreateTexture(&stagingDescriptor);
Expand Down Expand Up @@ -1586,12 +1595,11 @@ void WebGPUDriver::readPixels(Handle<HwRenderTarget> sourceRenderTargetHandle, c

// WebGPU's texture coordinates for copies are top-left, but Filament's y-coordinate is
// bottom-left. We must flip the y-coordinate relative to the texture we are reading from.
const uint32_t textureHeight{ textureToReadFrom.GetHeight() };
const uint32_t flippedY{ textureHeight - readY - actualHeight };
const uint32_t flippedY{ mipLeveledSrcHeight - readY - actualHeight };

const wgpu::TexelCopyTextureInfo source{
.texture = textureToReadFrom, // Read from the original or intermediate texture
.mipLevel = 0,
.mipLevel = srcMipLevel,
Copy link
Copy Markdown
Contributor

@z3moon z3moon Jan 26, 2026

Choose a reason for hiding this comment

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

if we use the intermediate texture (stagingTexture), do we still need to specify this as srcMipLevel? not 0? I guess the blit call will copy contents only for level 0.

Copy link
Copy Markdown
Collaborator Author

@yrcloud yrcloud Jan 29, 2026

Choose a reason for hiding this comment

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

You are right, but similar to the above reply, I suggest we not fix blit related issues in this change. So here we assume textureToReadFrom comes from either the original texture (who needs the correct mip level) or the intermediate texture with a mip level of 0. Intermediate texture + mip level > 0 is broken anyway, we fix it later in test_Blit.

.origin = {.x = readX, .y = flippedY, .z = 0,},
};
const wgpu::TexelCopyBufferInfo destination{
Expand Down
6 changes: 4 additions & 2 deletions filament/backend/test/test_Scissor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,10 @@ TEST_F(BackendTest, ScissorViewportRegion) {
Handle<HwTexture> srcTexture = addCleanup(api.createTexture(SamplerType::SAMPLER_2D,
kNumLevels, kSrcTexFormat, 1, kSrcTexWidth, kSrcTexHeight, 1,
TextureUsage::SAMPLEABLE | TextureUsage::COLOR_ATTACHMENT TEXTURE_USAGE_READ_PIXELS));
Handle<HwTexture> depthTexture = addCleanup(api.createTexture(SamplerType::SAMPLER_2D, 1,
TextureFormat::DEPTH16, 1, 512, 512, 1, TextureUsage::DEPTH_ATTACHMENT));

Handle<HwTexture> depthTexture =
addCleanup(api.createTexture(SamplerType::SAMPLER_2D, 1, TextureFormat::DEPTH16, 1,
512, 512, 1, TextureUsage::DEPTH_ATTACHMENT | TextureUsage::SAMPLEABLE));
Copy link
Copy Markdown
Collaborator

@pixelflinger pixelflinger Jan 26, 2026

Choose a reason for hiding this comment

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

I don't understand why SAMPLEABLE is needed?

Copy link
Copy Markdown
Collaborator Author

@yrcloud yrcloud Jan 29, 2026

Choose a reason for hiding this comment

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

tbh I don't know the exact reason either... I just found without it webgpu would crash, adding it (or not using a depth attachment) makes it work.
I'll investigate further about this.


// Render into the bottom-left quarter of the texture.
Viewport srcRect = {
Expand Down
Loading