Bug 222685 - Metal-ANGLE: Shared memory texture tests failing in iOS Simulator
Summary: Metal-ANGLE: Shared memory texture tests failing in iOS Simulator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-03 13:47 PST by Dean Jackson
Modified: 2021-04-29 17:22 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.42 KB, patch)
2021-04-16 17:33 PDT, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (8.50 KB, patch)
2021-04-16 17:58 PDT, Dean Jackson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (4.64 KB, patch)
2021-04-27 15:39 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (8.64 KB, patch)
2021-04-27 18:05 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2021-04-28 11:18 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (10.30 KB, patch)
2021-04-28 11:19 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (10.34 KB, patch)
2021-04-29 11:52 PDT, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2021-04-29 12:11 PDT, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2021-04-29 12:33 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2021-03-03 13:47:18 PST
The following tests fail on the iOS Simulator with the ANGLE Metal backend.

  webgl/1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-array-buffer-view.html [ Failure ]
  webgl/1.0.3/conformance/textures/texture-mips.html [ Failure ]

According to Kyle, the issue is the resource type of the textures (which can't be shared on Intel macOS), and the simulator code not expecting that.
Comment 1 Radar WebKit Bug Importer 2021-03-03 13:47:27 PST
<rdar://problem/75001722>
Comment 2 Kyle Piddington 2021-04-16 17:33:56 PDT
Created attachment 426297 [details]
Patch
Comment 3 EWS Watchlist 2021-04-16 17:34:44 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Dean Jackson 2021-04-16 17:58:26 PDT
Created attachment 426301 [details]
Patch
Comment 5 EWS 2021-04-16 19:06:22 PDT
Committed r276190 (236672@main): <https://commits.webkit.org/236672@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426301 [details].
Comment 6 Truitt Savell 2021-04-23 11:16:39 PDT
Reverted r276190 for reason:

broke a test internally.

Committed r276507 (236964@main): <https://commits.webkit.org/236964@main>
Comment 7 Kyle Piddington 2021-04-27 15:39:17 PDT
Created attachment 427205 [details]
Patch
Comment 8 Kyle Piddington 2021-04-27 18:05:40 PDT
Created attachment 427224 [details]
Patch
Comment 9 Kyle Piddington 2021-04-28 11:18:10 PDT
Created attachment 427281 [details]
Patch
Comment 10 Kyle Piddington 2021-04-28 11:19:08 PDT
Created attachment 427282 [details]
Patch
Comment 11 Kenneth Russell 2021-04-28 19:14:54 PDT
Comment on attachment 427282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427282&action=review

Looks good to me. Couple of small comments and suggestions. It looks like the win EWS failure is an unrelated flake. r+

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:308
> +    size_t stagingBufferSize        = stagingBuffer2DImageSize * regionSize.depth;

Are there any padding constraints for certain texture formats, in particular on the row pitch?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:337
> +    size_t stagingBuffer2DImageSize = bytesPer2DImage;

This helper function is identical to the one above except for the computation of stagingBuffer2DImageSize. Would it be worth coalescing them into one with a "bool isCompressedFormat" argument or similar?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:490
> +    //Block-compressed formats need a bit of massaging for copy.

Space after // (clang-format will fix this during upstreaming)

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:496
> +        //ASSERT(region.size.height % fmt.compressedBlockHeight == 0);

Remove commented-out code.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:535
> +        ANGLE_TRY(UploadTextureContentsWithStagingBuffer(contextMtl,textureAngleFormat,region,mipmapLevel,slice,data,bytesPerRow,bytesPer2DImage,texture));

Separate arguments with a space (clang-format will fix this for you during upstreaming, anyway).

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:156
> +        index.getType() != gl::TextureType::_2DMultisampleArray && forceGPUInitialization == false)

&& !forceGPUInitialization
Comment 12 Kyle Piddington 2021-04-29 11:52:49 PDT
Created attachment 427363 [details]
Patch
Comment 13 Kyle Piddington 2021-04-29 11:53:31 PDT
Comment on attachment 426301 [details]
Patch

This one should be dropped.
Comment 14 Kyle Piddington 2021-04-29 12:11:24 PDT
Created attachment 427366 [details]
Patch
Comment 15 Kyle Piddington 2021-04-29 12:33:20 PDT
Created attachment 427369 [details]
Patch
Comment 16 Kenneth Russell 2021-04-29 12:46:44 PDT
Comment on attachment 427369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427369&action=review

Still looks good; two small suggestions.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:325
> +angle::Result CopyCompressedTextureContentsToStagingBuffer(

Same comment as before about considering unifying these entry points with a bool argument.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:502
> +    //Copy to staging buffer before uploading to texture.

Space after "//"; also suggest putting these comments inside the if/else blocks to make them easier to read.
Comment 17 EWS 2021-04-29 16:57:35 PDT
kpiddington@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 427369 [details] from commit queue.
Comment 18 Kenneth Russell 2021-04-29 17:16:36 PDT
Comment on attachment 427369 [details]
Patch

CQ'ing for you Kyle.
Comment 19 EWS 2021-04-29 17:22:22 PDT
Committed r276825 (237180@main): <https://commits.webkit.org/237180@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427369 [details].