[webkit-reviews] review granted: [Bug 222685] Metal-ANGLE: Shared memory texture tests failing in iOS Simulator : [Attachment 427282] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kyle Piddington
<kpiddington at apple.com>'s request for review:
Bug 222685: Metal-ANGLE: Shared memory texture tests failing in iOS Simulator
https://bugs.webkit.org/show_bug.cgi?id=222685

Attachment 427282: Patch

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




--- Comment #11 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 427282
  --> https://bugs.webkit.org/attachment.cgi?id=427282
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