Bug 227452 - ANGLE Metal primitive restart range computation should not be done unless primitive restart is enabled
Summary: ANGLE Metal primitive restart range computation should not be done unless pri...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-28 08:23 PDT by Kimmo Kinnunen
Modified: 2021-06-29 06:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.12 KB, patch)
2021-06-28 08:41 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (8.35 KB, patch)
2021-06-29 01:26 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (8.35 KB, patch)
2021-06-29 02:05 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-06-28 08:23:34 PDT
ANGLE Metal primitive restart range computation should not be done unless primitive restart is enabled
Comment 1 Kimmo Kinnunen 2021-06-28 08:41:56 PDT
Created attachment 432397 [details]
Patch
Comment 2 EWS Watchlist 2021-06-28 08:43:06 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Kenneth Russell 2021-06-28 15:10:19 PDT
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?
Comment 4 Kimmo Kinnunen 2021-06-29 01:26:31 PDT
Created attachment 432458 [details]
Patch for landing
Comment 5 Kimmo Kinnunen 2021-06-29 01:27:31 PDT
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.
Comment 6 Kimmo Kinnunen 2021-06-29 02:05:58 PDT
Created attachment 432461 [details]
Patch for landing
Comment 7 EWS 2021-06-29 06:52:23 PDT
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].
Comment 8 Radar WebKit Bug Importer 2021-06-29 06:53:17 PDT
<rdar://problem/79913261>