WebGL 2: add read/write framebuffer binding points to related APIs

Source/core/html/canvas/WebGLRenderingContextBase.cpp

Index: Source/core/html/canvas/WebGLRenderingContextBase.cpp
diff --git a/Source/core/html/canvas/WebGLRenderingContextBase.cpp b/Source/core/html/canvas/WebGLRenderingContextBase.cpp
index 177893cd0e25848484b4d2a88363ac1d16799430..96b610391a61f61f88d61c4c442288e0fdee9a6d 100644
--- a/Source/core/html/canvas/WebGLRenderingContextBase.cpp
+++ b/Source/core/html/canvas/WebGLRenderingContextBase.cpp
@@ -224,9 +224,10 @@ namespace {
     class ScopedDrawingBufferBinder {
         STACK_ALLOCATED();
     public:
-        ScopedDrawingBufferBinder(DrawingBuffer* drawingBuffer, WebGLFramebuffer* framebufferBinding)
+        ScopedDrawingBufferBinder(DrawingBuffer* drawingBuffer, WebGLFramebuffer* framebufferBinding, GLenum target)

[Zhenyao Mo, 2015/06/09 20:29:51]

That's why I suggest you have an idea of drawFramebufferBinding and
readFramebufferBinding in the base class, even though they are just the same
framebuffer in WebGL 1, at least you can always be explicit which one is
intended. Otherwise it's very easy to get confused and make mistake.

See below why.

[yunchao, 2015/06/10 08:21:25]

I got it. But I think it is not a must. see the latest code.

             : m_drawingBuffer(drawingBuffer)
             , m_framebufferBinding(framebufferBinding)
+            , m_target(target)
         {
             // Commit DrawingBuffer if needed (e.g., for multisampling)
             if (!m_framebufferBinding && m_drawingBuffer)

[Zhenyao Mo, 2015/06/09 20:29:51]

This should be readFramebufferBinding

[yunchao, 2015/06/10 08:21:25]

Agree. I rename this variable to m_readFramebufferBinding to make it clear.

@@ -237,12 +238,13 @@ namespace {
         {
             // Restore DrawingBuffer if needed
             if (!m_framebufferBinding && m_drawingBuffer)

[Zhenyao Mo, 2015/06/09 20:29:50]

This actually should be drawFramebufferBinding.

[Zhenyao Mo, 2015/06/10 05:01:45]

This should also be readFramebufferBinding. Because ScopedDrawingBufferBinder is
used for ReadPixels/CopyTexImage2D/CopySubImage2D in the situation of reading
from a multisampled DrawingBuffer, so we need to blit to a single sampled buffer
for reading, during which the bindings could be changed and need to be
recovered. The below code where you have two ScopedDrawingBufferBinder doesn't
seem correct. Instead, for WebGL 1, this Binder class is correct, but if you
want it to work also for WebGL 2, you basically need to have
readFramebufferBinding and drawFramebufferBinding as Binder class members, so
you can recover them in the destructor.

[yunchao, 2015/06/10 08:21:25]

To make it clear, I add a comment for the class ScopedDrawingBufferBinder. It is
extracted from your comment above, :)

-                m_drawingBuffer->bind();
+                m_drawingBuffer->bind(m_target);
         }
 
     private:
         DrawingBuffer* m_drawingBuffer;
         RawPtrWillBeMember<WebGLFramebuffer> m_framebufferBinding;
+        GLenum m_target;
     };
 
     GLint clamp(GLint value, GLint min, GLint max)
@@ -912,8 +914,7 @@ WebGLRenderingContextBase::HowToClear WebGLRenderingContextBase::clearIfComposit
     drawingBuffer()->clearFramebuffers(clearMask);
 
     restoreStateAfterClear();
-    if (m_framebufferBinding)
-        webContext()->bindFramebuffer(GL_FRAMEBUFFER, objectOrZero(m_framebufferBinding.get()));
+    drawingBuffer()->restoreFramebufferBindings();
     drawingBuffer()->setBufferClearNeeded(false);
 
     return combinedClear ? CombinedClear : JustClear;
@@ -1018,8 +1019,7 @@ void WebGLRenderingContextBase::reshape(int width, int height)
 
     webContext()->bindTexture(GL_TEXTURE_2D, objectOrZero(m_textureUnits[m_activeTextureUnit].m_texture2DBinding.get()));
     webContext()->bindRenderbuffer(GL_RENDERBUFFER, objectOrZero(m_renderbufferBinding.get()));
-    if (m_framebufferBinding)
-        webContext()->bindFramebuffer(GL_FRAMEBUFFER, objectOrZero(m_framebufferBinding.get()));
+    drawingBuffer()->restoreFramebufferBindings();
 }
 
 int WebGLRenderingContextBase::drawingBufferWidth() const
@@ -1370,7 +1370,9 @@ bool WebGLRenderingContextBase::validateFramebufferTarget(GLenum target)
 
 WebGLFramebuffer* WebGLRenderingContextBase::getFramebufferBinding(GLenum target)
 {
-    return m_framebufferBinding.get();
+    if (target == GL_FRAMEBUFFER)
+        return m_framebufferBinding.get();
+    return nullptr;
 }
 
 GLenum WebGLRenderingContextBase::checkFramebufferStatus(GLenum target)
@@ -1381,10 +1383,11 @@ GLenum WebGLRenderingContextBase::checkFramebufferStatus(GLenum target)
         synthesizeGLError(GL_INVALID_ENUM, "checkFramebufferStatus", "invalid target");
         return 0;
     }
-    if (!getFramebufferBinding(target) || !getFramebufferBinding(target)->object())
+    WebGLFramebuffer* framebufferBinding = getFramebufferBinding(target);
+    if (!framebufferBinding || !framebufferBinding->object())
         return GL_FRAMEBUFFER_COMPLETE;
     const char* reason = "framebuffer incomplete";
-    GLenum result = m_framebufferBinding->checkStatus(&reason);
+    GLenum result = framebufferBinding->checkStatus(&reason);
     if (result != GL_FRAMEBUFFER_COMPLETE) {
         emitGLWarning("checkFramebufferStatus", reason);
         return result;
@@ -1402,7 +1405,7 @@ void WebGLRenderingContextBase::clear(GLbitfield mask)
         return;
     }
     const char* reason = "framebuffer incomplete";
-    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), &reason)) {
+    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), GL_FRAMEBUFFER, &reason)) {
         synthesizeGLError(GL_INVALID_FRAMEBUFFER_OPERATION, "clear", reason);
         return;
     }
@@ -1563,12 +1566,15 @@ void WebGLRenderingContextBase::copyTexImage2D(GLenum target, GLint level, GLenu
         return;
     }
     const char* reason = "framebuffer incomplete";
-    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), &reason)) {
+    if ((m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), GL_FRAMEBUFFER, &reason))
+        || (getFramebufferBinding(GL_READ_FRAMEBUFFER) && !getFramebufferBinding(GL_READ_FRAMEBUFFER)->onAccess(webContext(), GL_READ_FRAMEBUFFER, &reason))) {
         synthesizeGLError(GL_INVALID_FRAMEBUFFER_OPERATION, "copyTexImage2D", reason);
         return;
     }
     clearIfComposited();
-    ScopedDrawingBufferBinder binder(drawingBuffer(), m_framebufferBinding.get());
+    ScopedDrawingBufferBinder binder(drawingBuffer(), m_framebufferBinding.get(), GL_FRAMEBUFFER);
+    if (isWebGL2OrHigher())
+        ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER);

[Zhenyao Mo, 2015/06/09 20:29:50]

This is incorrect.  readBinder is only alive with the if {} clause. Besides, as
I mentioned in DrawingBuffer, there is no need to recover READ_FRAMEBUFFER for
DrawingBuffer.

[Zhenyao Mo, 2015/06/10 05:01:46]

See my second comments in ScopedDrawingBufferBinder. If you want to handle
WebGL2 case here, you should upgrade the ScopedDrawingBufferBinder to handle
both READ and DRAW bindings all together in one Binder, not separately in two
Binders.

[yunchao, 2015/06/10 08:21:25]

I got it. The think the latest code set the correct binding point for Binder.

     webContext()->copyTexImage2D(target, level, internalformat, x, y, width, height, border);
     // FIXME: if the framebuffer is not complete, none of the below should be executed.
     tex->setLevelInfo(target, level, internalformat, width, height, 1, GL_UNSIGNED_BYTE);
@@ -1606,12 +1612,15 @@ void WebGLRenderingContextBase::copyTexSubImage2D(GLenum target, GLint level, GL
         return;
     }
     const char* reason = "framebuffer incomplete";
-    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), &reason)) {
+    if ((m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), GL_FRAMEBUFFER, &reason))
+        || (getFramebufferBinding(GL_READ_FRAMEBUFFER) && !getFramebufferBinding(GL_READ_FRAMEBUFFER)->onAccess(webContext(), GL_READ_FRAMEBUFFER, &reason))) {
         synthesizeGLError(GL_INVALID_FRAMEBUFFER_OPERATION, "copyTexSubImage2D", reason);
         return;
     }
     clearIfComposited();
-    ScopedDrawingBufferBinder binder(drawingBuffer(), m_framebufferBinding.get());
+    ScopedDrawingBufferBinder binder(drawingBuffer(), m_framebufferBinding.get(), GL_FRAMEBUFFER);
+    if (isWebGL2OrHigher())
+        ScopedDrawingBufferBinder readBinder(drawingBuffer(), getFramebufferBinding(GL_READ_FRAMEBUFFER), GL_READ_FRAMEBUFFER);

[Zhenyao Mo, 2015/06/09 20:29:50]

Same here.

[Zhenyao Mo, 2015/06/10 05:01:46]

See my second comments in ScopedDrawingBufferBinder. If you want to handle
WebGL2 case here, you should upgrade the ScopedDrawingBufferBinder to handle
both READ and DRAW bindings all together in one Binder, not separately in two
Binders.

[yunchao, 2015/06/10 08:21:25]

I got it. The think the latest code set the correct binding point for Binder.

     webContext()->copyTexSubImage2D(target, level, xoffset, yoffset, x, y, width, height);
 }
 
@@ -1731,8 +1740,8 @@ void WebGLRenderingContextBase::deleteFramebuffer(WebGLFramebuffer* framebuffer)
         return;
     if (framebuffer == m_framebufferBinding) {
         m_framebufferBinding = nullptr;
-        drawingBuffer()->setFramebufferBinding(0);
-        // Have to call bindFramebuffer here to bind back to internal fbo.
+        drawingBuffer()->setFramebufferBinding(GL_FRAMEBUFFER, 0);
+        // Have to call drawingBuffer()->bind() here to bind back to internal fbo.

[Zhenyao Mo, 2015/06/09 20:29:50]

By the way, I just noticed, you should get rid of the default value for target
in bind() and explicitly call with bind(GL_FRAMEBUFFER).  There are only a few
places.

[yunchao, 2015/06/10 08:21:25]

Done.

         drawingBuffer()->bind();
     }
 }
@@ -1751,7 +1760,9 @@ void WebGLRenderingContextBase::deleteRenderbuffer(WebGLRenderbuffer* renderbuff
     if (renderbuffer == m_renderbufferBinding)
         m_renderbufferBinding = nullptr;
     if (m_framebufferBinding)
-        m_framebufferBinding->removeAttachmentFromBoundFramebuffer(renderbuffer);
+        m_framebufferBinding->removeAttachmentFromBoundFramebuffer(GL_FRAMEBUFFER, renderbuffer);
+    if (getFramebufferBinding(GL_READ_FRAMEBUFFER))
+        getFramebufferBinding(GL_READ_FRAMEBUFFER)->removeAttachmentFromBoundFramebuffer(GL_READ_FRAMEBUFFER, renderbuffer);
 }
 
 void WebGLRenderingContextBase::deleteShader(WebGLShader* shader)
@@ -1788,7 +1799,9 @@ void WebGLRenderingContextBase::deleteTexture(WebGLTexture* texture)
         }
     }
     if (m_framebufferBinding)
-        m_framebufferBinding->removeAttachmentFromBoundFramebuffer(texture);
+        m_framebufferBinding->removeAttachmentFromBoundFramebuffer(GL_FRAMEBUFFER, texture);
+    if (getFramebufferBinding(GL_READ_FRAMEBUFFER))
+        getFramebufferBinding(GL_READ_FRAMEBUFFER)->removeAttachmentFromBoundFramebuffer(GL_READ_FRAMEBUFFER, texture);
 
     // If the deleted was bound to the the current maximum index, trace backwards to find the new max texture index
     if (m_onePlusMaxNonDefaultTextureUnit == static_cast<unsigned long>(maxBoundTextureIndex + 1)) {
@@ -2008,7 +2021,8 @@ void WebGLRenderingContextBase::framebufferRenderbuffer(GLenum target, GLenum at
     // Don't allow the default framebuffer to be mutated; all current
     // implementations use an FBO internally in place of the default
     // FBO.
-    if (!m_framebufferBinding || !m_framebufferBinding->object()) {
+    WebGLFramebuffer* framebufferBinding = getFramebufferBinding(target);
+    if (!framebufferBinding || !framebufferBinding->object()) {
         synthesizeGLError(GL_INVALID_OPERATION, "framebufferRenderbuffer", "no framebuffer bound");
         return;
     }
@@ -2031,7 +2045,7 @@ void WebGLRenderingContextBase::framebufferRenderbuffer(GLenum target, GLenum at
     default:
         webContext()->framebufferRenderbuffer(target, attachment, renderbuffertarget, bufferObject);
     }
-    m_framebufferBinding->setAttachmentForBoundFramebuffer(attachment, buffer);
+    framebufferBinding->setAttachmentForBoundFramebuffer(target, attachment, buffer);
     applyStencilTest();
 }
 
@@ -2050,7 +2064,8 @@ void WebGLRenderingContextBase::framebufferTexture2D(GLenum target, GLenum attac
     // Don't allow the default framebuffer to be mutated; all current
     // implementations use an FBO internally in place of the default
     // FBO.
-    if (!m_framebufferBinding || !m_framebufferBinding->object()) {
+    WebGLFramebuffer* framebufferBinding = getFramebufferBinding(target);
+    if (!framebufferBinding || !framebufferBinding->object()) {
         synthesizeGLError(GL_INVALID_OPERATION, "framebufferTexture2D", "no framebuffer bound");
         return;
     }
@@ -2060,16 +2075,10 @@ void WebGLRenderingContextBase::framebufferTexture2D(GLenum target, GLenum attac
         webContext()->framebufferTexture2D(target, GL_DEPTH_ATTACHMENT, textarget, textureObject, level);
         webContext()->framebufferTexture2D(target, GL_STENCIL_ATTACHMENT, textarget, textureObject, level);
         break;
-    case GL_DEPTH_ATTACHMENT:

[Zhenyao Mo, 2015/06/09 20:29:50]

Why removing these two?

-        webContext()->framebufferTexture2D(target, attachment, textarget, textureObject, level);
-        break;
-    case GL_STENCIL_ATTACHMENT:
-        webContext()->framebufferTexture2D(target, attachment, textarget, textureObject, level);
-        break;
     default:
         webContext()->framebufferTexture2D(target, attachment, textarget, textureObject, level);
     }
-    m_framebufferBinding->setAttachmentForBoundFramebuffer(attachment, textarget, texture, level);
+    framebufferBinding->setAttachmentForBoundFramebuffer(target, attachment, textarget, texture, level);
     applyStencilTest();
 }
 
@@ -3323,7 +3332,9 @@ void WebGLRenderingContextBase::readPixels(GLint x, GLint y, GLsizei width, GLsi
         return;
     }
     const char* reason = "framebuffer incomplete";
-    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), &reason)) {
+    GLenum target = isWebGL2OrHigher() ? GL_READ_FRAMEBUFFER : GL_FRAMEBUFFER;
+    WebGLFramebuffer* readFramebufferBinding = getFramebufferBinding(target);
+    if (readFramebufferBinding && !readFramebufferBinding->onAccess(webContext(), target, &reason)) {
         synthesizeGLError(GL_INVALID_FRAMEBUFFER_OPERATION, "readPixels", reason);
         return;
     }
@@ -3344,14 +3355,14 @@ void WebGLRenderingContextBase::readPixels(GLint x, GLint y, GLsizei width, GLsi
     void* data = pixels->baseAddress();
 
     {
-        ScopedDrawingBufferBinder binder(drawingBuffer(), m_framebufferBinding.get());
+        ScopedDrawingBufferBinder binder(drawingBuffer(), readFramebufferBinding, target);

[Zhenyao Mo, 2015/06/09 20:29:51]

Again, I don't understand why we need the binder here. readPixels should not
change any bindings.

[Zhenyao Mo, 2015/06/09 20:33:40]

Never mind.  Of course we need the binder for multisampled fbos. But I think the
current setup for the binder is incorrect.  See my comments in the binder class.

[yunchao, 2015/06/10 08:21:25]

I think there is no problem.

         webContext()->readPixels(x, y, width, height, format, type, data);
     }
 
 #if OS(MACOSX)
     // FIXME: remove this section when GL driver bug on Mac is fixed, i.e.,
     // when alpha is off, readPixels should set alpha to 255 instead of 0.
-    if (!m_framebufferBinding && !drawingBuffer()->getActualAttributes().alpha) {
+    if (!readFramebufferBinding && !drawingBuffer()->getActualAttributes().alpha) {
         unsigned char* pixels = reinterpret_cast<unsigned char*>(data);
         for (GLsizei iy = 0; iy < height; ++iy) {
             for (GLsizei ix = 0; ix < width; ++ix) {
@@ -4629,7 +4640,7 @@ void WebGLRenderingContextBase::loseContextImpl(WebGLRenderingContextBase::LostC
 
     // Make absolutely sure we do not refer to an already-deleted texture or framebuffer.
     drawingBuffer()->setTexture2DBinding(0);
-    drawingBuffer()->setFramebufferBinding(0);
+    drawingBuffer()->setFramebufferBinding(GL_FRAMEBUFFER, 0);
 
     detachAndRemoveAllObjects();
 
@@ -5732,7 +5743,7 @@ bool WebGLRenderingContextBase::validateDrawArrays(const char* functionName, GLe
     }
 
     const char* reason = "framebuffer incomplete";
-    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), &reason)) {
+    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), GL_FRAMEBUFFER, &reason)) {
         synthesizeGLError(GL_INVALID_FRAMEBUFFER_OPERATION, functionName, reason);
         return false;
     }
@@ -5784,7 +5795,7 @@ bool WebGLRenderingContextBase::validateDrawElements(const char* functionName, G
     }
 
     const char* reason = "framebuffer incomplete";
-    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), &reason)) {
+    if (m_framebufferBinding && !m_framebufferBinding->onAccess(webContext(), GL_FRAMEBUFFER, &reason)) {
         synthesizeGLError(GL_INVALID_FRAMEBUFFER_OPERATION, functionName, reason);
         return false;
     }
@@ -6113,7 +6124,7 @@ void WebGLRenderingContextBase::setFramebuffer(GLenum target, WebGLFramebuffer*
         m_framebufferBinding = buffer;
         applyStencilTest();
     }
-    drawingBuffer()->setFramebufferBinding(objectOrZero(m_framebufferBinding.get()));
+    drawingBuffer()->setFramebufferBinding(target, objectOrZero(getFramebufferBinding(target)));
 
     if (!buffer) {
         // Instead of binding fb 0, bind the drawing buffer.