During virtual context switches only restore texture units that have changed from the previous cont…

gpu/command_buffer/service/context_state.cc

 // Copyright (c) 2012 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 "gpu/command_buffer/service/context_state.h"
 
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
 #include "gpu/command_buffer/service/buffer_manager.h"
 #include "gpu/command_buffer/service/error_state.h"
 #include "gpu/command_buffer/service/framebuffer_manager.h"
 #include "gpu/command_buffer/service/program_manager.h"
 #include "gpu/command_buffer/service/renderbuffer_manager.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_implementation.h"
 
 namespace gpu {
 namespace gles2 {
 
 namespace {
 
 void EnableDisable(GLenum pname, bool enable) {
   if (enable) {
     glEnable(pname);
   } else {
     glDisable(pname);
   }
 }
 
+GLuint Get2dServiceId(const TextureUnit& unit) {
+  return unit.bound_texture_2d.get()
+      ? unit.bound_texture_2d->service_id() : 0;
+}
+
+GLuint GetCubeServiceId(const TextureUnit& unit) {
+  return unit.bound_texture_cube_map.get()
+      ? unit.bound_texture_cube_map->service_id() : 0;
+}
+
+GLuint GetOesServiceId(const TextureUnit& unit) {
+  return unit.bound_texture_external_oes.get()
+      ? unit.bound_texture_external_oes->service_id() : 0;
+}
+
+GLuint GetArbServiceId(const TextureUnit& unit) {
+  return unit.bound_texture_rectangle_arb.get()
+      ? unit.bound_texture_rectangle_arb->service_id() : 0;
+}
+
 }  // anonymous namespace.
 
 TextureUnit::TextureUnit()
     : bind_target(GL_TEXTURE_2D) {
 }
 
 TextureUnit::~TextureUnit() {
 }
 
 ContextState::ContextState(FeatureInfo* feature_info, Logger* logger)
     : active_texture_unit(0),
       pack_reverse_row_order(false),
       fbo_binding_for_scissor_workaround_dirty_(false),
       feature_info_(feature_info),
       error_state_(ErrorState::Create(logger)) {
   Initialize();
 }
 
 ContextState::~ContextState() {
 }
 
-void ContextState::RestoreTextureUnitBindings(GLuint unit) const {
+void ContextState::RestoreTextureUnitBindings(
+    GLuint unit, const ContextState* prev_state) const {
   DCHECK_LT(unit, texture_units.size());
   const TextureUnit& texture_unit = texture_units[unit];
-  glActiveTexture(GL_TEXTURE0 + unit);
-  GLuint service_id = texture_unit.bound_texture_2d.get()
-                          ? texture_unit.bound_texture_2d->service_id()
-                          : 0;
-  glBindTexture(GL_TEXTURE_2D, service_id);
-  service_id = texture_unit.bound_texture_cube_map.get()
-                   ? texture_unit.bound_texture_cube_map->service_id()
-                   : 0;
-  glBindTexture(GL_TEXTURE_CUBE_MAP, service_id);
+  GLuint service_id_2d = Get2dServiceId(texture_unit);
+  GLuint service_id_cube = GetCubeServiceId(texture_unit);
+  GLuint service_id_oes = GetOesServiceId(texture_unit);
+  GLuint service_id_arb = GetArbServiceId(texture_unit);
 

[no sievers, 2013/12/19 19:35:38]

Can you just put 'if (prev_state)' around the whole block here?

[kaanb, 2013/12/20 13:20:31]

Done.

-  if (feature_info_->feature_flags().oes_egl_image_external) {
-    service_id = texture_unit.bound_texture_external_oes.get()
-                     ? texture_unit.bound_texture_external_oes->service_id()
-                     : 0;
-    glBindTexture(GL_TEXTURE_EXTERNAL_OES, service_id);
+  const TextureUnit* prev_texture_unit =
+      (prev_state) ? &prev_state->texture_units[unit] : NULL;

[no sievers, 2013/12/19 19:35:38]

nit: no need for '(',')'

[kaanb, 2013/12/20 13:20:31]

I removed all ternary conditional operators.

+  GLuint prev_service_id_2d =
+      (prev_texture_unit) ? Get2dServiceId(*prev_texture_unit) : 0;
+  GLuint prev_service_id_cube =
+      (prev_texture_unit) ? GetCubeServiceId(*prev_texture_unit) : 0;
+  GLuint prev_service_id_oes =
+      (prev_texture_unit) ? GetOesServiceId(*prev_texture_unit) : 0;
+  GLuint prev_service_id_arb =
+      (prev_texture_unit) ? GetArbServiceId(*prev_texture_unit) : 0;
+
+  bool binding_2d_changed = prev_service_id_2d != service_id_2d;

[no sievers, 2013/12/19 19:35:38]

There is one bug here:
- context A binds texture 0
- switch to context B
- bind texture N (>0)
- delete context B
- come back to context A and we don't have prev_state:
prev_service_id will be 0 and we won't restore the binding. I will still have N
bound.


You could just rename these to 'bind_texture_2d' etc. and
- initialize them to |true|
- only set them inside the 'if (prev_state)'-block otherwise
- for external/rectangle put the feature flag check from below into that
condition

That should fix it and make it slightly more readable.

[Ken Russell (switch to Gerrit), 2013/12/19 21:39:33]

Doesn't the code below already handle this case? If there's no previous state
then it unconditionally re-binds the textures to all of the binding points.

[no sievers, 2013/12/19 21:44:18]

Actually, you are right, I missed that. But still think it could be simplified
for readability by having less convoluted checks :)

[Ken Russell (switch to Gerrit), 2013/12/19 22:13:16]

Yes, I agree. :) It's a lot of code for a simple side-effect, and more thought
could probably reduce it dramatically.

[kaanb, 2013/12/20 13:20:31]

I simplified the code as suggested by sievers@, I'm adding a unittest for the
case he mentioned anyway and hopefully it is easier to see now that we bind all
targets when prev_state is NULL.

+  bool binding_cube_changed = prev_service_id_cube != service_id_cube;
+  bool binding_oes_changed = prev_service_id_oes != service_id_oes;
+  bool binding_arb_changed = prev_service_id_arb != service_id_arb;
+
+  // Early-out if nothing has changed from the previous state.
+  if (prev_state && !binding_2d_changed && !binding_cube_changed
+      && !binding_oes_changed && !binding_arb_changed) {
+    return;
   }
 
-  if (feature_info_->feature_flags().arb_texture_rectangle) {
-    service_id = texture_unit.bound_texture_rectangle_arb.get()
-                     ? texture_unit.bound_texture_rectangle_arb->service_id()
-                     : 0;
-    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, service_id);
+  glActiveTexture(GL_TEXTURE0 + unit);
+  if (!prev_state || binding_2d_changed) {
+    glBindTexture(GL_TEXTURE_2D, service_id_2d);
+  }
+  if (!prev_state || binding_cube_changed) {
+    glBindTexture(GL_TEXTURE_CUBE_MAP, service_id_cube);
+  }
+  if (feature_info_->feature_flags().oes_egl_image_external
+      && (!prev_state || binding_oes_changed)) {
+    glBindTexture(GL_TEXTURE_EXTERNAL_OES, service_id_oes);
+  }
+  if (feature_info_->feature_flags().arb_texture_rectangle
+      && (!prev_state || binding_arb_changed)) {
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, service_id_arb);
   }
 }
 
 void ContextState::RestoreBufferBindings() const {
   if (vertex_attrib_manager.get()) {
     Buffer* element_array_buffer =
         vertex_attrib_manager->element_array_buffer();
     glBindBuffer(GL_ELEMENT_ARRAY_BUFFER,
         element_array_buffer ? element_array_buffer->service_id() : 0);
   }
   glBindBuffer(GL_ARRAY_BUFFER,
                bound_array_buffer.get() ? bound_array_buffer->service_id() : 0);
 }
 
 void ContextState::RestoreRenderbufferBindings() const {
   // Restore Bindings
   glBindRenderbufferEXT(
       GL_RENDERBUFFER,
       bound_renderbuffer.get() ? bound_renderbuffer->service_id() : 0);
 }
 
 void ContextState::RestoreProgramBindings() const {
   glUseProgram(current_program.get() ? current_program->service_id() : 0);
 }
 
 void ContextState::RestoreActiveTexture() const {
   glActiveTexture(GL_TEXTURE0 + active_texture_unit);
 }
 
-void ContextState::RestoreAllTextureUnitBindings() const {
+void ContextState::RestoreAllTextureUnitBindings(
+    const ContextState* prev_state) const {
   // Restore Texture state.
   for (size_t ii = 0; ii < texture_units.size(); ++ii) {
-    RestoreTextureUnitBindings(ii);
+    RestoreTextureUnitBindings(ii, prev_state);
   }
   RestoreActiveTexture();
 }
 
 void ContextState::RestoreAttribute(GLuint attrib_index) const {
   const VertexAttrib* attrib =
       vertex_attrib_manager->GetVertexAttrib(attrib_index);
   const void* ptr = reinterpret_cast<const void*>(attrib->offset());
   Buffer* buffer = attrib->buffer();
   glBindBuffer(GL_ARRAY_BUFFER, buffer ? buffer->service_id() : 0);
@@ skipping 14 matching lines @@
     }
   }
   glVertexAttrib4fv(attrib_index, attrib_values[attrib_index].v);
 }
 
 void ContextState::RestoreGlobalState() const {
   InitCapabilities();
   InitState();
 }
 
-void ContextState::RestoreState() const {
-  RestoreAllTextureUnitBindings();
+void ContextState::RestoreState(const ContextState* prev_state) const {
+  RestoreAllTextureUnitBindings(prev_state);
 
   // Restore Attrib State
   // TODO: This if should not be needed. RestoreState is getting called
   // before GLES2Decoder::Initialize which is a bug.
   if (vertex_attrib_manager.get()) {
     // TODO(gman): Move this restoration to VertexAttribManager.
     for (size_t attrib = 0; attrib < vertex_attrib_manager->num_attribs();
          ++attrib) {
       RestoreAttribute(attrib);
     }
@@ skipping 11 matching lines @@
 
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.
 #include "gpu/command_buffer/service/context_state_impl_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu