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

gpu/command_buffer/service/context_state.cc

Index: gpu/command_buffer/service/context_state.cc
diff --git a/gpu/command_buffer/service/context_state.cc b/gpu/command_buffer/service/context_state.cc
index e8912cfe1625b769dc0047b935c2f098aed31da3..db933aa8e0b01b235f00ec2ffe52dbbb3c9b2b89 100644
--- a/gpu/command_buffer/service/context_state.cc
+++ b/gpu/command_buffer/service/context_state.cc
@@ -26,6 +26,26 @@ void EnableDisable(GLenum pname, bool enable) {
   }
 }
 
+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()
@@ -47,31 +67,51 @@ ContextState::ContextState(FeatureInfo* feature_info, Logger* logger)
 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);
-
-  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);
+  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.

+  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);
   }
 }
 
@@ -101,10 +141,11 @@ 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();
 }
@@ -139,8 +180,8 @@ void ContextState::RestoreGlobalState() const {
   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