Bug 236427 – Cocoa2d-HTLM5 game appears to use wrong vertex buffer data

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kyle Piddington]

Cocoa2d-HTLM5 game apepars to use wrong vertex buffer data

[Kyle Piddington]

Created attachment 451486 [details]
Patch

[Kyle Piddington]

<rdar://problem/87136345>

[EWS Watchlist]

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

[Kimmo Kinnunen]

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.

[Kyle Piddington]

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

[Kenneth Russell]

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.

[Kyle Piddington]

Created attachment 451719 [details]
Patch for landing

[Kyle Piddington]

Created attachment 451726 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/ThirdParty/ANGLE/ChangeLog contains OOPS!.

[Kyle Piddington]

Created attachment 451732 [details]
Patch for landing

[EWS]

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