Issue 584807 in chromium: Support OES_depth24 extension for WebGL's back buffer if available

[chro...@googlecode.com]

Status: Assigned
Owner: k...@chromium.org
CC: mar...@chromium.org
Labels: Type-Bug Pri-2 Cr-Blink-WebGL OS-All

New issue 584807 by k...@chromium.org: Support OES_depth24 extension for
WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

If the OES_depth24 extension is available, and the user requests a depth
buffer but no stencil buffer, the WebGL implementation should prefer a
DEPTH_COMPONENT24 depth buffer rather than use a packed depth/stencil
buffer.

Currently the code prefers to allocate a stencil buffer behind the scenes,
and on some implementations, this is causing performance problems because
the stencil buffer is present, but the glClear calls are only receiving
COLOR_BUFFER_BIT | DEPTH_BUFFER_BIT. The lack of STENCIL_BUFFER_BIT is
causing expensive readbacks of the stencil buffer's contents in order to
preserve them.


--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[chro...@googlecode.com]

Issue 584807: Support OES_depth24 extension for WebGL's back buffer if
available
https://code.google.com/p/chromium/issues/detail?id=584807

This issue is now blocking issue chrome-os-partner:46898.
See https://code.google.com/p/chrome-os-partner/issues/detail?id=46898

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings

[chro...@googlecode.com]

Comment #2 on issue 584807 by mar...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Could we simply not attach the stencil buffer instead?

[chro...@googlecode.com]

Comment #3 on issue 584807 by k...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Maybe, but I suspect that attaching a GL_DEPTH24_STENCIL8_OES renderbuffer
only to the GL_DEPTH_ATTACHMENT point would go down strange code paths in
most drivers. I'd rather change the code to explicitly allocate a
GL_DEPTH_COMPONENT24 renderbuffer.

[chro...@googlecode.com]

Comment #4 on issue 584807 by bugd...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available

https://code.google.com/p/chromium/issues/detail?id=584807#c4

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/2374479265b26ba0a4a658582b068a4526220774

commit 2374479265b26ba0a4a658582b068a4526220774
Author: kbr <k...@chromium.org>
Date: Wed Feb 10 01:11:38 2016

Prefer to use the OES_depth24 extension for WebGL's depth-only back buffers.

Instead of using the OES_packed_depth_stencil extension to obtain 24-bit
depth buffers, prefer the OES_depth24 extension. This avoids a situation
where the depth buffer is cleared but the stencil buffer must be
preserved, leading to performance problems on some GPUs.

BUG=584807

Review URL: https://codereview.chromium.org/1675973002

Cr-Commit-Position: refs/heads/master@{#374562}

[modify]
http://crrev.com/2374479265b26ba0a4a658582b068a4526220774/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
[modify]
http://crrev.com/2374479265b26ba0a4a658582b068a4526220774/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
[modify]
http://crrev.com/2374479265b26ba0a4a658582b068a4526220774/third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp

[chro...@googlecode.com]

Updates:
Status: Fixed

Comment #5 on issue 584807 by k...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Marking fixed. Need help from teams that have the affected hardware to
verify that the performance problem is solved.

[chro...@googlecode.com]

Comment #6 on issue 584807 by mar...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Thanks Ken. I haven't gotten my hands on a device yet, and people with
devices are in Taipei (so on new years break). I'll see if someone
somewhere can verify.

[chro...@googlecode.com]

Updates:
Status: Verified
Cc: oak-...@chromium.org oak-...@chromium.org

Comment #7 on issue 584807 by djku...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Thanks Ken,
I manually verified that this patch does indeed improve WebGL performance
dramatically.

Current chromeos-chrome is 50.0.2639.0_rc-r1, we expect to pick up this
change once chromeos-chrome rolls past 50.0.2647.0.

[chro...@googlecode.com]

Comment #8 on issue 584807 by bugd...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available

https://code.google.com/p/chromium/issues/detail?id=584807#c8

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/484475eb5757acd278b7ca323d2b2a2f074d196e

commit 484475eb5757acd278b7ca323d2b2a2f074d196e
Author: phoglund <phog...@chromium.org>
Date: Tue Feb 16 12:10:23 2016

Revert 1675973002 and 1683213002.

Reason for revert:
Speculative revert: appears to break printing/webgl-oversized-printing.html
on Linux MSAN:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/builds/8511

and possibly a bunch of other webkit tests.

Revert "Prefer to use the OES_depth24 extension for WebGL's depth-only back
buffers."

This reverts commit 2374479265b26ba0a4a658582b068a4526220774.

Revert "Clean up drawing buffer code."

This reverts commit d89c4e01716053d7bd4642f42c8d4a1ecefcaf92.

BUG=584807
TBR=k...@chromium.org

Review URL: https://codereview.chromium.org/1698303002

Cr-Commit-Position: refs/heads/master@{#375550}

[modify]
http://crrev.com/484475eb5757acd278b7ca323d2b2a2f074d196e/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
[modify]
http://crrev.com/484475eb5757acd278b7ca323d2b2a2f074d196e/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
[modify]
http://crrev.com/484475eb5757acd278b7ca323d2b2a2f074d196e/third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp

[chro...@googlecode.com]

Updates:
Status: Assigned

Comment #9 on issue 584807 by phog...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

I reverted your changes because they appeared to make
printing/webgl-oversized-printing.html red on Linux MSAN. The revert made
the bot green again, so now at least we know that. Feel free to unrevert
when you have taken appropriate action.

[chro...@googlecode.com]

Comment #10 on issue 584807 by sulli...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Re #9: it looks like these changes also broke several tests on the android
perfbots:
smoothness.key_silk_cases
v8.top_25_smooth
blink_style.key_mobile_sites
smoothness.scrolling_tough_ad_cases
power.typical_10_mobile
thread_times.key_mobile_sites_smooth

They are all green with the revert as well.

[chro...@googlecode.com]

Updates:
Status: Started

Comment #11 on issue 584807 by k...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Are there links to the failing perf tests and instructions on how to
reproduce the failures?

Unless those tests are using WebGL, I can't understand how
2374479265b26ba0a4a658582b068a4526220774 could have affected them.

[chro...@googlecode.com]

Updates:
Cc: sulli...@chromium.org

Comment #12 on issue 584807 by k...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

(No comment was entered for this change.)

[chro...@googlecode.com]

Comment #13 on issue 584807 by sulli...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

Sorry, here's a link to a failing build:
https://build.chromium.org/p/chromium.perf/builders/Android%20Galaxy%20S5%20Perf%20%281%29/builds/1539

You can see these cycle green in phoglund's revert in build 1540. He only
linked this bug on the revert, but it looks like crrev.com/1683213002 was
also reverted in the same patch, that could have been the culprit.

This failed on Android platforms including Galaxy S5, Nexus 5
(https://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5%20Perf%20%281%29/builds/1888),
Nexus 6
(https://build.chromium.org/p/chromium.perf/builders/Android%20Nexus6%20Perf%20%281%29/builds/1542),
Nexus 7v2, and Nexus 9.

You can run locally with tools/perf/run_benchmark smoothness.key_silk_cases
--browser=<the browser you built for android> or run on the perf trybots:
https://www.chromium.org/developers/telemetry/performance-try-bots

[chro...@googlecode.com]

Comment #14 on issue 584807 by k...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

The change in 2374479265b26ba0a4a658582b068a4526220774 definitely causes a
huge increase in the run time of the printing/webgl-oversized-printing.html
layout test on Linux. My best hypothesis at this point is that the old
version of Mesa that we use for testing purposes has poor support for
DEPTH_COMPONENT24 depth buffers.

I still have no idea why the various failures of the perf tests happened in
response to this change; the few I sampled don't seem to be using WebGL at
all, so the changes to DrawingBuffer.[ch]pp should have been no-ops. Still
investigating.

[chro...@googlecode.com]

Comment #15 on issue 584807 by mar...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

@14: But OES_depth24 is only exposed if you build mesa for GLES, and for
chrome we build mesa for big GL, right?

[chro...@googlecode.com]

Updates:
Cc: -oak-...@chromium.org

Comment #16 on issue 584807 by mar...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

removing what seems like an invalid CC:

[chro...@googlecode.com]

Updates:
Cc: -oak-...@chromium.org

Comment #17 on issue 584807 by mar...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

hmm I guess they're private groups. Either way, that causes email spam.

[chro...@googlecode.com]

Comment #18 on issue 584807 by k...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

marcheu@: you're right that we build Mesa for GL and not GLES, but the
question is whether Chromium's command buffer claims support for the
OES_depth24 extension, which it does when running on desktop GL.

[chro...@googlecode.com]

Comment #19 on issue 584807 by mar...@chromium.org: Support OES_depth24

extension for WebGL's back buffer if available
https://code.google.com/p/chromium/issues/detail?id=584807

So what does the command buffer use to emulate the OES_depth24 extension on
big GL? If I know, I can look at the mesa code to see if there's a good
reason for the slowdown.