cc: VideoResourceUpdater: don't dismiss the sync_point given by the compositor

cc/resources/video_resource_updater.cc

 // Copyright 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 "cc/resources/video_resource_updater.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/base/util.h"
 #include "cc/output/gl_renderer.h"
 #include "cc/resources/resource_provider.h"
 #include "gpu/GLES2/gl2extchromium.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "media/base/video_frame.h"
 #include "media/blink/skcanvas_video_renderer.h"
 #include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/khronos/GLES2/gl2ext.h"
 #include "ui/gfx/geometry/size_conversions.h"
 
 namespace cc {
 
 namespace {
 
 const ResourceFormat kRGBResourceFormat = RGBA_8888;
 
 class SyncPointClientImpl : public media::VideoFrame::SyncPointClient {
  public:
-  explicit SyncPointClientImpl(gpu::gles2::GLES2Interface* gl) : gl_(gl) {}
+  explicit SyncPointClientImpl(gpu::gles2::GLES2Interface* gl,
+                               uint32 sync_point)
+      : gl_(gl), sync_point_(sync_point) {}
   ~SyncPointClientImpl() override {}
-  uint32 InsertSyncPoint() override { return gl_->InsertSyncPointCHROMIUM(); }
+  uint32 InsertSyncPoint() override { return sync_point_; }
   void WaitSyncPoint(uint32 sync_point) override {
     gl_->WaitSyncPointCHROMIUM(sync_point);
   }
 
  private:
   gpu::gles2::GLES2Interface* gl_;
+  uint32 sync_point_;
 };
 
 }  // namespace
 
 VideoResourceUpdater::PlaneResource::PlaneResource(
     unsigned int resource_id,
     const gfx::Size& resource_size,
     ResourceFormat resource_format,
     gpu::Mailbox mailbox)
     : resource_id(resource_id),
@@ skipping 319 matching lines @@
 void VideoResourceUpdater::ReturnTexture(
     base::WeakPtr<VideoResourceUpdater> updater,
     const scoped_refptr<media::VideoFrame>& video_frame,
     uint32 sync_point,
     bool lost_resource,
     BlockingTaskRunner* main_thread_task_runner) {
   // TODO(dshwang) this case should be forwarded to the decoder as lost
   // resource.
   if (lost_resource || !updater.get())
     return;
   // VideoFrame::UpdateReleaseSyncPoint() creates new sync point using the same

[danakj, 2015/04/14 15:56:27]

What about this comment? Why is it wrong?

[llandwerlin-old, 2015/04/14 16:21:54]

Thanks, I will update the comment.

It's wrong because prior to https://codereview.chromium.org/312803002/ we kept
adding sync points to the media::VideoFrame.

If we want to keep only one sync point, we need to build up a chain of
dependency. We can do this by inserting a WaitSyncPointCHROMIUM op in the
command stream and create a new sync point to be used by the media::VideoFrame,
but since we do not draw with the GL texture from the renderer, we can directly
replace the sync point inside the media::VideoFrame.

   // GL context which created the given |sync_point|, so discard the
   // |sync_point|.
-  SyncPointClientImpl client(updater->context_provider_->ContextGL());
+  SyncPointClientImpl client(updater->context_provider_->ContextGL(),
+                             sync_point);
   video_frame->UpdateReleaseSyncPoint(&client);
 }
 
 VideoFrameExternalResources VideoResourceUpdater::CreateForHardwarePlanes(
     const scoped_refptr<media::VideoFrame>& video_frame) {
   TRACE_EVENT0("cc", "VideoResourceUpdater::CreateForHardwarePlanes");
   media::VideoFrame::Format frame_format = video_frame->format();
 
   DCHECK_EQ(frame_format, media::VideoFrame::NATIVE_TEXTURE);
   if (frame_format != media::VideoFrame::NATIVE_TEXTURE)
@@ skipping 60 matching lines @@
     resource_it->ref_count = 0;
     updater->DeleteResource(resource_it);
     return;
   }
 
   --resource_it->ref_count;
   DCHECK_GE(resource_it->ref_count, 0);
 }
 
 }  // namespace cc