Bug 227452 – ANGLE Metal primitive restart range computation should not be done unless primitive restart is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Kimmo Kinnunen]

ANGLE Metal primitive restart range computation should not be done unless primitive restart is enabled

[Kimmo Kinnunen]

Created attachment 432397 [details]
Patch

[EWS Watchlist]

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

[Kenneth Russell]

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?

[Kimmo Kinnunen]

Created attachment 432458 [details]
Patch for landing

[Kimmo Kinnunen]

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.

[Kimmo Kinnunen]

Created attachment 432461 [details]
Patch for landing

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/79913261>