Command buffer: feedback loop detection for CopyTexSubImage3D

gpu/command_buffer/service/gles2_cmd_decoder.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index eaa02c8514c9eed3f9089b8c000c886527ed09a5..f5b164c3bf3d2fc2c998c5ca47dc5cd6b1a7a547 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -1353,8 +1353,11 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   static GLint GetColorEncodingFromInternalFormat(GLenum internalformat);
 
   // Check that the currently bound read framebuffer's color image
-  // isn't the target texture of the glCopyTex{Sub}Image2D.
-  bool FormsTextureCopyingFeedbackLoop(TextureRef* texture, GLint level);
+  // isn't the target texture of the glCopyTex{Sub}Image{2D|3D}.
+  bool FormsTextureCopyingFeedbackLoop(
+      TextureRef* texture,
+      GLint level,
+      GLint layer);
 
   // Check if a framebuffer meets our requirements.
   // Generates |gl_error| if the framebuffer is incomplete.
@@ -4242,8 +4245,11 @@ bool GLES2DecoderImpl::CheckBoundDrawFramebufferValid(const char* func_name) {
 
 bool GLES2DecoderImpl::CheckBoundReadFramebufferValid(
     const char* func_name, GLenum gl_error) {
-  GLenum target = features().chromium_framebuffer_multisample ?
-      GL_READ_FRAMEBUFFER : GL_FRAMEBUFFER;
+  GLenum target = GL_FRAMEBUFFER;
+  if (feature_info_->IsWebGL2OrES3Context() ||

[qiankun, 2016/07/28 13:59:31]

Can you explain why webgl2 or es3 are different here? 

[Zhenyao Mo, 2016/07/28 14:51:37]

I don't think this is necessary.  chromium_framebuffer_multisample has to be
true if it's WebGL2 or ES3.

[yunchao, 2016/07/28 15:53:05]

Got it.

[yunchao, 2016/07/28 15:53:06]

For read buffer, we should get info from GL_READ_FRAMEBUFFER for webgl 2 or es3,
instead of GL_FRAMEBUFFER(Which is identical to GL_DRAW_FRAMEBUFFER). 
However, as Zhenyao pointed out, the chromium_framebuffer_multisample should be
true for webgl2 or es3 context. 

+      features().chromium_framebuffer_multisample) {
+    target = GL_READ_FRAMEBUFFER;
+  }
   Framebuffer* framebuffer = GetFramebufferInfoForTarget(target);
   bool valid = CheckFramebufferValid(
       framebuffer, target, gl_error, func_name);
@@ -4287,17 +4293,24 @@ GLint GLES2DecoderImpl::GetColorEncodingFromInternalFormat(
 }
 
 bool GLES2DecoderImpl::FormsTextureCopyingFeedbackLoop(
-    TextureRef* texture, GLint level) {
-  Framebuffer* framebuffer = features().chromium_framebuffer_multisample ?
-      framebuffer_state_.bound_read_framebuffer.get() :
-      framebuffer_state_.bound_draw_framebuffer.get();
+    TextureRef* texture, GLint level, GLint layer) {
+  bool is_webgl2_or_es3 = feature_info_->IsWebGL2OrES3Context();
+  Framebuffer* framebuffer = NULL;

[qiankun, 2016/07/28 13:59:31]

Use nullptr.

+  if (is_webgl2_or_es3) {
+    framebuffer = GetFramebufferInfoForTarget(GL_READ_FRAMEBUFFER_EXT);

[Zhenyao Mo, 2016/07/28 14:51:37]

I don't think it's necessary.  is_webgl2_or_es3 is always true if
features().chromium_framebuffer_multisample is true. The original code is
enough.

[yunchao, 2016/07/28 15:53:06]

Done.

+  } else {
+    framebuffer = features().chromium_framebuffer_multisample ?
+        framebuffer_state_.bound_read_framebuffer.get() :
+        framebuffer_state_.bound_draw_framebuffer.get();
+  }
   if (!framebuffer)
     return false;
-  const Framebuffer::Attachment* attachment = framebuffer->GetAttachment(
-      GL_COLOR_ATTACHMENT0);
+  const Framebuffer::Attachment* attachment = is_webgl2_or_es3 ?

[Zhenyao Mo, 2016/07/28 14:51:37]

This is unnecessary.  In ES2 READ_BUFFER is just COLOR_ATTACHMENT0.

[yunchao, 2016/07/28 15:04:04]

Why here is not necessary, Zhenyao? I don't understand. What if we set read
buffer by readBuffer() to other attachment instead of COLOR_ATTACHMENT0? 
You can also see the code GLES2DecoderImpl::DoReadBuffer() ->
FramebufferManager::set_read_buffer(). And
Framebuffer::GetReadBufferAttachment() will get the correct read attachment when
binding to a fbo.

Well, I know that in ES2 or WebGL 1, the read buffer is definitely
COLOR_ATTACHMENT0. But you know, for WebGL 2 or ES3, we have readBuffer() API to
change the read buffer attachment. 

[yunchao, 2016/07/28 15:06:05]

Sorry, I got what you mean. We can only use
framebuffer->GetReadBufferAttachment() to cover all situations. 

[Zhenyao Mo, 2016/07/28 15:06:38]

What I mean is you can always call framebuffer->GetReadBufferAttachment().

[yunchao, 2016/07/28 15:53:06]

Yeah.

[yunchao, 2016/07/28 15:53:06]

Yeah.

+      framebuffer->GetReadBufferAttachment() :
+      framebuffer->GetAttachment(GL_COLOR_ATTACHMENT0);
   if (!attachment)
     return false;
-  return attachment->FormsFeedbackLoop(texture, level);
+  return attachment->FormsFeedbackLoop(texture, level, layer);
 }
 
 gfx::Size GLES2DecoderImpl::GetBoundReadFrameBufferSize() {
@@ -13012,7 +13025,7 @@ void GLES2DecoderImpl::DoCopyTexImage2D(
     return;
   }
 
-  if (FormsTextureCopyingFeedbackLoop(texture_ref, level)) {
+  if (FormsTextureCopyingFeedbackLoop(texture_ref, level, 0)) {
     LOCAL_SET_GL_ERROR(
         GL_INVALID_OPERATION,
         func_name, "source and destination textures are the same");
@@ -13186,7 +13199,7 @@ void GLES2DecoderImpl::DoCopyTexSubImage2D(
     return;
   }
 
-  if (FormsTextureCopyingFeedbackLoop(texture_ref, level)) {
+  if (FormsTextureCopyingFeedbackLoop(texture_ref, level, 0)) {
     LOCAL_SET_GL_ERROR(
         GL_INVALID_OPERATION,
         func_name, "source and destination textures are the same");
@@ -13293,9 +13306,17 @@ void GLES2DecoderImpl::DoCopyTexSubImage3D(
     return;
   }
 
-  // TODO(yunchao): Follow-up CLs are necessary. For instance, feedback loop
-  // detection, emulation of unsized formats in core profile, clear the 3d
-  // textures if it is uncleared, out-of-bounds reading, etc.
+  if (FormsTextureCopyingFeedbackLoop(texture_ref, level, zoffset)) {
+    LOCAL_SET_GL_ERROR(
+        GL_INVALID_OPERATION,
+        func_name, "source and destination textures are the same");
+    return;
+  }
+
+  // TODO(yunchao): Follow-up CLs are necessary. For instance:
+  // 1. emulation of unsized formats in core profile
+  // 2. clear the 3d textures if it is uncleared.
+  // 3. out-of-bounds reading, etc.
 
   glCopyTexSubImage3D(target, level, xoffset, yoffset, zoffset, x, y, width,
                       height);