StreamTextureImages can now override a Texture's service id

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 7b7e36ed74ecf509c3526872d29ea7db3e89e7b1..9a4c9db8b84b2b9dccd86970dc01eb3aed68a170 100644
--- a/gpu/command_buffer/service/texture_manager.cc
+++ b/gpu/command_buffer/service/texture_manager.cc
@@ -1004,6 +1004,27 @@ void Texture::SetLevelInfo(GLenum target,
   }
 }
 
+void Texture::SetStreamTextureServiceId(GLuint service_id) {
+  GLuint new_service_id = service_id ? service_id : owned_service_id_;
+
+  // Take no action if this isn't an OES_EXTERNAL texture.
+  if (target_ && target_ != GL_TEXTURE_EXTERNAL_OES)
+    return;
+
+  if (service_id_ != new_service_id) {
+    service_id_ = new_service_id;
+    IncrementManagerServiceIdGeneration();
+    if (gl::GLContext* context = gl::GLContext::GetCurrent()) {
+      // It would be preferable to pass in the decoder, and ask it to do this
+      // instead.  However, there are several cases, such as TextureDefinition,
+      // that show up without a clear context owner.  So, instead, we use the
+      // current state's state restorer.
+      if (gl::GLStateRestorer* restorer = context->GetGLStateRestorer())
+        restorer->RestoreAllExternalTextureBindingsIfNeeded();
+    }
+  }
+}
+
 void Texture::MarkLevelAsInternalWorkaround(GLenum target, GLint level) {
   DCHECK_GE(level, 0);
   size_t face_index = GLES2Util::GLTargetToFaceIndex(target);
@@ -1477,6 +1498,7 @@ void Texture::SetLevelImageInternal(GLenum target,
                                     gl::GLImage* image,
                                     GLStreamTextureImage* stream_texture_image,
                                     ImageState state) {
+  DCHECK(!stream_texture_image || stream_texture_image == image);
   DCHECK_GE(level, 0);
   size_t face_index = GLES2Util::GLTargetToFaceIndex(target);
   DCHECK_LT(static_cast<size_t>(face_index),
@@ -1487,8 +1509,16 @@ void Texture::SetLevelImageInternal(GLenum target,
       face_infos_[face_index].level_infos[level];
   DCHECK_EQ(info.target, target);
   DCHECK_EQ(info.level, level);
-  info.image = image;
-  info.stream_texture_image = stream_texture_image;
+
+  if (info.image != image) {

[piman, 2016/06/03 20:32:35]

I find this semantic a bit odd and hard to reason about (I don't know whether
SetLevelImage will reset the service_id unless I already know abou tthe current
image)... Is it that there is some code that only wants to change the
ImageState? Would it make sense instead to add a SetLevelImageState method that
only changes the image_state, and have SetLevelImage unconditionally reset the
service_id?

[watk, 2016/06/03 20:40:59]

Yes, and yes I think so. Will do.

+    info.image = image;
+    info.stream_texture_image = stream_texture_image;
+    // If |stream_texture_image| is changing we have to reset the stream texture
+    // service id if one was set. This is important because if the outgoing
+    // image owns the stream texture service id, then |service_id_| may no
+    // longer be valid after the previous image is destructed.
+    SetStreamTextureServiceId(0);
+  }
   info.image_state = state;
 
   UpdateCanRenderCondition();
@@ -1506,8 +1536,10 @@ void Texture::SetLevelImage(GLenum target,
 void Texture::SetLevelStreamTextureImage(GLenum target,
                                          GLint level,
                                          GLStreamTextureImage* image,
-                                         ImageState state) {
+                                         ImageState state,
+                                         GLuint service_id) {
   SetLevelImageInternal(target, level, image, image, state);
+  SetStreamTextureServiceId(service_id);
 }
 
 const Texture::LevelInfo* Texture::GetLevelInfo(GLint target,
@@ -1619,30 +1651,6 @@ bool Texture::CanRenderTo(const FeatureInfo* feature_info, GLint level) const {
   return (color_renderable || depth_renderable || stencil_renderable);
 }
 
-void Texture::SetUnownedServiceId(GLuint service_id) {
-  GLuint new_service_id = service_id;
-
-  // Take no action if this isn't an OES_EXTERNAL texture.
-  if (target_ && target_ != GL_TEXTURE_EXTERNAL_OES)
-    return;
-
-  if (!service_id)
-    new_service_id = owned_service_id_;
-
-  if (service_id_ != new_service_id) {
-    service_id_ = new_service_id;
-    IncrementManagerServiceIdGeneration();
-    if (gl::GLContext* context = gl::GLContext::GetCurrent()) {
-      // It would be preferable to pass in the decoder, and ask it to do this
-      // instead.  However, there are several cases, such as TextureDefinition,
-      // that show up without a clear context owner.  So, instead, we use the
-      // current state's state restorer.
-      if (gl::GLStateRestorer* restorer = context->GetGLStateRestorer())
-        restorer->RestoreAllExternalTextureBindingsIfNeeded();
-    }
-  }
-}
-
 GLenum Texture::GetCompatibilitySwizzleForChannel(GLenum channel) {
   return GetSwizzleForChannel(channel, compatibility_swizzle_);
 }
@@ -2100,9 +2108,11 @@ void TextureManager::SetLevelStreamTextureImage(TextureRef* ref,
                                                 GLenum target,
                                                 GLint level,
                                                 GLStreamTextureImage* image,
-                                                Texture::ImageState state) {
+                                                Texture::ImageState state,
+                                                GLuint service_id) {
   DCHECK(ref);
-  ref->texture()->SetLevelStreamTextureImage(target, level, image, state);
+  ref->texture()->SetLevelStreamTextureImage(target, level, image, state,
+                                             service_id);
 }
 
 size_t TextureManager::GetSignatureSize() const {