From 9b6f01d603c0f1e1e2e1768d79f56958991e4163 Mon Sep 17 00:00:00 2001 From: "Yan,Shaobo" Date: Tue, 17 Jan 2023 09:29:20 +0000 Subject: [PATCH] Fix flipY logic in CopyTextureForBrowser() CopyTextureForBrowser() handles flipY by flipping source texture then applying copy op. But this is not correct. Instead it should find the copy rect and apply flipY op in copy rect only, which is the same as WebGPU GPUImageCopyExternalImage.flipY definition (https://www.w3.org/TR/webgpu/#dom-gpuimagecopyexternalimage-flipy). This CL fixed the issue and updated related end2end tests. Bug: dawn:1635 Change-Id: I3ea7c9de44fb45224bc438486e0e92385446bfb1 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/116924 Reviewed-by: Corentin Wallez Kokoro: Kokoro Commit-Queue: Shaobo Yan --- .../native/CopyTextureForBrowserHelper.cpp | 30 ++++--------------- .../CopyExternalTextureForBrowserTests.cpp | 18 ++++++----- .../end2end/CopyTextureForBrowserTests.cpp | 13 ++++---- 3 files changed, 22 insertions(+), 39 deletions(-) diff --git a/src/dawn/native/CopyTextureForBrowserHelper.cpp b/src/dawn/native/CopyTextureForBrowserHelper.cpp index 011ca851e7..ea1039f104 100644 --- a/src/dawn/native/CopyTextureForBrowserHelper.cpp +++ b/src/dawn/native/CopyTextureForBrowserHelper.cpp @@ -96,26 +96,7 @@ static const char sCopyForBrowserShader[] = R"( var output : VertexOutputs; output.position = vec4((texcoord[VertexIndex] * 2.0 - vec2(1.0, 1.0)), 0.0, 1.0); - - // Y component of scale is calculated by the copySizeHeight / textureHeight. Only - // flipY case can get negative number. - var flipY = uniforms.scale.y < 0.0; - - // Texture coordinate takes top-left as origin point. We need to map the - // texture to triangle carefully. - if (flipY) { - // We need to get the mirror positions(mirrored based on y = 0.5) on flip cases. - // Adopt transform to src texture and then mapping it to triangle coord which - // do a +1 shift on Y dimension will help us got that mirror position perfectly. - output.texcoords = (texcoord[VertexIndex] * uniforms.scale + uniforms.offset) * - vec2(1.0, -1.0) + vec2(0.0, 1.0); - } else { - // For the normal case, we need to get the exact position. - // So mapping texture to triangle firstly then adopt the transform. - output.texcoords = (texcoord[VertexIndex] * - vec2(1.0, -1.0) + vec2(0.0, 1.0)) * - uniforms.scale + uniforms.offset; - } + output.texcoords = texcoord[VertexIndex] * uniforms.scale + uniforms.offset; return output; } @@ -435,10 +416,11 @@ MaybeError DoCopyForBrowser(DeviceBase* device, sourceInfo->origin.y / static_cast(sourceInfo->size.height) // offset }; - // Handle flipY. FlipY here means we flip the source texture firstly and then - // do copy. This helps on the case which source texture is flipped and the copy - // need to unpack the flip. - if (options->flipY) { + // The NDC to framebuffer space transform maps inverts the Y coordinate such that NDC [-1, 1] + // (resp [-1, -1]) maps to framebuffer space [0, 0] (resp [0, height-1]). So we need to undo + // this flip when converting positions to texcoords. + // https://www.w3.org/TR/webgpu/#coordinate-systems + if (!options->flipY) { uniformData.scaleY *= -1.0; uniformData.offsetY += copySize->height / static_cast(sourceInfo->size.height); } diff --git a/src/dawn/tests/end2end/CopyExternalTextureForBrowserTests.cpp b/src/dawn/tests/end2end/CopyExternalTextureForBrowserTests.cpp index 07ccb93dc6..3bcdc3310b 100644 --- a/src/dawn/tests/end2end/CopyExternalTextureForBrowserTests.cpp +++ b/src/dawn/tests/end2end/CopyExternalTextureForBrowserTests.cpp @@ -124,17 +124,19 @@ class CopyExternalTextureForBrowserTests : public Parent { } std::vector GetDefaultExpectedData(bool flipY, - wgpu::Origin3D origin, + wgpu::Origin3D srcOrigin, wgpu::Extent3D rect) { std::vector expected; - for (uint32_t row = origin.y; row < origin.y + rect.height; ++row) { - for (uint32_t col = origin.x; col < origin.x + rect.width; ++col) { + for (uint32_t rowInRect = 0; rowInRect < rect.height; ++rowInRect) { + for (uint32_t colInRect = 0; colInRect < rect.width; ++colInRect) { + uint32_t row = rowInRect + srcOrigin.y; + uint32_t col = colInRect + srcOrigin.x; + if (flipY) { - uint32_t flippedRow = kHeight - row - 1; - expected.push_back(kDefaultSourceRGBA[flippedRow][col]); - } else { - expected.push_back(kDefaultSourceRGBA[row][col]); + row = (rect.height - rowInRect - 1) + srcOrigin.y; } + + expected.push_back(kDefaultSourceRGBA[row][col]); } } @@ -182,7 +184,7 @@ class CopyExternalTextureForBrowserTests_Basic }; } // anonymous namespace -TEST_P(CopyExternalTextureForBrowserTests_Basic, FullCopy) { +TEST_P(CopyExternalTextureForBrowserTests_Basic, Copy) { DAWN_SUPPRESS_TEST_IF(IsOpenGLES()); DAWN_SUPPRESS_TEST_IF(IsOpenGL() && IsLinux()); diff --git a/src/dawn/tests/end2end/CopyTextureForBrowserTests.cpp b/src/dawn/tests/end2end/CopyTextureForBrowserTests.cpp index 21b1852b6f..2fc78937ec 100644 --- a/src/dawn/tests/end2end/CopyTextureForBrowserTests.cpp +++ b/src/dawn/tests/end2end/CopyTextureForBrowserTests.cpp @@ -280,16 +280,15 @@ class CopyTextureForBrowserTests : public Parent { all(textureLoad(dst, vec2(dstTexCoord), 0) == nonCoveredColor); } else { // Calculate source texture coord. - var srcTexCoord = dstTexCoord - uniforms.dstCopyOrigin + - uniforms.srcCopyOrigin; - // Note that |flipY| equals flip src texture firstly and then do copy from src - // subrect to dst subrect. This helps on blink part to handle some input texture - // which is flipped and need to unpack flip during the copy. - // We need to calculate the expect y coord based on this rule. + var srcTexCoordInRect = dstTexCoord - uniforms.dstCopyOrigin; + + // Note that |flipY| equals flip src texture in copy sub rect. if (uniforms.dstTextureFlipY == 1u) { - srcTexCoord.y = u32(srcSize.y) - srcTexCoord.y - 1u; + srcTexCoordInRect.y = uniforms.copySize.y - srcTexCoordInRect.y - 1; } + var srcTexCoord = srcTexCoordInRect + uniforms.srcCopyOrigin; + var srcColor = textureLoad(src, vec2(srcTexCoord), 0); var dstColor = textureLoad(dst, vec2(dstTexCoord), 0);