ANGLE Metal primitive restart range computation should not be done unless primitive restart is enabled
Created attachment 432397 [details] Patch
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment on attachment 432397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432397&action=review This is easier to read and understand. r+ with a couple of small questions. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:753 > + if (!isSimpleType || !glContext->getState().isPrimitiveRestartEnabled()) I think it would be worth keeping the comment above, perhaps revised. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:781 > + size_t currentIndexOffset = offset / indexTypeBytes; Is offset verified by this point to be divisible by indexTypeBytes? > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:787 > + int64_t nIndicesInSlice = ((int64_t)range.restartBegin - currentIndexOffset) - ((int64_t) range.restartBegin - currentIndexOffset) % nIndicesPerPrimitive; Here and below: do you want to upgrade these C-style casts to C++ style to make their intent clearer? Also: I never remember operator precedence; wonder whether a temporary variable or more parentheses would make this clearer. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:819 > + drawCommands.push_back({indicesLeft, currentIndexOffset * indexTypeBytes}); Just checking - it's guaranteed that this last if-test will always pick up any trailing draw commands from the loop above?
Created attachment 432458 [details] Patch for landing
Comment on attachment 432397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432397&action=review >> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:781 >> + size_t currentIndexOffset = offset / indexTypeBytes; > > Is offset verified by this point to be divisible by indexTypeBytes? Yeah, it's a GL error otherwise. >> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:787 >> + int64_t nIndicesInSlice = ((int64_t)range.restartBegin - currentIndexOffset) - ((int64_t) range.restartBegin - currentIndexOffset) % nIndicesPerPrimitive; > > Here and below: do you want to upgrade these C-style casts to C++ style to make their intent clearer? > > Also: I never remember operator precedence; wonder whether a temporary variable or more parentheses would make this clearer. Yes, but not in this patch. I don't yet understand the point of the casts. If they end up being removed, it's easier to review in patch targeting that. >> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:819 >> + drawCommands.push_back({indicesLeft, currentIndexOffset * indexTypeBytes}); > > Just checking - it's guaranteed that this last if-test will always pick up any trailing draw commands from the loop above? Yes, as far as I understand. Also, if there would be a bug, maybe it'd be simpler to fix in some other patch.
Created attachment 432461 [details] Patch for landing
Committed r279372 (239238@main): <https://commits.webkit.org/239238@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432461 [details].
<rdar://problem/79913261>