Report the SurfaceTexture tranform matrix to CC timely

content/common/gpu/stream_texture_android.cc

 // Copyright (c) 2013 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 "content/common/gpu/stream_texture_android.h"
 
 #include <string.h>
 
 #include "base/bind.h"
 #include "base/strings/stringize_macros.h"
@@ skipping 45 matching lines @@
   }
 
   return false;
 }
 
 StreamTexture::StreamTexture(GpuCommandBufferStub* owner_stub,
                              int32_t route_id,
                              uint32_t texture_id)
     : surface_texture_(gfx::SurfaceTexture::Create(texture_id)),
       size_(0, 0),
-      has_valid_frame_(false),
-      has_pending_frame_(false),
       owner_stub_(owner_stub),
       route_id_(route_id),
       has_listener_(false),
       texture_id_(texture_id),
       framebuffer_(0),
       vertex_shader_(0),
       fragment_shader_(0),
       program_(0),
       vertex_buffer_(0),
       u_xform_location_(-1),
@@ skipping 55 matching lines @@
               ->feature_info()
               ->workarounds()
               .use_virtualized_gl_contexts);
   if (needs_make_current) {
     scoped_make_current.reset(new ui::ScopedMakeCurrent(
         owner_stub_->decoder()->GetGLContext(), owner_stub_->surface()));
   }
   return scoped_make_current;
 }
 
-void StreamTexture::UpdateTexImage() {
-  DCHECK(surface_texture_.get());
-  DCHECK(owner_stub_);
-
-  if (!has_pending_frame_) return;
-
-  scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
-
-  surface_texture_->UpdateTexImage();
-
-  has_valid_frame_ = true;
-  has_pending_frame_ = false;
-
-  float mtx[16];
-  surface_texture_->GetTransformMatrix(mtx);
-
-  if (memcmp(current_matrix_, mtx, sizeof(mtx)) != 0) {
-    memcpy(current_matrix_, mtx, sizeof(mtx));
-
-    if (has_listener_) {
-      GpuStreamTextureMsg_MatrixChanged_Params params;
-      memcpy(&params.m00, mtx, sizeof(mtx));
-      owner_stub_->channel()->Send(
-          new GpuStreamTextureMsg_MatrixChanged(route_id_, params));
-    }
-  }
-
-  if (scoped_make_current.get()) {
-    // UpdateTexImage() implies glBindTexture().
-    // The cmd decoder takes care of restoring the binding for this GLImage as
-    // far as the current context is concerned, but if we temporarily change
-    // it, we have to keep the state intact in *that* context also.
-    const gpu::gles2::ContextState* state =
-        owner_stub_->decoder()->GetContextState();
-    const gpu::gles2::TextureUnit& active_unit =
-        state->texture_units[state->active_texture_unit];
-    glBindTexture(GL_TEXTURE_EXTERNAL_OES,
-                  active_unit.bound_texture_external_oes.get()
-                      ? active_unit.bound_texture_external_oes->service_id()
-                      : 0);
-  }
-}
-
 bool StreamTexture::CopyTexImage(unsigned target) {
   if (target == GL_TEXTURE_2D) {
     glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, size_.width(), size_.height(), 0,
                  GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
     return CopyTexSubImage(GL_TEXTURE_2D, gfx::Point(), gfx::Rect(size_));
   }
 
   if (target != GL_TEXTURE_EXTERNAL_OES)
     return false;
 
   if (!owner_stub_ || !surface_texture_.get())
     return true;
 
   GLint texture_id;
   glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES, &texture_id);
   DCHECK(texture_id);
 
   // The following code only works if we're being asked to copy into
   // |texture_id_|. Copying into a different texture is not supported.
   if (static_cast<unsigned>(texture_id) != texture_id_)
     return false;
 
-  UpdateTexImage();
-
   TextureManager* texture_manager =
       owner_stub_->decoder()->GetContextGroup()->texture_manager();
   gpu::gles2::Texture* texture =
       texture_manager->GetTextureForServiceId(texture_id_);
   if (texture) {
     // By setting image state to UNBOUND instead of COPIED we ensure that
     // CopyTexImage() is called each time the surface texture is used for
     // drawing.
     texture->SetLevelImage(GL_TEXTURE_EXTERNAL_OES, 0, this,
                            gpu::gles2::Texture::UNBOUND);
   }
 
   return true;
 }
 
 void StreamTexture::OnFrameAvailable() {
-  has_pending_frame_ = true;
   if (has_listener_ && owner_stub_) {
     owner_stub_->channel()->Send(
         new GpuStreamTextureMsg_FrameAvailable(route_id_));
   }
+  scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());

[no sievers, 2016/01/27 19:05:20]

This is not safe here (unlike the previous call site). |owner_stub_| might be
NULL.
But you can early-out for that.

[jchen10, 2016/01/28 01:31:39]

Done.

+
+  surface_texture_->UpdateTexImage();

[no sievers, 2016/01/27 19:05:20]

It's not really clear how this fixes it. It just reorders things but the
fundamental race of dequeue (which might change the transform matrix needed for
the current buffer) with pending draw commands in the cmdbuffer remains.

[jchen10, 2016/01/28 01:31:39]

The current problematic work flow is like:
1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
2. StreamTexure sends the event to CC over IPC;
3. CC uses the default or previous transform matrix, which is not fresh, to
caculate RenderQuad for the VideoLayer, and issues the wrong GL commands to GPU
process;
4. GPU process executes the commands from CC, and asks StreamTexture to prepare
the video texture; 
5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
frame;
6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
matrix for the new video frame, and sends it to CC over IPC, which is already
late;

With this CL, the new work flow is:
1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
2. StreamTexure sends the event to CC over IPC;
3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
frame;
4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
matrix for the new video frame, and sends it to CC over IPC;
5. CC uses new transform matrix to caculate RenderQuad for the VideoLayer, and
issues the right GL commands to GPU process;
6. GPU process executes the commands from CC, and asks StreamTexture to prepare
the video texture;
7. StreamTexture doesn't need to bother SurfaceTexture, as the new video frame
was already updated correctly in the step 3;

+
+  float mtx[16];
+  surface_texture_->GetTransformMatrix(mtx);
+
+  if (memcmp(current_matrix_, mtx, sizeof(mtx)) != 0) {
+    memcpy(current_matrix_, mtx, sizeof(mtx));
+
+    if (has_listener_) {
+      GpuStreamTextureMsg_MatrixChanged_Params params;
+      memcpy(&params.m00, mtx, sizeof(mtx));
+      owner_stub_->channel()->Send(
+          new GpuStreamTextureMsg_MatrixChanged(route_id_, params));
+    }
+  }
+  if (scoped_make_current.get()) {
+    // UpdateTexImage() implies glBindTexture().
+    // The cmd decoder takes care of restoring the binding for this GLImage as
+    // far as the current context is concerned, but if we temporarily change
+    // it, we have to keep the state intact in *that* context also.
+    const gpu::gles2::ContextState* state =
+        owner_stub_->decoder()->GetContextState();
+    const gpu::gles2::TextureUnit& active_unit =
+        state->texture_units[state->active_texture_unit];
+    glBindTexture(GL_TEXTURE_EXTERNAL_OES,
+                  active_unit.bound_texture_external_oes.get()
+                      ? active_unit.bound_texture_external_oes->service_id()
+                      : 0);
+  }
 }
 
 gfx::Size StreamTexture::GetSize() {
   return size_;
 }
 
 unsigned StreamTexture::GetInternalFormat() {
   return GL_RGBA;
 }
 
@@ skipping 50 matching lines @@
 
   if (rect != gfx::Rect(size_)) {
     LOG(ERROR) << "Sub-rectangle is not supported";
     return false;
   }
 
   GLint target_texture = 0;
   glGetIntegerv(GL_TEXTURE_BINDING_2D, &target_texture);
   DCHECK(target_texture);
 
-  UpdateTexImage();
-
   if (!framebuffer_) {
     glGenFramebuffersEXT(1, &framebuffer_);
 
     // This vertex shader introduces a y flip before applying the stream
     // texture matrix.  This is required because the stream texture matrix
     // Android provides is intended to  be used in a y-up coordinate system,
     // whereas Chromium expects y-down.
 
     // clang-format off
     const char kVertexShader[] = STRINGIZE(
@@ skipping 64 matching lines @@
   return false;
 }
 
 void StreamTexture::OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
                                  uint64_t process_tracing_id,
                                  const std::string& dump_name) {
   // TODO(ericrk): Add OnMemoryDump for GLImages. crbug.com/514914
 }
 
 }  // namespace content