cc: Skip generating video release sync token if not needed.

I removed the optimizations in SyncPointClientImpl where it'd skip
generating sync tokens if there was no existing release sync token.
That caused a 5% cpu time regression in the video benchmarks.

I also added some tests to verify that sync tokens are being generated
at the right places.

R=jbauman
BUG=750872

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: If739e3fb6705cf8b0739fc029ed3bd75dc43eabf
Reviewed-on: https://chromium-review.googlesource.com/596424Reviewed-by: John Bauman <jbauman@chromium.org>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491560}

cc/resources/video_resource_updater.cc

@@ -102,21 +102,33 @@ VideoFrameExternalResources::ResourceType ResourceTypeForVideoFrame(
 
 class SyncTokenClientImpl : public media::VideoFrame::SyncTokenClient {
  public:
-  explicit SyncTokenClientImpl(gpu::gles2::GLES2Interface* gl) : gl_(gl) {}
+  SyncTokenClientImpl(gpu::gles2::GLES2Interface* gl, gpu::SyncToken sync_token)
+      : gl_(gl), sync_token_(sync_token) {}
   ~SyncTokenClientImpl() override = default;
 
   void GenerateSyncToken(gpu::SyncToken* sync_token) override {
-    const uint64_t fence_sync = gl_->InsertFenceSyncCHROMIUM();
-    gl_->ShallowFlushCHROMIUM();
-    gl_->GenSyncTokenCHROMIUM(fence_sync, sync_token->GetData());
+    if (sync_token_.HasData()) {
+      *sync_token = sync_token_;
+    } else {
+      const uint64_t fence_sync = gl_->InsertFenceSyncCHROMIUM();
+      gl_->ShallowFlushCHROMIUM();
+      gl_->GenSyncTokenCHROMIUM(fence_sync, sync_token->GetData());
+    }
   }
 
   void WaitSyncToken(const gpu::SyncToken& sync_token) override {
-    gl_->WaitSyncTokenCHROMIUM(sync_token.GetConstData());
+    if (sync_token.HasData()) {
+      gl_->WaitSyncTokenCHROMIUM(sync_token.GetConstData());
+      if (sync_token_.HasData() && sync_token_ != sync_token) {
+        gl_->WaitSyncTokenCHROMIUM(sync_token_.GetConstData());
+        sync_token_.Clear();
+      }
+    }
   }
 
  private:
   gpu::gles2::GLES2Interface* gl_;
+  gpu::SyncToken sync_token_;
   DISALLOW_COPY_AND_ASSIGN(SyncTokenClientImpl);
 };
 
@@ -567,12 +579,9 @@ void VideoResourceUpdater::ReturnTexture(
   // resource.
   if (lost_resource || !updater.get())
     return;
-  // First wait on the sync token returned by the browser so that the release
-  // sync token implicitly depends on it.
-  gpu::gles2::GLES2Interface* gl = updater->context_provider_->ContextGL();
-  gl->WaitSyncTokenCHROMIUM(sync_token.GetConstData());
   // The video frame will insert a wait on the previous release sync token.
-  SyncTokenClientImpl client(gl);
+  SyncTokenClientImpl client(updater->context_provider_->ContextGL(),
+                             sync_token);
   video_frame->UpdateReleaseSyncToken(&client);
 }
 
@@ -614,7 +623,8 @@ void VideoResourceUpdater::CopyPlaneTexture(
       false, false, false);
   gl->DeleteTextures(1, &src_texture_id);
 
-  SyncTokenClientImpl client(gl);
+  // Pass an empty sync token to force generation of a new sync token.
+  SyncTokenClientImpl client(gl, gpu::SyncToken());
   gpu::SyncToken sync_token = video_frame->UpdateReleaseSyncToken(&client);
 
   // Set sync token otherwise resource is assumed to be synchronized.

cc/resources/video_resource_updater_unittest.cc

@@ -174,7 +174,9 @@ class VideoResourceUpdaterTest : public testing::Test {
     return video_frame;
   }
 
-  static void ReleaseMailboxCB(const gpu::SyncToken& sync_token) {}
+  void SetReleaseSyncToken(const gpu::SyncToken& sync_token) {
+    release_sync_token_ = sync_token;
+  }
 
   scoped_refptr<media::VideoFrame> CreateTestHardwareVideoFrame(
       unsigned target) {
@@ -184,19 +186,17 @@ class VideoResourceUpdaterTest : public testing::Test {
     gpu::Mailbox mailbox;
     mailbox.name[0] = 51;
 
-    const gpu::SyncToken sync_token(
-        gpu::CommandBufferNamespace::GPU_IO, 0,
-        gpu::CommandBufferId::FromUnsafeValue(0x123), 7);
     gpu::MailboxHolder mailbox_holders[media::VideoFrame::kMaxPlanes] = {
-        gpu::MailboxHolder(mailbox, sync_token, target)};
+        gpu::MailboxHolder(mailbox, kMailboxSyncToken, target)};
     scoped_refptr<media::VideoFrame> video_frame =
-        media::VideoFrame::WrapNativeTextures(media::PIXEL_FORMAT_ARGB,
-                                              mailbox_holders,
-                                              base::Bind(&ReleaseMailboxCB),
-                                              size,             // coded_size
-                                              gfx::Rect(size),  // visible_rect
-                                              size,             // natural_size
-                                              base::TimeDelta());  // timestamp
+        media::VideoFrame::WrapNativeTextures(
+            media::PIXEL_FORMAT_ARGB, mailbox_holders,
+            base::Bind(&VideoResourceUpdaterTest::SetReleaseSyncToken,
+                       base::Unretained(this)),
+            size,                // coded_size
+            gfx::Rect(size),     // visible_rect
+            size,                // natural_size
+            base::TimeDelta());  // timestamp
     EXPECT_TRUE(video_frame);
     return video_frame;
   }
@@ -218,36 +218,44 @@ class VideoResourceUpdaterTest : public testing::Test {
     const int kDimension = 10;
     gfx::Size size(kDimension, kDimension);
 
-    const gpu::SyncToken sync_token(
-        gpu::CommandBufferNamespace::GPU_IO, 0,
-        gpu::CommandBufferId::FromUnsafeValue(0x123), 7);
     const unsigned target = GL_TEXTURE_RECTANGLE_ARB;
     const int kPlanesNum = 3;
     gpu::MailboxHolder mailbox_holders[media::VideoFrame::kMaxPlanes];
     for (int i = 0; i < kPlanesNum; ++i) {
       gpu::Mailbox mailbox;
       mailbox.name[0] = 50 + 1;
-      mailbox_holders[i] = gpu::MailboxHolder(mailbox, sync_token, target);
+      mailbox_holders[i] =
+          gpu::MailboxHolder(mailbox, kMailboxSyncToken, target);
     }
     scoped_refptr<media::VideoFrame> video_frame =
-        media::VideoFrame::WrapNativeTextures(media::PIXEL_FORMAT_I420,
-                                              mailbox_holders,
-                                              base::Bind(&ReleaseMailboxCB),
-                                              size,             // coded_size
-                                              gfx::Rect(size),  // visible_rect
-                                              size,             // natural_size
-                                              base::TimeDelta());  // timestamp
+        media::VideoFrame::WrapNativeTextures(
+            media::PIXEL_FORMAT_I420, mailbox_holders,
+            base::Bind(&VideoResourceUpdaterTest::SetReleaseSyncToken,
+                       base::Unretained(this)),
+            size,                // coded_size
+            gfx::Rect(size),     // visible_rect
+            size,                // natural_size
+            base::TimeDelta());  // timestamp
     EXPECT_TRUE(video_frame);
     return video_frame;
   }
 
+  static const gpu::SyncToken kMailboxSyncToken;
+
   WebGraphicsContext3DUploadCounter* context3d_;
   scoped_refptr<TestContextProvider> context_provider_;
   std::unique_ptr<SharedBitmapManagerAllocationCounter> shared_bitmap_manager_;
   std::unique_ptr<ResourceProvider> resource_provider3d_;
   std::unique_ptr<ResourceProvider> resource_provider_software_;
+  gpu::SyncToken release_sync_token_;
 };
 
+const gpu::SyncToken VideoResourceUpdaterTest::kMailboxSyncToken =
+    gpu::SyncToken(gpu::CommandBufferNamespace::GPU_IO,
+                   0,
+                   gpu::CommandBufferId::FromUnsafeValue(0x123),
+                   7);
+
 TEST_F(VideoResourceUpdaterTest, SoftwareFrame) {
   bool use_stream_video_draw_quad = false;
   VideoResourceUpdater updater(context_provider_.get(),
@@ -566,5 +574,96 @@ TEST_F(VideoResourceUpdaterTest, CreateForHardwarePlanes_TextureQuad) {
   EXPECT_EQ(0, context3d_->TextureCreationCount());
 }
 
+// Passthrough the sync token returned by the compositor if we don't have an
+// existing release sync token.
+TEST_F(VideoResourceUpdaterTest, PassReleaseSyncToken) {
+  VideoResourceUpdater updater(context_provider_.get(),
+                               resource_provider3d_.get(),
+                               false /* use_stream_video_draw_quad */);
+
+  const gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
+                                  gpu::CommandBufferId::FromUnsafeValue(0x123),
+                                  123);
+
+  {
+    scoped_refptr<media::VideoFrame> video_frame =
+        CreateTestRGBAHardwareVideoFrame();
+
+    VideoFrameExternalResources resources =
+        updater.CreateExternalResourcesFromVideoFrame(video_frame);
+
+    ASSERT_EQ(resources.release_callbacks.size(), 1u);
+    resources.release_callbacks[0].Run(sync_token, false, nullptr);
+  }
+
+  EXPECT_EQ(release_sync_token_, sync_token);
+}
+
+// Generate new sync token because video frame has an existing sync token.
+TEST_F(VideoResourceUpdaterTest, GenerateReleaseSyncToken) {
+  VideoResourceUpdater updater(context_provider_.get(),
+                               resource_provider3d_.get(),
+                               false /* use_stream_video_draw_quad */);
+
+  const gpu::SyncToken sync_token1(gpu::CommandBufferNamespace::GPU_IO, 0,
+                                   gpu::CommandBufferId::FromUnsafeValue(0x123),
+                                   123);
+
+  const gpu::SyncToken sync_token2(gpu::CommandBufferNamespace::GPU_IO, 0,
+                                   gpu::CommandBufferId::FromUnsafeValue(0x234),
+                                   234);
+
+  {
+    scoped_refptr<media::VideoFrame> video_frame =
+        CreateTestRGBAHardwareVideoFrame();
+
+    VideoFrameExternalResources resources =
+        updater.CreateExternalResourcesFromVideoFrame(video_frame);
+
+    ASSERT_EQ(resources.release_callbacks.size(), 1u);
+    resources.release_callbacks[0].Run(sync_token1, false, nullptr);
+    resources.release_callbacks[0].Run(sync_token2, false, nullptr);
+  }
+
+  EXPECT_TRUE(release_sync_token_.HasData());
+  EXPECT_NE(release_sync_token_, sync_token1);
+  EXPECT_NE(release_sync_token_, sync_token2);
+}
+
+// Pass mailbox sync token as is if no GL operations are performed before frame
+// resources are handed off to the compositor.
+TEST_F(VideoResourceUpdaterTest, PassMailboxSyncToken) {
+  VideoResourceUpdater updater(context_provider_.get(),
+                               resource_provider3d_.get(),
+                               false /* use_stream_video_draw_quad */);
+
+  scoped_refptr<media::VideoFrame> video_frame =
+      CreateTestRGBAHardwareVideoFrame();
+
+  VideoFrameExternalResources resources =
+      updater.CreateExternalResourcesFromVideoFrame(video_frame);
+
+  ASSERT_EQ(resources.mailboxes.size(), 1u);
+  EXPECT_TRUE(resources.mailboxes[0].HasSyncToken());
+  EXPECT_EQ(resources.mailboxes[0].sync_token(), kMailboxSyncToken);
+}
+
+// Generate new sync token for compositor when copying the texture.
+TEST_F(VideoResourceUpdaterTest, GenerateSyncTokenOnTextureCopy) {
+  VideoResourceUpdater updater(context_provider_.get(),
+                               resource_provider3d_.get(),
+                               false /* use_stream_video_draw_quad */);
+
+  scoped_refptr<media::VideoFrame> video_frame =
+      CreateTestStreamTextureHardwareVideoFrame(true /* needs_copy */);
+
+  VideoFrameExternalResources resources =
+      updater.CreateExternalResourcesFromVideoFrame(video_frame);
+
+  ASSERT_EQ(resources.mailboxes.size(), 1u);
+  EXPECT_TRUE(resources.mailboxes[0].HasSyncToken());
+  EXPECT_NE(resources.mailboxes[0].sync_token(), kMailboxSyncToken);
+}
+
 }  // namespace
 }  // namespace cc