WebGL 2: validate read buffer attachment when reading from FBO

Source/core/html/canvas/WebGL2RenderingContextBase.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "core/html/canvas/WebGL2RenderingContextBase.h"
 
 #include "bindings/core/v8/WebGLAny.h"
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLImageElement.h"
@@ skipping 49 matching lines @@
     m_currentTransformFeedbackPrimitivesWrittenQuery = nullptr;
 }
 
 void WebGL2RenderingContextBase::initializeNewContext()
 {
     ASSERT(!isContextLost());
     ASSERT(drawingBuffer());
 
     m_readFramebufferBinding = nullptr;
 
+    // set the default value of read buffer for drawing buffer
+    m_readBuffer = GL_BACK;
+
     m_boundCopyReadBuffer = nullptr;
     m_boundCopyWriteBuffer = nullptr;
     m_boundPixelPackBuffer = nullptr;
     m_boundPixelUnpackBuffer = nullptr;
     m_boundTransformFeedbackBuffer = nullptr;
     m_boundUniformBuffer = nullptr;
 
     m_currentBooleanOcclusionQuery = nullptr;
     m_currentTransformFeedbackPrimitivesWrittenQuery = nullptr;
 
@@ skipping 87 matching lines @@
         return;
 
     webContext()->invalidateSubFramebuffer(target, attachments.size(), attachments.data(), x, y, width, height);
 }
 
 void WebGL2RenderingContextBase::readBuffer(GLenum mode)
 {
     if (isContextLost())
         return;
 
+    switch (mode) {
+    case GL_BACK:
+    case GL_NONE:
+    case GL_COLOR_ATTACHMENT0:
+        break;
+    default:
+        if (mode > GL_COLOR_ATTACHMENT0
+            && mode < static_cast<GLenum>(GL_COLOR_ATTACHMENT0 + maxColorAttachments()))
+            break;
+        synthesizeGLError(GL_INVALID_ENUM, "readBuffer", "invalid read buffer");
+        return;
+    }
+
+    WebGLFramebuffer* readFramebufferBinding = getFramebufferBinding(GL_READ_FRAMEBUFFER);
+    if (!readFramebufferBinding) {
+        ASSERT(drawingBuffer());
+        if (mode != GL_BACK && mode != GL_NONE) {
+            synthesizeGLError(GL_INVALID_OPERATION, "readBuffer", "invalid read buffer");
+            return;
+        }
+        // translate GL_BACK to GL_COLOR_ATTACHMENT0, because the default
+        // framebuffer for WebGL is not fb 0, it is an internal fbo.
+        if (mode == GL_BACK)
+            mode = GL_COLOR_ATTACHMENT0;
+        m_readBuffer = mode;

[Zhenyao Mo, 2015/07/06 17:52:25]

I think it's better to cache the original value here instead of the mapped ones,
but it's just my opinion.  See from your code, it actually doesn't make a
difference in logic.

[Ken Russell (switch to Gerrit), 2015/07/06 21:19:13]

I agree with Mo; it's clearer if the cache holds the un-translated value. Also,
m_readBuffer is not currently ever fetched. See below.

[yunchao, 2015/07/07 08:10:39]

Yeah. Agree with you two. It should cache the original value, and fetch the
cached value in getter.

+    } else {
+        if (mode == GL_BACK) {
+            synthesizeGLError(GL_INVALID_OPERATION, "readBuffer", "invalid read buffer");
+            return;
+        }
+        readFramebufferBinding->readBuffer(mode);
+    }
     webContext()->readBuffer(mode);
 }
 
 void WebGL2RenderingContextBase::renderbufferStorageImpl(
     GLenum target, GLsizei samples, GLenum internalformat, GLsizei width, GLsizei height,
     const char* functionName)
 {
     switch (internalformat) {
     case GL_R8UI:
     case GL_R8I:
@@ skipping 1583 matching lines @@
         return getIntParameter(scriptState, pname);
     case GL_PACK_SKIP_ROWS:
         return getIntParameter(scriptState, pname);
     case GL_PIXEL_PACK_BUFFER_BINDING:
         return WebGLAny(scriptState, PassRefPtrWillBeRawPtr<WebGLObject>(m_boundPixelPackBuffer.get()));
     case GL_PIXEL_UNPACK_BUFFER_BINDING:
         return WebGLAny(scriptState, PassRefPtrWillBeRawPtr<WebGLObject>(m_boundPixelUnpackBuffer.get()));
     case GL_RASTERIZER_DISCARD:
         return getBooleanParameter(scriptState, pname);
     case GL_READ_BUFFER:
-        return getUnsignedIntParameter(scriptState, pname);
+        {
+            GLint value = 0;
+            if (!isContextLost()) {
+                webContext()->getIntegerv(pname, &value);
+                // translate GL_COLOR_ATTACHMENT0 to GL_BACK for the default framebuffer
+                if (value == GL_COLOR_ATTACHMENT0 && !getFramebufferBinding(GL_READ_FRAMEBUFFER))
+                    value = GL_BACK;

[Ken Russell (switch to Gerrit), 2015/07/06 21:19:13]

The point of caching m_readBuffer is so that its value can be used here instead
of calling getIntegerv and doing this somewhat error-prone translation. Could
you please modify this code to return the readBuffer from the bound read
framebuffer (if any), or m_readBuffer if there is none?

[yunchao, 2015/07/07 08:10:39]

Done.

+            }
+            return WebGLAny(scriptState, static_cast<unsigned>(value));
+        }
     case GL_READ_FRAMEBUFFER_BINDING:
         return WebGLAny(scriptState, PassRefPtrWillBeRawPtr<WebGLObject>(m_readFramebufferBinding.get()));
     case GL_SAMPLE_ALPHA_TO_COVERAGE:
         return getBooleanParameter(scriptState, pname);
     case GL_SAMPLE_COVERAGE:
         return getBooleanParameter(scriptState, pname);
     // case GL_SAMPLER_BINDING WebGLSampler
     case GL_TEXTURE_BINDING_2D_ARRAY:
         return WebGLAny(scriptState, PassRefPtrWillBeRawPtr<WebGLObject>(m_textureUnits[m_activeTextureUnit].m_texture2DArrayBinding.get()));
     case GL_TEXTURE_BINDING_3D:
@@ skipping 433 matching lines @@
 GLenum WebGL2RenderingContextBase::boundFramebufferColorFormat()
 {
     if (m_readFramebufferBinding && m_readFramebufferBinding->object())
         return m_readFramebufferBinding->colorBufferFormat();
     if (m_requestedAttributes.alpha())
         return GL_RGBA;
     return GL_RGB;
 }
 
 } // namespace blink