Bug 222685 – Metal-ANGLE: Shared memory texture tests failing in iOS Simulator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Dean Jackson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/75001722>

[Kyle Piddington]

Created attachment 426297 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Dean Jackson]

Created attachment 426301 [details]
Patch

[EWS]

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].

[Truitt Savell]

Reverted r276190 for reason:

broke a test internally.

Committed r276507 (236964@main): <https://commits.webkit.org/236964@main>

[Kyle Piddington]

Created attachment 427205 [details]
Patch

[Kyle Piddington]

Created attachment 427224 [details]
Patch

[Kyle Piddington]

Created attachment 427281 [details]
Patch

[Kyle Piddington]

Created attachment 427282 [details]
Patch

[Kenneth Russell]

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

[Kyle Piddington]

Created attachment 427363 [details]
Patch

[Kyle Piddington]

Comment on attachment 426301 [details]
Patch

This one should be dropped.

[Kyle Piddington]

Created attachment 427366 [details]
Patch

[Kyle Piddington]

Created attachment 427369 [details]
Patch

[Kenneth Russell]

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.

[EWS]

[e-mail] 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.

[Kenneth Russell]

Comment on attachment 427369 [details]
Patch

CQ'ing for you Kyle.

[EWS]

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].