Issue metadata
Sign in to add a comment
|
WebGL Uniform Buffer Objects not updated correctly in ANGLE
Reported by
david.ca...@gmail.com,
Dec 7 2017
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36 Steps to reproduce the problem: 1. go to https://www.babylonjs-playground.com/#RA9C0A 2. Check plane color 3. Expected color is gray 4. Current color is yellow What is the expected behavior? Plane color should be gray What went wrong? Moving mouse seems to trigger the right result Did this work before? Yes 62 Does this work in other browsers? Yes Chrome version: 63.0.3239.84 Channel: stable OS Version: 10.0 Flash Version:
,
Dec 7 2017
This blocking further M63 stable roll out. Please take a look ASAP and let us know if it is not a blocker. Thank you.
,
Dec 7 2017
I'm sorry, but whatever this is it doesn't block M63 stable. ANGLE folks, could you please bisect this further?
,
Dec 7 2017
Will have to see if this affects WebGL 1 or is WebGL 2 specific. Likely because of a dirty bits change.
,
Dec 7 2017
Only affects webgl2 from my testing
,
Dec 7 2017
Offending CL: StateManager11: Add internal dirty bits for uniforms. This eliminates some of the redundant work we do in uniform state updates. Driver uniforms and constant buffers are no longer synched with every draw call, but only when the StateManager11 thinks they might be dirty. Should improve overall draw call throughput. BUG=angleproject:1155 BUG=angleproject:1390 Unfortunately I'll be out Friday and Monday. If someone wants to take this before then, feel free. Otherwise I'll take it up Tuesday. It would be useful still to find a more tightly contained repro case.
,
Dec 7 2017
Also we could disable the dirty bits to make a quick fix - it should be as simple as flagging them always as dirty until we fix the more optimal path.
,
Dec 8 2017
Let's try to boil down the test case to something smaller and see whether the correct fix is merge-able back to M63. I think we wouldn't want to disable dirty bits on top-of-tree (in order to merge back to M63) without knowing exactly what's going on. Submitter, any help you can offer in producing a more reduced test case will help.
,
Dec 8 2017
Here is the simpler possible repro: https://www.babylonjs.com/bugs/default.html By the way do you think this could be hot fixed before Wednesday? We are shipping babylon.js 3.1 this wed and 40% of our content is broken due to this bug (yes I know I should have tested on dev version earlier and I promise to do it now :))
,
Dec 8 2017
I'm sorry, David, it's highly unlikely we can ship a browser update to Chrome Stable that quickly.
,
Dec 8 2017
,
Dec 8 2017
Upgrading to P1. There's been another report in Issue 793307 which is probably the same issue.
,
Dec 8 2017
Kai said he'd own these at least for the next couple of days.
,
Dec 8 2017
,
Dec 8 2017
What could be the ETA? I can defer the release a bit. I do not want to sniff Chrome version to turn off WebGL2
,
Dec 8 2017
David, your latest test case https://www.babylonjs.com/bugs/default.html fails on macOS with this error: [.Offscreen-For-WebGL-0x7fcdfa022400]GL ERROR :GL_INVALID_OPERATION : glDrawArrays: uniform buffers : buffer or buffer range not large enough at index 0 Are you respecting the UNIFORM_BLOCK_DATA_SIZE minimum size requirement? It doesn't look like it in your test case.
,
Dec 8 2017
Re: #15: we will know more once we know what the fix is. Probably a couple of weeks.
,
Dec 8 2017
Adding "RBS" for tracking purpose.
,
Dec 8 2017
Re: #16 I only tested on windows Re: #17. Ok I will sadly turn off webgl2 in babylonjs 3.1 for m63 :(
,
Dec 8 2017
Just udpated the test case to align UBO on 4
,
Dec 8 2017
,
Dec 8 2017
buffer data packing is platform/driver dependent, so you should not rely on a specific machine working as the OK sign. Instead, the data store needs to be big enough according to std140 rules.
,
Dec 8 2017
#20 probably will make it work all around.
,
Dec 8 2017
,
Dec 8 2017
Pretty confident these bugs are the same. FYI, zmo added a test case here: https://github.com/KhronosGroup/WebGL/pull/2559
,
Dec 8 2017
,
Dec 8 2017
Re: #22 This is not our production code. It is obvious we are not using plain constants. I did it quickly to help
,
Dec 8 2017
Don't worry about your reduced test case for now. I will see if the original test case works after fixing Mo's CTS case which was based on the repro in issue 793307 .
,
Dec 8 2017
Copying from other bug: Here's the log for that range: https://crrev.com/501743..501756 which includes this ANGLE roll: https://chromium.googlesource.com/angle/angle.git/+log/9df395c..715e7f1
,
Dec 8 2017
,
Dec 8 2017
Also link the to the culprit CL from #6: https://chromium-review.googlesource.com/c/angle/angle/+/659228
,
Dec 8 2017
,
Dec 8 2017
Verified this fixes both the CTS case added by zmo, and the original requester's test case (which has a new url): https://www.babylonjs-playground.com/indexstable#RA9C0A Requesting mergeback to M63 of the ANGLE change at https://chromium-review.googlesource.com/c/angle/angle/+/818178 This is a very small and simple change that disables a recently added optimization that broke a key feature in WebGL 2.0 and will affect a large fraction of WebGL 2.0 content in the wild. I believe I would just need to merge it to the "chromium/3239" branch *in the angle repository*, without pushing a DEPS change to the chromium release branch, is that correct?
,
Dec 8 2017
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/9b88991c7211fdc9144c793b66f429ee01fce032 commit 9b88991c7211fdc9144c793b66f429ee01fce032 Author: Kai Ninomiya <kainino@chromium.org> Date: Sat Dec 09 00:37:33 2017 Disable DIRTY_BIT_PROGRAM_UNIFORM_BUFFERS This dirty bit isn't getting set somewhere where it should be, so work around this by just enabling the dirty bit always. The bit was added here: https://chromium-review.googlesource.com/c/angle/angle/+/659228 There will be a followup to re-enable this dirty bit once the correctness has been fixed. Test: https://github.com/KhronosGroup/WebGL/pull/2559 Bug: chromium:792966 Change-Id: I473a60b39eff70e59ab55ff7b74f06fdb0db1305 Reviewed-on: https://chromium-review.googlesource.com/818178 Commit-Queue: Kai Ninomiya <kainino@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> [modify] https://crrev.com/9b88991c7211fdc9144c793b66f429ee01fce032/src/libANGLE/renderer/d3d/d3d11/StateManager11.cpp
,
Dec 9 2017
,
Dec 9 2017
Pls update the bug once change is well baked/verified in canary. Thank you.
,
Dec 9 2017
,
Dec 9 2017
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 11 2017
The NextAction date has arrived: 2017-12-11
,
Dec 11 2017
How is the change looking in canary?
,
Dec 11 2017
Issue 793102 has been merged into this issue.
,
Dec 11 2017
This commit https://chromiumdash-staging.googleplex.com/commit/18c48eaff0d955525c03160c510fc909bda0d5a6 seems to have only just made it into canary (3291) late last night. However checking the crash/ reports for the new canaries, I do not see any issues in related code, and we haven't gotten any new reports.
,
Dec 11 2017
Also, I've verified that it's fixed in the Canary release (3291).
,
Dec 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/c13c3dde6d24810805078b60de964146703a981f commit c13c3dde6d24810805078b60de964146703a981f Author: Kai Ninomiya <kainino@chromium.org> Date: Mon Dec 11 22:19:49 2017 Disable DIRTY_BIT_PROGRAM_UNIFORM_BUFFERS This dirty bit isn't getting set somewhere where it should be, so work around this by just enabling the dirty bit always. The bit was added here: https://chromium-review.googlesource.com/c/angle/angle/+/659228 There will be a followup to re-enable this dirty bit once the correctness has been fixed. TBR=kbr@chromium.org Test: https://github.com/KhronosGroup/WebGL/pull/2559 Bug: chromium:792966 Change-Id: I473a60b39eff70e59ab55ff7b74f06fdb0db1305 Reviewed-on: https://chromium-review.googlesource.com/818178 Commit-Queue: Kai Ninomiya <kainino@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> (cherry picked from commit 9b88991c7211fdc9144c793b66f429ee01fce032) Reviewed-on: https://chromium-review.googlesource.com/820750 Reviewed-by: Kai Ninomiya <kainino@chromium.org> [modify] https://crrev.com/c13c3dde6d24810805078b60de964146703a981f/src/libANGLE/renderer/d3d/d3d11/StateManager11.cpp
,
Dec 12 2017
Is bug 794035 related to this?
,
Dec 12 2017
No, totally unrelated. This fix is Windows-only, the other is Mac-only AFAIK.
,
Dec 12 2017
Yeah, sorry missed that bug 794035 is for Mac.
,
Dec 12 2017
This is fixed - going to open a new bug for re-enabling the dirty bit to improve performance.
,
Dec 12 2017
Verified on Beta 64.0.3282.25. Canary channel looking good, no new crashes or reports.
,
Dec 12 2017
Approving merge to M63 branch 3239 based on comment #50. Please merge ASAP. Thank you.
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/2ff870db3a3ba33c6dbf4b88c9ec2e52127b7206 commit 2ff870db3a3ba33c6dbf4b88c9ec2e52127b7206 Author: Kai Ninomiya <kainino@chromium.org> Date: Tue Dec 12 22:50:09 2017 Disable DIRTY_BIT_PROGRAM_UNIFORM_BUFFERS This dirty bit isn't getting set somewhere where it should be, so work around this by just enabling the dirty bit always. The bit was added here: https://chromium-review.googlesource.com/c/angle/angle/+/659228 There will be a followup to re-enable this dirty bit once the correctness has been fixed. TBR=kbr@chromium.org Test: https://github.com/KhronosGroup/WebGL/pull/2559 Bug: chromium:792966 Change-Id: I473a60b39eff70e59ab55ff7b74f06fdb0db1305 Reviewed-on: https://chromium-review.googlesource.com/818178 Commit-Queue: Kai Ninomiya <kainino@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> (cherry picked from commit 9b88991c7211fdc9144c793b66f429ee01fce032) Reviewed-on: https://chromium-review.googlesource.com/822454 Reviewed-by: Kai Ninomiya <kainino@chromium.org> [modify] https://crrev.com/2ff870db3a3ba33c6dbf4b88c9ec2e52127b7206/src/libANGLE/renderer/d3d/d3d11/StateManager11.cpp
,
Dec 14 2017
Rechecked this issue on Windows 10 using chrome version 63.0.3239.108. Fix is working as intended. Plane color is displayed as gray. Tagging issue with TE-verified labels for M63
,
Dec 14 2017
david.catuhe: FYI, this fix should be rolling out now in 63.0.3239.108. https://chromereleases.googleblog.com/2017/12/stable-channel-update-for-desktop_14.html
,
Dec 19 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by pbomm...@chromium.org
, Dec 7 2017Components: -Blink>WebGL Internals>GPU>ANGLE
Labels: ReleaseBlock-Stable M-64 M-65 M-63
Status: Available (was: Unconfirmed)
110 KB
110 KB View Download