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

Issue 763695 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocking:
issue 660844



Sign in to add a comment

WebGL reading depth texture while it's bound to FBO but with depth writes disabled

Reported by esent...@gmail.com, Sep 10 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.79 Safari/537.36

Steps to reproduce the problem:
1. Open http://esenthel.com/download/Temp/ChromeWebGLDepthRWBug/Application.html
2. You will see that the background is black - FAIL
3. If you press "Control" on the Keyboard the background will turn blue - OK

What is the expected behavior?
background should always be blue

What went wrong?
The problem is that when rendering the blue background (sky), I have depth texture bound to FBO, which I use for depth testing.
I am reading from that depth texture in the shader.
Depth writes are disabled, so I'm not reading+writing at the same time, I am only reading (depth tests + reads in the shader).
This is completely legal on GLES3 (WebGL2), DesktopGL and DirectX 9/11.
However if you're actually using DX11 to support WebGL in Chrome, then you must make extra step to make it work:
In order to be able to have depth buffer bound as depth stencil, and read from it, you need to operate on a ID3D11DepthStencilView *view that was created with:

D3D11_DEPTH_STENCIL_VIEW_DESC dsvd;
..
dsvd.Flags=D3D11_DSV_READ_ONLY_DEPTH;
if(depth_has_stencil)dsvd.Flags|=D3D11_DSV_READ_ONLY_STENCIL;

When you have this view, you can bind it as current DepthStencil RT and read from it in the shader.

In my game engine I always create 2 ID3D11DepthStencilView views;
struct Image
{
  ID3D11DepthStencilView *view, *read_only_view; // 'view' is the normal view, and 'read_only_view' with read-only flags
};
And then I use those views depending if I need writes or just reads.
My DirectX 11 implementation works for this demo, and so does my GLES3 implementation on Android and iOS, only on Chrome and FireFox it fails.

In the web demo, when Control is detected, then no depth buffer is bound to the FBO, and then it works, but it should work always.

Did this work before? N/A 

Does this work in other browsers? N/A

Chrome version: 61.0.3163.79  Channel: stable
OS Version: 10.0
Flash Version:
 
Labels: Needs-Triage-M61
Cc: rbasuvula@chromium.org
Labels: hasbisect-per-revision M-63
Owner: kainino@chromium.org
Status: Assigned (was: Unconfirmed)
Tested in chrome Stable #61.0.3163.79 and canary #63.0.3212.0 on Win 10.0 & 7 able to reproduce the issue.
Below are the Bisect Details:

Bisect Info:
=============
Good Build: 56.0.2909.0(Revision - 429737)
Bad Build: 56.0.2910.0(Revision - 430103)

You are probably looking for a change made after 429884 (known good), but no later than 429885 (first known bad).

CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/09f68d2c222f99f56df5f578f4d3dd5387854b32..61a4859d1d5a2f5082d486843c58a5b3f8795144

From the CL above, assigning the issue to the concern owner

@kainino: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url:https://codereview.chromium.org/2473933002

Note: Issue not reproduce on Mac 10.12.6 & Ubuntu 14.04.
Is that bisect correct? I suspect this never would have worked in D3D11. Doesn't seem like a regression issue.

Comment 4 by esent...@gmail.com, Sep 11 2017

In the meantime I've petitioned Microsoft to do something about the API design of depth stencil views in DX11, if you agree with me please vote on this idea:
https://wpdev.uservoice.com/forums/110705-universal-windows-platform/suggestions/31351033-depth-textures-d3d11-dsv-read-only-depth-api-desig
Thanks
Labels: -hasbisect-per-revision -M-63
jmadill: the bisect may be correct in the sense that the bug doesn't repro on WebGL 1.0 and only on WebGL 2.0. I can't imagine it's a real regression.

I can't reproduce this on Mac. Perhaps it's ANGLE/D3D specific?

Comment 6 by esent...@gmail.com, Sep 12 2017

In the web gł 1.0 I wasn't able to create a depth texture so for that version I use a different code path in which the sky shader doesn't read from the depth buffer, that's why it's working there. So you will not see the problem there.

Yes as stated before this is DX11 specific, because of DX11 requirement to use depth stencil view with D3D11_DSV_READ_ONLY_DEPTH. It's explained in the first post. In my opinion it's a design flaw in DX11 api, for which I would love if Microsoft would lift that requirement, that's why I've created the post on Microsoft website 
It's definitely D3D specific. Related question: Do we detect feedback loops for depth textures when we have depth buffers bound for write? This would be something the spec might need to address.
Components: Internals>GPU>ANGLE
Ok, thanks. Sorry, did a very quick first triage.

jmadill: is this something we want to work around in ANGLE? If so, would someone familiar with D3D be able take it?
Cc: kbr@chromium.org kainino@chromium.org
Owner: jmad...@chromium.org
Yeah, it's not totally simple how to do it, but it's definitely ANGLE related. You can assign it to me, if someone else wants to grab it go for it.

Ken, question for you. How do we handle feedback loops between depth textures and the depth buffer in WebGL 2.0?

Comment 10 by kbr@chromium.org, Sep 12 2017

Blocking: 660844
Cc: zmo@chromium.org yunchao...@intel.com
Labels: -Needs-Triage-M61
In WebGL 2.0 they're not called out specifically in the spec, but it's clear they should be. https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.26 covers the case of the color attachment to the framebuffer for WebGL 1.0, but for WebGL 2.0 (and the WEBGL_draw_buffers extension) this checking needs to be extended not only for multiple color attachments, but also for sampling depth textures.

Yes, the behavior here should be made identical among platforms, and conformance tests should be added. Today, we don't pay attention to the color masks when determining whether there's a rendering feedback loop for color attachments. I'm not sure whether it's feasible to require that if depth writes are disabled, that it's legal to sample a depth texture that's also attached to the FBO. The OpenGL ES 3.0.5 spec section 4.4.3.1 "Rendering Feedback Loops" and section 3.8.10.3 "Rendering Feedback Loops" don't say anything about depth textures and disabling depth writes, so I suspect that at least some OpenGL ES drivers will give undefined results in this scenario. This informs making this pattern illegal in WebGL, requiring copying the texture if it's desired to both have it bound as a sampled texture as well as the current FBO's depth attachment.

Cc: jmad...@chromium.org
Owner: kbr@chromium.org
OK, thanks Ken. If you don't mind I'll put this on you to clarify the intended behaviour, then you can re-assign to me for the implementation when its settled.

Comment 12 by kbr@chromium.org, Sep 12 2017

Sounds fine Jamie.

Comment 13 by kbr@chromium.org, Sep 20 2017

Submitter: it turns out that this topic came up three years ago (!) in the OpenGL ES working group, and on some mobile GPUs it would actually be problematic to first populate the depth texture, and then bind it to a texture unit, disable depth writes, and start sampling it. The state change of disabling depth writes is not sufficient to hint to all OpenGL ES drivers that a pipeline bubble has to be inserted. So your current native GLES 3.0 code is not guaranteed to work according to the spec; you're relying on undefined behavior.

The likely direction is that WebGL will adopt the strict definition in the spec and you'll get INVALID_OPERATION during your draw call, indicating there is a rendering feedback loop. Sorry, but you'll need to copy your depth texture in order to both sample it and perform the depth test against it. We'll need a new WebGL 2.0 conformance test to cover this if this is the resolution.

Jamie: let's wait for the resolution to come back from the ES working group before taking the next step. I'll update this bug then.

Comment 14 by esent...@gmail.com, Sep 20 2017

Thank you for the reply.
I am able to perform this operation on Android Samsung Galaxy Note 4 GLES3, iOS iPad Mini 2 GLES3.
The only potential issues you can encounter with that, is that if you render something to the depth buffer, and then in the next draw call, you try to read in the shader from the depth buffer while in the same render pass, the depth may not get flushed yet from the FBO (on-chip memory) to the texture memory, so you may be reading from texture memory as if the writes in this render pass still haven't been performed (content from previous frame).
However there's a workaround for that - unattach the depth buffer from FBO and reattach it, this way the drivers flush the depth buffer to the texture memory, with this workaround I'm able to read the depth buffer while it's bound to FBO on those devices without issues.
The same is also working when using desktop GL, DX9 and DX11 with the mentioned D3D11_DSV_READ_ONLY_DEPTH flag.
My personal opinion is we should prefer performance instead of introducing artificial limitations.
Performance - with depth attached I can make use of depth tests, thanks to which fewer pixels need to be processed in the shader, and there are no need for depth to texture copies on DX11.
Artificial limitation - the mentioned operation -is- supported in DX11, however needs to be handled with the D3D11_DSV_READ_ONLY_DEPTH.

So the 2 options we have:
1) leave as it is, and I would have to either make an extra copy of the depth buffer to another texture and read from that, or detach the depth buffer from the FBO thus disabling depth tests and forcing the shader to be performed on all pixels even if normally they're occluded - this option would make things run slower

2) add support for D3D11_DSV_READ_ONLY_DEPTH and benefit with faster performance

Thank you

Comment 15 by zmo@chromium.org, Sep 22 2017

We definitely need to add depth texture feedback loop detection and add conformance test for it.

Comment 16 by kbr@chromium.org, Sep 25 2017

The OpenGL ES working group has said they will propose a specification clarification for this. I'll update this bug once it's done.

Thanks Ken. Just ran into this in my own work today and read into the spec a bit. I will share what my reading for posterity.

The 3.0 spec defines Feedback Loops Between Textures and the Framebuffer (4.4.3) and Rendering Feedback Loops (3.8.10.3). For Rendering loops, texture sampling seems undefined regardless of mask values in the framebuffer object. At least, no mention of mask values is made, leading me to believe the texture samples are undefined.

Similarly in the 4.4.3.1 section discussion of Framebuffer loops, no mention of mask values is made.
Note that there is a WebGL test that makes a depth/stencil feedback loop with depth masked out, as discussed in this issue:

https://www.khronos.org/registry/webgl/sdk/tests/conformance2/rendering/depth-stencil-feedback-loop.html?webglVersion=2

Chrome fails this in ToT, but passes with the passthrough command buffer. However, I think the test is not sufficient to test correctness, because ANGLE internally binds an incomplete texture instead of the depth texture, and sample values will not be correct.

Comment 19 by esent...@gmail.com, Nov 14 2017

Is there any news on the matter?
This is for my engine, which you can see the demo here:
http://esenthel.com/site/Esenthel%20Fantasy%20Demo/Esenthel%20Fantasy%20Demo.html
Because of this problem I have to disable Deferred Renderer for now, and can't show the full potential of my engine.
If D3D11_DSV_READ_ONLY_DEPTH would get supported, then the rendering would get a performance boost.
Thank you.

Comment 20 by kbr@chromium.org, Nov 14 2017

I'm sorry, but the OpenGL ES working group seems to have resolved that it will be considered a rendering feedback loop if the same depth/stencil texture is both an input to the current program and bound to the draw framebuffer, regardless of whether depth writes are enabled. Generalizing OpenGL ES' behavior to desktop OpenGL's behavior was problematic. I don't have the details of the discussion.

WebGL will follow the OpenGL ES rules, so this will be forbidden in a future version of all browsers, and a conformance test will be written.

Sorry for this resolution but I hope that you will understand that this is needed for portability to all devices.

Comment 21 by esent...@gmail.com, Nov 14 2017

Thank you for your reply.
I'll start working on a workaround then.
Ken, does this WebGL test still need to be updated?
Cc: enga@chromium.org
Owner: ----
Status: Available (was: Assigned)
Something still needs to be done in this area. I think the test is correct but perhaps not complete. Chrome still has a suppression for the test, and that needs to be resolved.

Let me make this Available since I'm not actively working on it.

Project Member

Comment 24 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/38fe68403b0badd2c8bc92d6396df28b028a64c1

commit 38fe68403b0badd2c8bc92d6396df28b028a64c1
Author: Jamie Madill <jmadill@chromium.org>
Date: Fri Sep 21 06:16:23 2018

Remove secondary Texture rendering loop check.

This check was not used. It applied only to rendering Feedback Loops.
The behaviour is undefined in non-WebGL and for WebGL we have a
separate validation check.

Also update the feedback loop tests to ignore the current GL states.
This change is based on feedback from the OpenGL ES working group.

Bug:  angleproject:2763 
Bug: chromium:763695
Change-Id: I9882b4f9af2d43fc7b5604ff36dadcc79dfd378f
Reviewed-on: https://chromium-review.googlesource.com/1228373
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Yuly Novikov <ynovikov@chromium.org>

[modify] https://crrev.com/38fe68403b0badd2c8bc92d6396df28b028a64c1/src/libANGLE/State.cpp
[modify] https://crrev.com/38fe68403b0badd2c8bc92d6396df28b028a64c1/src/libANGLE/Framebuffer.h
[modify] https://crrev.com/38fe68403b0badd2c8bc92d6396df28b028a64c1/src/libANGLE/renderer/d3d/d3d11/StateManager11.cpp
[modify] https://crrev.com/38fe68403b0badd2c8bc92d6396df28b028a64c1/src/libANGLE/Framebuffer.cpp
[modify] https://crrev.com/38fe68403b0badd2c8bc92d6396df28b028a64c1/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.cpp
[modify] https://crrev.com/38fe68403b0badd2c8bc92d6396df28b028a64c1/src/libANGLE/renderer/d3d/ImageD3D.cpp
[modify] https://crrev.com/38fe68403b0badd2c8bc92d6396df28b028a64c1/src/tests/gl_tests/WebGLCompatibilityTest.cpp

Project Member

Comment 25 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8006ba4895a93f2385c9a35a0d10228b6fd06750

commit 8006ba4895a93f2385c9a35a0d10228b6fd06750
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Fri Sep 21 09:40:06 2018

Roll src/third_party/angle e25b8006115b..ab5fb5edb186 (3 commits)

https://chromium.googlesource.com/angle/angle.git/+log/e25b8006115b..ab5fb5edb186


git log e25b8006115b..ab5fb5edb186 --date=short --no-merges --format='%ad %ae %s'
2018-09-21 oetuaho@nvidia.com Reland "Support EXT_blend_func_extended in the GLES2 context"
2018-09-21 jmadill@chromium.org Remove secondary Texture rendering loop check.
2018-09-21 oetuaho@nvidia.com Fix using a large vertex attrib divisor on D3D11


Created with:
  gclient setdep -r src/third_party/angle@ab5fb5edb186

The AutoRoll server is located here: https://autoroll.skia.org/r/angle-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:763695
TBR=jmadill@chromium.org

Change-Id: I8f4813a39e32e17129ba3231ee14f49a9e0870a8
Reviewed-on: https://chromium-review.googlesource.com/1238314
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#593134}
[modify] https://crrev.com/8006ba4895a93f2385c9a35a0d10228b6fd06750/DEPS

Sign in to add a comment