Bug 236427 - Cocoa2d-HTLM5 game appears to use wrong vertex buffer data
Summary: Cocoa2d-HTLM5 game appears to use wrong vertex buffer data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kyle Piddington
URL:
Keywords: InRadar
Depends on: 220896
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-09 19:44 PST by Kyle Piddington
Modified: 2022-02-24 04:02 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.68 KB, patch)
2022-02-09 19:46 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch for landing (3.70 MB, patch)
2022-02-11 10:11 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch for landing (4.68 KB, patch)
2022-02-11 10:49 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch for landing (4.68 KB, patch)
2022-02-11 11:45 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle Piddington 2022-02-09 19:44:48 PST
Cocoa2d-HTLM5 game apepars to use wrong vertex buffer data
Comment 1 Kyle Piddington 2022-02-09 19:46:59 PST
Created attachment 451486 [details]
Patch
Comment 2 Kyle Piddington 2022-02-09 19:47:02 PST
<rdar://problem/87136345>
Comment 3 EWS Watchlist 2022-02-09 19:48:57 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Kimmo Kinnunen 2022-02-10 01:45:53 PST
Looks good as an interim solution with an addition:
Please add a // FIXME: explaining why consulting mElementArrayDirty is necessary when it should not be necessary.

Since you have the repro:
Could we add an assert at the end of ContextMtl::setupDraw that mRenderEncoder state matches the expected ProgramMtl state? So recursively in in ProgramMtl assertion that VertexArrayMtl state? And in VertexArrayMtl, assertion that  mCurrentArrayBuffers, offsets etc are in RenderCommandEncoder state cache correctly?

With the assumption that RenderCommandEncoder state is really diverging from expected state (your explanation in the commit message):
I'd imagine this assertion to fail with the test case for the first draw call that overrides the expected state, e.g. to reveal the real bug? 

With the assumption that RenderCommandEncoder reflects the expected state correctly but that the dirty bits that sync the expected state are wrong:
I'd imagine this assertion to fail with the test case for the first draw call that expects the expected state and does not set anything, but then the expected state was not updated because some omission with the dirty bit handling.
Comment 5 Kyle Piddington 2022-02-10 09:35:21 PST
(In reply to Kimmo Kinnunen from comment #4)
> Looks good as an interim solution with an addition:
> Please add a // FIXME: explaining why consulting mElementArrayDirty is
> necessary when it should not be necessary.
> 
> Since you have the repro:
> Could we add an assert at the end of ContextMtl::setupDraw that
> mRenderEncoder state matches the expected ProgramMtl state? So recursively
> in in ProgramMtl assertion that VertexArrayMtl state? And in VertexArrayMtl,
> assertion that  mCurrentArrayBuffers, offsets etc are in
> RenderCommandEncoder state cache correctly?
I tried just this, though 'assert' might have been incorrect. Unfortunately, this is an odd case where I suspect the vertex data and the program data match, but we're getting stale vertex data. I went through the vertex attribute handling flow 3 or 4 times yesterday, but for the life of me can't figure out where we've diverged. Only some elements are using the wrong vertex data, not all.
> 
> With the assumption that RenderCommandEncoder state is really diverging from
> expected state (your explanation in the commit message):
> I'd imagine this assertion to fail with the test case for the first draw
> call that overrides the expected state, e.g. to reveal the real bug? 

I'll certainly continue to investigate, once we confirm this patches up our repros. 

> With the assumption that RenderCommandEncoder reflects the expected state
> correctly but that the dirty bits that sync the expected state are wrong:
> I'd imagine this assertion to fail with the test case for the first draw
> call that expects the expected state and does not set anything, but then the
> expected state was not updated because some omission with the dirty bit
> handling.
Comment 6 Kenneth Russell 2022-02-10 19:42:09 PST
For completeness: in a conference call today Geoff Lang from the ANGLE team suggested that the bug might be that when the element array changes, any vertex attributes that previously required format conversion need to be re-converted.

Perhaps this hint could help reduce the amount of work that needs to be done when the element array changes, though this can be revisited later if that's too complicated or fragile a change.
Comment 7 Kyle Piddington 2022-02-11 10:11:57 PST
Created attachment 451719 [details]
Patch for landing
Comment 8 Kyle Piddington 2022-02-11 10:49:09 PST
Created attachment 451726 [details]
Patch for landing
Comment 9 EWS 2022-02-11 11:39:36 PST
ChangeLog entry in Source/ThirdParty/ANGLE/ChangeLog contains OOPS!.
Comment 10 Kyle Piddington 2022-02-11 11:45:45 PST
Created attachment 451732 [details]
Patch for landing
Comment 11 EWS 2022-02-11 13:01:59 PST
Committed r289664 (247152@main): <https://commits.webkit.org/247152@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451732 [details].