Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp

Index: third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp b/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp
index bced978dd43864c3a5dab992fd560fd15f494bb0..0ad4f0e2d0fcd519ff229a86a0688aab3e63a410 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp
@@ -203,6 +203,7 @@ WebGLFramebuffer::WebGLFramebuffer(WebGLRenderingContextBase* ctx)
     , m_object(0)
     , m_destructionInProgress(false)
     , m_hasEverBeenBound(false)
+    , m_webGL1DepthStencilConsistent(true)
     , m_readBuffer(GL_COLOR_ATTACHMENT0)
 {
     ctx->contextGL()->GenFramebuffers(1, &m_object);
@@ -221,38 +222,72 @@ WebGLFramebuffer::~WebGLFramebuffer()
 
 void WebGLFramebuffer::setAttachmentForBoundFramebuffer(GLenum target, GLenum attachment, GLenum texTarget, WebGLTexture* texture, GLint level, GLint layer)
 {
-    ASSERT(isBound(target));
-    removeAttachmentFromBoundFramebuffer(target, attachment);
-    if (!m_object)
-        return;
-    if (texture && texture->object()) {
-        m_attachments.add(attachment, WebGLTextureAttachment::create(texture, texTarget, level, layer));
-        drawBuffersIfNecessary(false);
-        texture->onAttached();
+    DCHECK(m_object);
+    DCHECK(isBound(target));
+    if (context()->isWebGL2OrHigher()) {
+        if (attachment == GL_DEPTH_STENCIL_ATTACHMENT) {
+            setAttachmentInternal(target, GL_DEPTH_ATTACHMENT, texTarget, texture, level, layer);
+            setAttachmentInternal(target, GL_STENCIL_ATTACHMENT, texTarget, texture, level, layer);
+        } else {
+            setAttachmentInternal(target, attachment, texTarget, texture, level, layer);
+        }
+        GLuint textureId = objectOrZero(texture);
+        // texTarget can be 0 if detaching using framebufferTextureLayer.
+        DCHECK(texTarget || !textureId);
+        switch (texTarget) {
+        case 0:
+        case GL_TEXTURE_3D:
+        case GL_TEXTURE_2D_ARRAY:
+            context()->contextGL()->FramebufferTextureLayer(target, attachment, textureId, level, layer);
+            break;
+        default:
+            DCHECK_EQ(layer, 0);
+            context()->contextGL()->FramebufferTexture2D(target, attachment, texTarget, textureId, level);
+            break;
+        }
+    } else {
+        DCHECK_EQ(layer, 0);
+        setAttachmentInternal(target, attachment, texTarget, texture, level, layer);
+        switch (attachment) {
+        case GL_DEPTH_ATTACHMENT:
+        case GL_STENCIL_ATTACHMENT:
+        case GL_DEPTH_STENCIL_ATTACHMENT:
+            commitWebGL1DepthStencilIfConsistent(target);
+            break;
+        default:
+            context()->contextGL()->FramebufferTexture2D(target, attachment, texTarget, objectOrZero(texture), level);
+            break;
+        }
     }
 }
 
 void WebGLFramebuffer::setAttachmentForBoundFramebuffer(GLenum target, GLenum attachment, WebGLRenderbuffer* renderbuffer)
 {
-    ASSERT(isBound(target));
-    removeAttachmentFromBoundFramebuffer(target, attachment);
-    if (!m_object)
-        return;
-    if (renderbuffer && renderbuffer->object()) {
-        m_attachments.add(attachment, WebGLRenderbufferAttachment::create(renderbuffer));
-        drawBuffersIfNecessary(false);
-        renderbuffer->onAttached();
+    DCHECK(m_object);
+    DCHECK(isBound(target));
+    if (context()->isWebGL2OrHigher()) {
+        if (attachment == GL_DEPTH_STENCIL_ATTACHMENT) {
+            setAttachmentInternal(target, GL_DEPTH_ATTACHMENT, renderbuffer);
+            setAttachmentInternal(target, GL_STENCIL_ATTACHMENT, renderbuffer);
+        } else {
+            setAttachmentInternal(target, attachment, renderbuffer);
+        }
+        context()->contextGL()->FramebufferRenderbuffer(target, attachment, GL_RENDERBUFFER, objectOrZero(renderbuffer));
+    } else {
+        setAttachmentInternal(target, attachment, renderbuffer);
+        switch (attachment) {
+        case GL_DEPTH_ATTACHMENT:
+        case GL_STENCIL_ATTACHMENT:
+        case GL_DEPTH_STENCIL_ATTACHMENT:
+            commitWebGL1DepthStencilIfConsistent(target);
+            break;
+        default:
+            context()->contextGL()->FramebufferRenderbuffer(target, attachment, GL_RENDERBUFFER, objectOrZero(renderbuffer));
+            break;
+        }
     }
 }
 
-void WebGLFramebuffer::attach(GLenum target, GLenum attachment, GLenum attachmentPoint)
-{
-    ASSERT(isBound(target));
-    WebGLAttachment* attachmentObject = getAttachment(attachment);
-    if (attachmentObject)
-        attachmentObject->attach(context()->contextGL(), target, attachmentPoint);
-}
-
 WebGLSharedObject* WebGLFramebuffer::getAttachmentObject(GLenum attachment) const
 {
     if (!m_object)
@@ -267,30 +302,6 @@ WebGLFramebuffer::WebGLAttachment* WebGLFramebuffer::getAttachment(GLenum attach
     return (it != m_attachments.end()) ? it->value.get() : 0;
 }
 
-void WebGLFramebuffer::removeAttachmentFromBoundFramebuffer(GLenum target, GLenum attachment)
-{
-    ASSERT(isBound(target));
-    if (!m_object)
-        return;
-
-    WebGLAttachment* attachmentObject = getAttachment(attachment);
-    if (attachmentObject) {
-        attachmentObject->onDetached(context()->contextGL());
-        m_attachments.remove(attachment);
-        drawBuffersIfNecessary(false);
-        switch (attachment) {
-        case GL_DEPTH_STENCIL_ATTACHMENT:
-            attach(target, GL_DEPTH_ATTACHMENT, GL_DEPTH_ATTACHMENT);
-            attach(target, GL_STENCIL_ATTACHMENT, GL_STENCIL_ATTACHMENT);
-            break;
-        case GL_DEPTH_ATTACHMENT:
-        case GL_STENCIL_ATTACHMENT:
-            attach(target, GL_DEPTH_STENCIL_ATTACHMENT, GL_DEPTH_STENCIL_ATTACHMENT);
-            break;
-        }
-    }
-}
-
 void WebGLFramebuffer::removeAttachmentFromBoundFramebuffer(GLenum target, WebGLSharedObject* attachment)
 {
     ASSERT(isBound(target));
@@ -300,51 +311,44 @@ void WebGLFramebuffer::removeAttachmentFromBoundFramebuffer(GLenum target, WebGL
         return;
 
     bool checkMore = true;
+    bool isWebGL1 = !context()->isWebGL2OrHigher();
+    bool checkWebGL1DepthStencil = false;
     while (checkMore) {
         checkMore = false;
         for (const auto& it : m_attachments) {
             WebGLAttachment* attachmentObject = it.value.get();
             if (attachmentObject->isSharedObject(attachment)) {
                 GLenum attachmentType = it.key;
-                attachmentObject->unattach(context()->contextGL(), target, attachmentType);
-                removeAttachmentFromBoundFramebuffer(target, attachmentType);
+                switch (attachmentType) {
+                case GL_DEPTH_ATTACHMENT:
+                case GL_STENCIL_ATTACHMENT:
+                case GL_DEPTH_STENCIL_ATTACHMENT:
+                    if (isWebGL1) {
+                        checkWebGL1DepthStencil = true;
+                    } else {
+                        attachmentObject->unattach(context()->contextGL(), target, attachmentType);
+                    }
+                    break;
+                default:
+                    attachmentObject->unattach(context()->contextGL(), target, attachmentType);
+                    break;
+                }
+                removeAttachmentInternal(target, attachmentType);
                 checkMore = true;
                 break;
             }
         }
     }
+    if (checkWebGL1DepthStencil)
+        commitWebGL1DepthStencilIfConsistent(target);
 }
 
 GLenum WebGLFramebuffer::checkDepthStencilStatus(const char** reason) const
 {
-    if (context()->isWebGL2OrHigher())
+    if (context()->isWebGL2OrHigher() || m_webGL1DepthStencilConsistent)
         return GL_FRAMEBUFFER_COMPLETE;
-    WebGLAttachment* depthAttachment = nullptr;
-    WebGLAttachment* stencilAttachment = nullptr;
-    WebGLAttachment* depthStencilAttachment = nullptr;
-    for (const auto& it : m_attachments) {
-        WebGLAttachment* attachment = it.value.get();
-        ASSERT(attachment);
-        switch (it.key) {
-        case GL_DEPTH_ATTACHMENT:
-            depthAttachment = attachment;
-            break;
-        case GL_STENCIL_ATTACHMENT:
-            stencilAttachment = attachment;
-            break;
-        case GL_DEPTH_STENCIL_ATTACHMENT:
-            depthStencilAttachment = attachment;
-            break;
-        default:
-            break;
-        }
-    }
-    if ((depthStencilAttachment && (depthAttachment || stencilAttachment))
-        || (depthAttachment && stencilAttachment)) {
-        *reason = "conflicting DEPTH/STENCIL/DEPTH_STENCIL attachments";
-        return GL_FRAMEBUFFER_UNSUPPORTED;
-    }
-    return GL_FRAMEBUFFER_COMPLETE;
+    *reason = "conflicting DEPTH/STENCIL/DEPTH_STENCIL attachments";
+    return GL_FRAMEBUFFER_UNSUPPORTED;
 }
 
 bool WebGLFramebuffer::hasStencilBuffer() const
@@ -411,6 +415,89 @@ void WebGLFramebuffer::drawBuffersIfNecessary(bool force)
     }
 }
 
+void WebGLFramebuffer::setAttachmentInternal(GLenum target, GLenum attachment, GLenum texTarget, WebGLTexture* texture, GLint level, GLint layer)
+{
+    DCHECK(isBound(target));
+    DCHECK(m_object);
+    removeAttachmentInternal(target, attachment);
+    if (texture && texture->object()) {
+        m_attachments.add(attachment, WebGLTextureAttachment::create(texture, texTarget, level, layer));
+        drawBuffersIfNecessary(false);
+        texture->onAttached();
+    }
+}
+
+void WebGLFramebuffer::setAttachmentInternal(GLenum target, GLenum attachment, WebGLRenderbuffer* renderbuffer)
+{
+    DCHECK(isBound(target));
+    DCHECK(m_object);
+    removeAttachmentInternal(target, attachment);
+    if (renderbuffer && renderbuffer->object()) {
+        m_attachments.add(attachment, WebGLRenderbufferAttachment::create(renderbuffer));
+        drawBuffersIfNecessary(false);
+        renderbuffer->onAttached();
+    }
+}
+
+void WebGLFramebuffer::removeAttachmentInternal(GLenum target, GLenum attachment)
+{
+    DCHECK(isBound(target));
+    DCHECK(m_object);
+
+    WebGLAttachment* attachmentObject = getAttachment(attachment);
+    if (attachmentObject) {
+        attachmentObject->onDetached(context()->contextGL());
+        m_attachments.remove(attachment);
+        drawBuffersIfNecessary(false);
+    }
+}
+
+void WebGLFramebuffer::commitWebGL1DepthStencilIfConsistent(GLenum target)
+{
+    DCHECK(!context()->isWebGL2OrHigher());
+    WebGLAttachment* depthAttachment = nullptr;
+    WebGLAttachment* stencilAttachment = nullptr;
+    WebGLAttachment* depthStencilAttachment = nullptr;
+    int count = 0;
+    for (const auto& it : m_attachments) {
+        WebGLAttachment* attachment = it.value.get();
+        DCHECK(attachment);
+        switch (it.key) {
+        case GL_DEPTH_ATTACHMENT:
+            depthAttachment = attachment;
+            ++count;
+            break;
+        case GL_STENCIL_ATTACHMENT:
+            stencilAttachment = attachment;
+            ++count;
+            break;
+        case GL_DEPTH_STENCIL_ATTACHMENT:
+            depthStencilAttachment = attachment;
+            ++count;
+            break;
+        default:
+            break;
+        }
+    }
+
+    m_webGL1DepthStencilConsistent = count <= 1;
+    if (!m_webGL1DepthStencilConsistent)
+        return;

[Ken Russell (switch to Gerrit), 2016/09/29 03:07:43]

Can this lazy flushing of the framebuffer's attachments cause Blink's idea of
the framebuffer's state to get out of sync with respect to the command buffer's
in a way that's observable? It looks like everything's OK to me:
 - calls which modify the draw framebuffer are all handled correctly via calls
to WebGLFramebuffer::checkDepthStencilStatus()
 - queries of the framebuffer's attachments return Blink's cached values
 - the command buffer's notion of framebuffer completeness includes the WebGL
1.0 rules per the change to Framebuffer::IsPossiblyComplete above

I can't think of anything that could be missing. Mo, can you?

+
+    gpu::gles2::GLES2Interface* gl = context()->contextGL();
+    if (depthAttachment) {
+        depthAttachment->attach(gl, target, GL_DEPTH_ATTACHMENT);
+        gl->FramebufferRenderbuffer(target, GL_STENCIL_ATTACHMENT, GL_RENDERBUFFER, 0);
+    } else if (stencilAttachment) {
+        gl->FramebufferRenderbuffer(target, GL_DEPTH_ATTACHMENT, GL_RENDERBUFFER, 0);
+        stencilAttachment->attach(gl, target, GL_STENCIL_ATTACHMENT);
+    } else if (depthStencilAttachment) {
+        depthStencilAttachment->attach(gl, target, GL_DEPTH_STENCIL_ATTACHMENT);
+    } else {
+        gl->FramebufferRenderbuffer(target, GL_DEPTH_STENCIL_ATTACHMENT, GL_RENDERBUFFER, 0);
+    }
+}
+
 GLenum WebGLFramebuffer::getDrawBuffer(GLenum drawBuffer)
 {
     int index = static_cast<int>(drawBuffer - GL_DRAW_BUFFER0_EXT);