New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 792966 link

Starred by 4 users

WebGL Uniform Buffer Objects not updated correctly in ANGLE

Reported by david.ca...@gmail.com, Dec 7 2017

Issue description

UserAgent: 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:
 
Cc: kbr@chromium.org gov...@chromium.org jmad...@chromium.org
Components: -Blink>WebGL Internals>GPU>ANGLE
Labels: ReleaseBlock-Stable M-64 M-65 M-63
Status: Available (was: Unconfirmed)
Able to reproduce the issue and as mentioned by bug reporter this is regression which started in M63, Please find the bisect result below :

Bisect Result :
You are probably looking for a change made after 501746 (known good), but no later than 501747 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
  https://chromium.googlesource.com/chromium/src/+log/38712899213441d2330deebef677e56980d18b15..11b54cadcd04ea3b5650839abff6a6060d63e978


GPU details attached. 


Note : This is also reproducible on latest Chrome Dev(64.0.3278.0) and Canary(65.0.3287.0) as well.
gpu.htm
110 KB View Download
This blocking further M63 stable roll out. Please take a look ASAP and let us know if it is not a blocker. Thank you.

Comment 3 by kbr@chromium.org, Dec 7 2017

Cc: geoffl...@chromium.org cwallez@chromium.org
Labels: -ReleaseBlock-Stable
I'm sorry, but whatever this is it doesn't block M63 stable.

ANGLE folks, could you please bisect this further?
Owner: jmad...@chromium.org
Status: Assigned (was: Available)
Will have to see if this affects WebGL 1 or is WebGL 2 specific. Likely because of a dirty bits change.
Only affects webgl2 from my testing
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.
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.

Comment 8 by kbr@chromium.org, 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.

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 :))

Comment 10 by kbr@chromium.org, Dec 8 2017

I'm sorry, David, it's highly unlikely we can ship a browser update to Chrome Stable that quickly.

Comment 11 by kbr@chromium.org, Dec 8 2017

Blocking: 793307

Comment 12 by kbr@chromium.org, Dec 8 2017

Components: Blink>WebGL
Labels: -Pri-2 Pri-1
Upgrading to P1. There's been another report in  Issue 793307  which is probably the same issue.

Comment 13 by kbr@chromium.org, Dec 8 2017

Owner: kainino@chromium.org
Kai said he'd own these at least for the next couple of days.

Cc: pbomm...@chromium.org
What could be the ETA? I can defer the release a bit. I do not want to sniff Chrome version to turn off WebGL2

Comment 16 by kbr@chromium.org, 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.

Comment 17 by kbr@chromium.org, Dec 8 2017

Re: #15: we will know more once we know what the fix is. Probably a couple of weeks.

Labels: ReleaseBlock-Stable
Adding "RBS" for tracking purpose.
Re: #16 I only tested on windows
Re: #17. Ok I will sadly turn off webgl2 in babylonjs 3.1 for m63 :(
Just udpated the test case to align UBO on 4
Cc: kainino@chromium.org jdarpinian@chromium.org
 Issue 793307  has been merged into this issue.

Comment 22 by zmo@chromium.org, 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.

Comment 23 by zmo@chromium.org, Dec 8 2017

#20 probably will make it work all around.
Summary: WebGL Uniform Buffer Objects not updated correctly in ANGLE (was: WebGL inconsistency with others browsers)
Pretty confident these bugs are the same. FYI, zmo added a test case here:
https://github.com/KhronosGroup/WebGL/pull/2559

Status: Started (was: Assigned)
Re: #22 This is not our production code. It is obvious we are not using plain constants. I did it quickly to help
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 .
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
Blocking: -793307
Also link the to the culprit CL from #6:
https://chromium-review.googlesource.com/c/angle/angle/+/659228
Blocking: angleproject:1390
Labels: Merge-Request-64 Merge-Request-63
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?
Project Member

Comment 34 by sheriffbot@chromium.org, Dec 8 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
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
Project Member

Comment 35 by bugdroid1@chromium.org, 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

Cc: abdulsyed@chromium.org
NextAction: 2017-12-11
Pls update the bug once change is well baked/verified in canary. Thank you.

Comment 38 by kbr@chromium.org, Dec 9 2017

Blocking: 793102
Project Member

Comment 39 by sheriffbot@chromium.org, Dec 9 2017

Labels: -Merge-Request-64 Hotlist-Merge-Approved Merge-Approved-64
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
The NextAction date has arrived: 2017-12-11
How is the change looking in canary?
 Issue 793102  has been merged into this issue.
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.
Also, I've verified that it's fixed in the Canary release (3291).
Project Member

Comment 45 by bugdroid1@chromium.org, Dec 11 2017

Labels: -merge-approved-64 merge-merged-3282
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

Is  bug 794035  related to this?
No, totally unrelated. This fix is Windows-only, the other is Mac-only AFAIK.
Yeah, sorry missed that  bug 794035  is for Mac.
Status: Fixed (was: Started)
This is fixed - going to open a new bug for re-enabling the dirty bit to improve performance.
Verified on Beta 64.0.3282.25.

Canary channel looking good, no new crashes or reports.
Labels: -Merge-Review-63 Merge-Approved-63
Approving merge to M63 branch 3239 based on comment #50. Please merge ASAP. Thank you.
Project Member

Comment 52 by bugdroid1@chromium.org, Dec 12 2017

Labels: -merge-approved-63 merge-merged-3239
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

Labels: TE-Verified-M63 TE-Verified-63.0.3239.108
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
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

Comment 55 by zmo@chromium.org, Dec 19 2017

Cc: zmo@chromium.org
 Issue 796180  has been merged into this issue.

Sign in to add a comment