[Command buffer] CopyTexSubImage3D: Copying pixels outside of framebuffer should be untouched

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 68b894846bb0ff89ffbdd837869f1f4f0492aaf6..bbec74200dc5ab71e68d892e5d8fb9169d96a2db 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -13264,16 +13264,62 @@ void GLES2DecoderImpl::DoCopyTexSubImage3D(
     return;
   }
 
-  // For 3D textures, we always clear the entire texture. See the code in
-  // TextureManager::ValidateAndDoTexSubImage for TexSubImage3D.
+  // For 3D textures, we always clear the entire texture to 0 if it is not
+  // cleared. See the code in TextureManager::ValidateAndDoTexSubImage
+  // for TexSubImage3D.
   if (!texture->IsLevelCleared(target, level)) {
     texture_manager()->ClearTextureLevel(this, texture_ref, target, level);
     DCHECK(texture->IsLevelCleared(target, level));
+  } else {
+    // For any pixel lying outside the framebuffer, all channels of the
+    // associated texel are initialized to 0.
+    ScopedResolvedFrameBufferBinder binder(this, false, true);
+    gfx::Size size = GetBoundReadFrameBufferSize();
+    GLint copyX = 0;
+    GLint copyY = 0;
+    GLint copyWidth = 0;
+    GLint copyHeight = 0;
+    Clip(x, width, size.width(), &copyX, &copyWidth);
+    Clip(y, height, size.height(), &copyY, &copyHeight);
+    if (x != copyX || y != copyY ||
+        width != copyWidth || height != copyHeight) {
+      uint32_t pixels_size = 0;
+      // TODO(piman): OpenGL ES 3.0.4 Section 3.8.5 specifies how to pick an
+      // effective internal format if internal_format is unsized, which is a
+      // fairly involved logic. For now, just make sure we pick something valid.
+      GLenum format =
+          TextureManager::ExtractFormatFromStorageFormat(internal_format);
+      GLenum type =
+          TextureManager::ExtractTypeFromStorageFormat(internal_format);
+      if (!format || !type) {

[piman, 2016/08/11 18:16:31]

This shouldn't happen, the internal_format was selected previously with either a
TexImage3D, or a TexStorage3D.
For TexStorage3D it's always a sized internal format, so no problem.
For TexImage3D we validated that the internal format, if unsized, is a valid one
that corresponds to a valid format/type pair.

So maybe replace with a DCHECK?

[yunchao, 2016/08/12 15:42:45]

Done.

+        LOCAL_SET_GL_ERROR(
+            GL_INVALID_OPERATION,
+            func_name, "Invalid unsized internal format.");
+        return;
+      }
+
+      if (!GLES2Util::ComputeImageDataSizes(
+          width, height, 1, format, type,
+          state_.unpack_alignment, &pixels_size, NULL, NULL)) {
+        LOCAL_SET_GL_ERROR(GL_OUT_OF_MEMORY, func_name, "dimensions too large");
+        return;
+      }
+
+      if (!EnsureGPUMemoryAvailable(pixels_size)) {

[Ken Russell (switch to Gerrit), 2016/08/11 18:09:44]

I don't think this call is necessary. The texture's storage is already
allocated. This is just a (Copy)TexSubImage3D call, not a TexImage3D call which
would perform allocation.

[piman, 2016/08/11 18:16:31]

I don't think this is needed - we're not allocating new GPU memory, only writing
it.

[yunchao, 2016/08/12 15:42:45]

Done.

+          LOCAL_SET_GL_ERROR(GL_OUT_OF_MEMORY, func_name, "out of memory");
+        return;
+      }
+
+      // some part was clipped so clear the rect.
+      std::unique_ptr<char[]> zero(new char[pixels_size]);
+      memset(zero.get(), 0, pixels_size);
+      glTexSubImage3D(target, level, xoffset, yoffset, zoffset,
+                      width, height, 1, format, type, zero.get());

[Ken Russell (switch to Gerrit), 2016/08/11 18:09:44]

This sort of manual zeroing of the pixels that are outside the framebuffer isn't
done in DoCopyTexSubImage2D, for example -- what's the essential difference
between the 2D and 3D cases? Why doesn't the usual texture clearing code handle
this case?

[piman, 2016/08/11 18:33:16]

The 2D clearing code can deal with tracking partially filled textures, whereas
it doesn't for 3D.

That being said, it is true that we are inconsistent here. In CopyTexSubImage2D
we define the pixels-outside-of-the-FB to be unwritten, whereas here we define
them to be 0.
Following the 2D case would be consistent, and probably a bit simpler overall.
So +1, let's not do this, instead make sure we first clear the level if needed,
and second just clip the copy region.

[yunchao, 2016/08/12 15:42:45]

Yes. The spec is not consistent between CopyTexSubImage2D and CopyTexSubImage3D
if pixels are outside of read framebuffer. This change suppose that the spec is
correct for CopyTexSubImage3D and follows the spec, in which the pixels outside
of read framebuffer should be initialized to 0.

[Ken Russell (switch to Gerrit), 2016/08/12 17:39:15]

Thanks for pointing out that inconsistency; it's an oversight. CopyTexSubImage2D
was changed some time ago to not force the pixels outside the framebuffer to be
cleared, and this change was never propagated to the WebGL 2.0 spec.

https://github.com/KhronosGroup/WebGL/pull/1965 updates the WebGL 2.0 spec.
Let's make this CL implement that new behavior.

[yunchao, 2016/08/12 23:59:43]

Done.

+    }
   }
 
   // TODO(yunchao): Follow-up CLs are necessary. For instance:
   // 1. emulation of unsized formats in core profile
-  // 2. out-of-bounds reading, etc.
 
   glCopyTexSubImage3D(target, level, xoffset, yoffset, zoffset, x, y, width,
                       height);

[piman, 2016/08/11 18:16:31]

We want to adjust xoffset/yoffset/width/height to copyX/Y/Width/Height here if
we have to clip. The pixels read outside the framebuffer are undefined - not
necessarily unwritten to the destination - so even if we clear them above, we
may end up with undefined values overwriting the 0's.

Note that we need to do that independently of whether the texture needed to be
cleared.

[yunchao, 2016/08/12 15:42:45]

Done.