Force CUBE_MAP_POSITIVE_X texture allocation before CopyTexImage2D on Intel Mac

gpu/command_buffer/service/texture_manager.cc

Index: gpu/command_buffer/service/texture_manager.cc
diff --git a/gpu/command_buffer/service/texture_manager.cc b/gpu/command_buffer/service/texture_manager.cc
index 796b2870ed189c9af5c2a68593afaa4d23860b21..117ef50a8212b20fb3cc7757d556791341a572c3 100644
--- a/gpu/command_buffer/service/texture_manager.cc
+++ b/gpu/command_buffer/service/texture_manager.cc
@@ -2402,6 +2402,51 @@ bool TextureManager::ValidateTexImage(
   return true;
 }
 
+void TextureManager::DoCubeMapWorkaround(
+    DecoderTextureState* texture_state,
+    ContextState* state,
+    DecoderFramebufferState* framebuffer_state,
+    TextureRef* texture_ref,
+    const char* function_name,
+    const DoTexImageArguments& args) {
+  std::vector<GLenum> undefined_faces;
+  Texture* texture = texture_ref->texture();
+  if (texture_state->force_cube_complete) {
+    int width = 0;
+    int height = 0;
+    for (unsigned i = 0; i < 6; i++) {
+      GLenum target = static_cast<GLenum>(GL_TEXTURE_CUBE_MAP_POSITIVE_X + i);
+      bool defined = texture->GetLevelSize(
+          target, args.level, &width, &height, nullptr);
+      if (!defined && target != args.target)
+        undefined_faces.push_back(GL_TEXTURE_CUBE_MAP_POSITIVE_X + i);
+    }
+  } else {

[piman, 2016/09/20 20:51:44]

Do we need to restrict this path to if args.target !=
GL_TEXTURE_CUBE_MAP_POSITIVE_X? This is ensured in the ValidateAndDoTexImage
path (which explicitly checks args.target != GL_TEXTURE_CUBE_MAP_POSITIVE_X),
but not in the new copy path.

[qiankun, 2016/09/21 02:38:16]

Add the restriction to WorkaroundCopyTexImageCubeMap() and add back the ASSERT.
GL_TEXTURE_CUBE_MAP_POSITIVE_X doesn't need this workaround.

+    int width = 0;
+    int height = 0;
+    if (!texture->GetLevelSize(GL_TEXTURE_CUBE_MAP_POSITIVE_X, args.level,
+                               &width, &height, nullptr)) {
+      undefined_faces.push_back(GL_TEXTURE_CUBE_MAP_POSITIVE_X);
+    }
+  }
+  if (!memory_type_tracker_->EnsureGPUMemoryAvailable(
+          (undefined_faces.size() + 1) * args.pixels_size)) {
+    ERRORSTATE_SET_GL_ERROR(state->GetErrorState(), GL_OUT_OF_MEMORY,
+                            function_name, "out of memory");
+    return;
+  }
+  DoTexImageArguments new_args = args;
+  std::unique_ptr<char[]> zero(new char[args.pixels_size]);
+  memset(zero.get(), 0, args.pixels_size);
+  for (GLenum face : undefined_faces) {
+    new_args.target = face;
+    new_args.pixels = zero.get();
+    DoTexImage(texture_state, state, framebuffer_state,

[piman, 2016/09/20 20:51:44]

The comment on l.2472 explains that this code only works if the unpack buffer is
not bound (because we're passing a pointer to CPU data, not buffer data), and
indeed it was only called if !feature_info_->IsES3Enabled(), which ensures we
don't have it bound.
In the new copy path there is no such check (and indeed it sounds like it
matters on mac even when ES3 is enabled), so maybe we need to resolve this TODO
first?

[qiankun, 2016/09/21 02:38:16]

I resolved this TODO: reset unpack buffer before this workaround and restore it
after the workaround.

+               function_name, texture_ref, new_args);
+    texture->MarkLevelAsInternalWorkaround(face, args.level);
+  }
+}
+
 void TextureManager::ValidateAndDoTexImage(
     DecoderTextureState* texture_state,
     ContextState* state,
@@ -2425,43 +2470,8 @@ void TextureManager::ValidateAndDoTexImage(
         args.target != GL_TEXTURE_CUBE_MAP_POSITIVE_X));
   if (need_cube_map_workaround && !buffer) {
     // TODO(zmo): The following code does not work with an unpack buffer bound.
-    std::vector<GLenum> undefined_faces;
-    if (texture_state->force_cube_complete) {
-      int width = 0;
-      int height = 0;
-      for (unsigned i = 0; i < 6; i++) {
-        GLenum target = static_cast<GLenum>(GL_TEXTURE_CUBE_MAP_POSITIVE_X + i);
-        bool defined = texture->GetLevelSize(
-            target, args.level, &width, &height, nullptr);
-        if (!defined && target != args.target)
-          undefined_faces.push_back(GL_TEXTURE_CUBE_MAP_POSITIVE_X + i);
-      }
-    } else {
-      DCHECK(texture_state->force_cube_map_positive_x_allocation &&
-             args.target != GL_TEXTURE_CUBE_MAP_POSITIVE_X);
-      int width = 0;
-      int height = 0;
-      if (!texture->GetLevelSize(GL_TEXTURE_CUBE_MAP_POSITIVE_X, args.level,
-                                 &width, &height, nullptr)) {
-        undefined_faces.push_back(GL_TEXTURE_CUBE_MAP_POSITIVE_X);
-      }
-    }
-    if (!memory_type_tracker_->EnsureGPUMemoryAvailable(
-            (undefined_faces.size() + 1) * args.pixels_size)) {
-      ERRORSTATE_SET_GL_ERROR(state->GetErrorState(), GL_OUT_OF_MEMORY,
-                              function_name, "out of memory");
-      return;
-    }
-    DoTexImageArguments new_args = args;
-    std::unique_ptr<char[]> zero(new char[args.pixels_size]);
-    memset(zero.get(), 0, args.pixels_size);
-    for (GLenum face : undefined_faces) {
-      new_args.target = face;
-      new_args.pixels = zero.get();
-      DoTexImage(texture_state, state, framebuffer_state,
-                 function_name, texture_ref, new_args);
-      texture->MarkLevelAsInternalWorkaround(face, args.level);
-    }
+    DoCubeMapWorkaround(texture_state, state, framebuffer_state,
+                        texture_ref, function_name, args);
   }
 
   if (texture_state->unpack_overlapping_rows_separately_unpack_buffer &&