Bug 229941

Created attachment 437827 [details] Patch

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

Comment on attachment 437827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437827&action=review > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.h:47 > +angle::Result InitializeTextureContentsGPU(const gl::Context *context, This prototype is unused? > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:116 > + size_t textureHeight = texture.get()->width(index.getNativeLevel()); height, not width > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:121 > + switch(textureObjFormat.actualFormatId) I think this code is more robustly implemented in formatutils.cpp computeCompressedImageSize See vkhelpers.cpp for call sites of computeCompressedImageSize. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:420 > + if(mtl::Buffer::MakeBuffer(contextMtl, bufferSizeInBytes, nullptr, &bufferOut) != angle::Result::Continue) Style nit: Here the pattern: != angle::Resutl::Continue means you maybe are doing something against the spirit of the coding convention of the project. Typically: The call graphs start with functions that return `angle::Result`. So when call graph progresses deeper, all calls to such functions can use the macros, ANGLE_TRY When you enter function without such return value, then you mostly don't re-enter angle::Result functions anymore. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:435 > + mtl::BufferRef zeroBuffer = GetCompressedBufferForTextureWithFormat(context, texture, textureObjFormat, index, bytesPerRow, bytesPerImage); So here your code is create metal buffer() if (cpu texture) use the contents of the metal buffer as texture contents () else send the buffer to metal() In other words: it might not make sense to make a metal resource but use it to just obtain a zero-filled "malloc" buffer. Instead, you could do if (cpu texture) { context->getZeroFilledBuffer() use the zero filled cpu buffer for contents } else { create metal buffer() send the buffer to metal() } > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:442 > + if(texture->isCPUAccessible()) Nit: In this commit, there's a lot of if(condition) and: //My Comment I believe the correct formatting is if (condition) and // Comment. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:483 > + } Not saying this commit should necessarily solve all below, but maybe it would be possible to move to this direction: In the ideal world, my layman guess for the best algorithm for the whole function would be: 1) if texture should be cleared with a render pass, clear with render pass 2) else 2.1) compute the zero buffer size for the texture, regardless if it's compressed or uncompressed 2.2) if texture is cpu accessible, clear with replaceRegion 2.3) else clear with buffer blit 3) ASSERT that no texture is left uncleared For the condition "texture should be cleared with a render pass": I just don't understand the current if statements. Currently I understand: - compressed textures: no - due to some simulator limitations: yes If I understand correctly: - For all other formats, zero buffer means zero texture - All there is to do is to know the size of the texture data and you can replace the texture contents with either replaceRegion or blit - Textures that can get relatively big, it may be better to do a render pass clear or blit (?). Is this why multisample goes to render pass path? If this is the case, the heuristic should be about the size, not about which type. - We seem to assume replaceRegion is better than blit or render pass - Depth/Stencil might be special case if 24_8 is emulated with two textures (is it a problem in this level?) I tried to understand the tables in https://developer.apple.com/metal/Metal-Feature-Set-Tables.pdf If I undestand correctly: - normal color, multisample, multisample array, depth, stencil can cleared with a render pass - compressed color should be cleared with an upload If I see correctly, the TextureD3D::initializeContents works pretty much this way

Created attachment 437922 [details] Patch

Comment on attachment 437922 [details] Patch This is pleasingly small. While I'm double-checking the math, would you consider uploading another revision explicitly enabling the test: webgl/2.0.y/conformance/extensions/webgl-compressed-texture-s3tc-srgb.html in LayoutTests/TestExpectations ? Or if you've tested locally and would prefer to enable the test in a follow-on (to prevent this from being reverted if there's a failure on some unrelated platform), that's fine. Thanks.

Comment on attachment 437922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437922&action=review > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:106 > +bool GetCompressedBufferSizeForTextureWithFormat(const TextureRef &texture, Looking more closely, this seems to duplicate InternalFormat::computeCompressedImageSize in src/libANGLE/formatutils.cpp. Is it possible to just use that by calling gl::GetSizedInternalFormatInfo, passing in the sized internal format of the compressed texture?

(In reply to Kenneth Russell from comment #6) > Comment on attachment 437922 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=437922&action=review > > > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:106 > > +bool GetCompressedBufferSizeForTextureWithFormat(const TextureRef &texture, > > Looking more closely, this seems to duplicate > InternalFormat::computeCompressedImageSize in src/libANGLE/formatutils.cpp. > > Is it possible to just use that by calling gl::GetSizedInternalFormatInfo, > passing in the sized internal format of the compressed texture? We also need to calculate the row length. It's almost the same as computeCompressedIamgeSize. This just wraps calling two functions into one. If you'd like me to decouple it, I can do that too.

Comment on attachment 437922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437922&action=review >> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:106 >> +bool GetCompressedBufferSizeForTextureWithFormat(const TextureRef &texture, > > Looking more closely, this seems to duplicate InternalFormat::computeCompressedImageSize in src/libANGLE/formatutils.cpp. > > Is it possible to just use that by calling gl::GetSizedInternalFormatInfo, passing in the sized internal format of the compressed texture? Sorry, I was looking just at the outBytesPerRow computation and failed to see you were actually calling computeCompressedImageSize. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:121 > + ((size.width + textureObjFormat.actualInternalFormat().compressedBlockWidth - 1u) / textureObjFormat.actualInternalFormat().compressedBlockWidth) * textureObjFormat.actualAngleFormat().pixelBytes; More concretely, can this use InternalFormat::computeRowPitch instead? Does it produce the same answer? > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:132 > + assert(textureObjFormat.actualAngleFormat().isBlock); Please use ANGLE's ASSERT. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:142 > + if(texture->isCPUAccessible()) Both of these initialization paths only handle 2D compressed textures. The conformance tests aren't covering the cube map and 3D paths well (for those compressed texture formats which support 3D - mainly, the BC formats) but it would be safest to add code for those - I think applications are at least using cube map compressed textures in the wild. Would you be willing to do this? We can collaborate on expanding the conformance suite. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:185 > + // If dst format is depth or stencil, ignore. This path is handled seperatley. typo: separately

Created attachment 437932 [details] Patch

Comment on attachment 437932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437932&action=review > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:148 > + if(texture->isCPUAccessible()) In earlier version of this patch, had a concern that this isn't handling the possibility of cube map or 3D compressed textures. The conformance suite is lacking in these areas, but cube map compressed textures are probably being used in the wild. Can these paths be enhanced to handle at least that case, and explicitly bail out of the 3D case if we don't want to try supporting it now?

Comment on attachment 437932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437932&action=review > Source/ThirdParty/ANGLE/ChangeLog:6 > + Zero-initialize compressed textures explicitly, as they aren't implicitly initalized in Metal. To capture a comment on an earlier version of this patch: can this conformance test be explicitly enabled in LayoutTests/TestExpectations?

Created attachment 437933 [details] Patch

Created attachment 437935 [details] Patch

Comment on attachment 437935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437935&action=review Thanks for producing all of the revisions. Looks good to me. r+ > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:136 > + assert(textureObjFormat.actualAngleFormat().isBlock); Would prefer ANGLE's ASSERT. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:162 > + texture->replaceRegion(contextMtl, mtlTextureRegion, index.getNativeLevel(), layer, buffer.data(), bytesPerRow, 0); Re-reviewing this, it looks correct for cube map textures because the passed-in layer is the cube map face index from [0..5]. > LayoutTests/ChangeLog:9 > + * TestExpectations: Comment "Explicitly turn on conformance test until all of webgl/2.0.y is enabled".

Comment on attachment 437935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437935&action=review Great. I'll post you a clang-format command you could use > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:109 > + size_t & outBytesPerRow, space (I think this would be size_t *bytesPerRowOut) in most ANGLE code. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:111 > +{ space > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:115 > + if(!textureObjFormat.intendedInternalFormat().computeCompressedImageSize(size, &bufferSizeInBytes)) spaces "if (" > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:129 > +angle::Result InitializeCompressedTextureContents(const gl::Context *context, This is anonymous namespace or static function? (static based on this file?) > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:133 > + const uint layer, const uint -> "unsigned" (typically indented to the opening parenthesis ?) > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:147 > + }; extra ;

<rdar://problem/83045378>

Created attachment 438277 [details] Patch

Created attachment 438278 [details] Patch

Created attachment 438423 [details] Patch for landing

Committed r282627 (241784@main): <https://commits.webkit.org/241784@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438423 [details].