WebGL 2: fix the bug in invalidate-framebuffer.html

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

Index: third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
index 41eba55f6a1455e1b350099b0c0923cb146e6e85..5f49bb90b5edf0f120f40180d3eab6121f68491d 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
@@ -375,11 +375,66 @@ ScriptValue WebGL2RenderingContextBase::getInternalformatParameter(ScriptState*
     }
 }
 
+bool WebGL2RenderingContextBase::checkAndTranslateAttachments(const char* functionName, GLenum target, Vector<GLenum>& attachments)

[Ken Russell (switch to Gerrit), 2015/11/17 23:06:37]

It's a little nasty to modify the Vector which comes in from the JavaScript
bindings. I think it would be better to mark it as a const Vector& of GLenum in
WebGL2RenderingContextBase.h and here, and allocate a new vector with the
translated values.

[yunchao, 2015/11/19 01:43:00]

Done.

+{
+    GLsizei size = attachments.size();
+    GLenum* buffer = attachments.data();

[Ken Russell (switch to Gerrit), 2015/11/17 23:06:37]

It's not necessary to fetch the data pointer out of "attachments" -- just
reference attachments[i] directly.

[yunchao, 2015/11/19 01:42:59]

Done.

[yunchao, 2015/11/19 01:42:59]

Done.

+
+    WebGLFramebuffer* framebufferBinding = getFramebufferBinding(target);
+    ASSERT(framebufferBinding || drawingBuffer());
+    if (!framebufferBinding) {
+        // For the default framebuffer
+        // Translate GL_BACK/GL_DEPTH/GL_STENCIL, because the default framebuffer of WebGL is not fb 0, it is an internal fbo
+        for (GLsizei i = 0; i < size; ++i) {
+            switch (buffer[i]) {
+            case GL_BACK:

[yunchao, 2015/11/12 12:14:50]

In GLES spec, it is GL_COLOR, but in WebGL spec, it is GL_BACK (say in
drawBuffer). Both of them means GL_COLOR_ATTACHMENT0 of the internal fbo
(default framebuffer of WebGL).

I would like it is GL_BACK, then it is aligned with other APIs in WebGL spec.

[Ken Russell (switch to Gerrit), 2015/11/17 23:06:37]

Sorry, but no, this should follow the OpenGL ES specification. WebGL does not
deviate arbitrarily from the ES specs.

[yunchao, 2015/11/19 01:42:59]

Done.

+                buffer[i] = GL_COLOR_ATTACHMENT0;
+                break;
+            case GL_DEPTH:
+                buffer[i] = GL_DEPTH_ATTACHMENT;
+                break;
+            case GL_STENCIL:
+                buffer[i] = GL_STENCIL_ATTACHMENT;
+                break;
+            default:
+                synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid attachment");
+                return false;
+            }
+        }
+    } else {
+        // For the FBO
+        for (GLsizei i = 0; i < size; ++i) {
+            switch (buffer[i]) {
+            case GL_COLOR_ATTACHMENT0:
+            case GL_DEPTH_ATTACHMENT:
+            case GL_STENCIL_ATTACHMENT:
+            case GL_DEPTH_STENCIL_ATTACHMENT:
+                break;
+            default:
+                if (buffer[i] > GL_COLOR_ATTACHMENT0
+                    && buffer[i] < static_cast<GLenum>(GL_COLOR_ATTACHMENT0 + maxColorAttachments()))
+                    break;
+                synthesizeGLError(GL_INVALID_OPERATION, functionName, "invalid attachment");
+                return false;
+            }
+        }
+    }
+    return true;
+}
+
 void WebGL2RenderingContextBase::invalidateFramebuffer(GLenum target, Vector<GLenum>& attachments)
 {
     if (isContextLost())
         return;
 
+    if (!validateFramebufferTarget(target)) {
+        synthesizeGLError(GL_INVALID_ENUM, "invalidateFramebuffer", "invalid target");
+        return;
+    }
+
+    if (!checkAndTranslateAttachments("invalidateFramebuffe", target, attachments))

[Ken Russell (switch to Gerrit), 2015/11/17 23:06:37]

invalidateFramebuffe -> invalidateFramebuffer

[yunchao, 2015/11/19 01:42:59]

Done.

+        return;
+
     webContext()->invalidateFramebuffer(target, attachments.size(), attachments.data());
 }
 
@@ -388,6 +443,19 @@ void WebGL2RenderingContextBase::invalidateSubFramebuffer(GLenum target, Vector<
     if (isContextLost())
         return;
 
+    if (!validateFramebufferTarget(target)) {
+        synthesizeGLError(GL_INVALID_ENUM, "invalidateFramebuffer", "invalid target");
+        return;
+    }
+
+    if (width < 0 || height < 0) {

[yunchao, 2015/11/12 12:14:51]

Not sure about this code snippet, In OGL spec, it will raise a GL_INVALID_VALUE
for this case, but it seems be OK in GLES spec.

[Ken Russell (switch to Gerrit), 2015/11/17 23:06:37]

In this case it seems better to be safer by generating INVALID_VALUE as is
already tested by conformance2/renderbuffers/invalidate-framebuffer.html.
Otherwise it'll be necessary to clamp these to 0 to avoid INVALID_VALUE errors
from OpenGL drivers.

[yunchao, 2015/11/19 01:43:00]

OK, let's generate GL_INVALID_ENUM for this case.

+        synthesizeGLError(GL_INVALID_VALUE, "invalidateSubFramebuffer", "invalid width or height");
+        return;
+    }
+
+    if (!checkAndTranslateAttachments("invalidateSubFramebuffe", target, attachments))

[Ken Russell (switch to Gerrit), 2015/11/17 23:06:37]

invalidateSubFramebuffe -> invalidateSubFramebuffer

[yunchao, 2015/11/19 01:43:00]

Done.

+        return;
+
     webContext()->invalidateSubFramebuffer(target, attachments.size(), attachments.data(), x, y, width, height);
 }