cc: Various code safety improvements in video compositing code.

This CL adds comments and DHCECKs clarifying the thread safety aspects of
VideoFrameProviderClientImpl.

BUG=336733

Review URL: https://codereview.chromium.org/1033563002

Cr-Commit-Position: refs/heads/master@{#322521}

cc/layers/video_frame_provider.h

@@ -6,6 +6,7 @@
 #define CC_LAYERS_VIDEO_FRAME_PROVIDER_H_
 
 #include "base/memory/ref_counted.h"
+#include "cc/base/cc_export.h"
 
 namespace media {
 class VideoFrame;
@@ -18,11 +19,11 @@ namespace cc {
 // PutCurrentFrame() from the compositor thread. If so, the caller is
 // responsible for making sure Client::DidReceiveFrame() and
 // Client::DidUpdateMatrix() are only called from this same thread.
-class VideoFrameProvider {
+class CC_EXPORT VideoFrameProvider {
  public:
   virtual ~VideoFrameProvider() {}
 
-  class Client {
+  class CC_EXPORT Client {
    public:
     // Provider will call this method to tell the client to stop using it.
     // StopUsingProvider() may be called from any thread. The client should

cc/layers/video_frame_provider_client_impl.cc

@@ -13,17 +13,13 @@ namespace cc {
 
 // static
 scoped_refptr<VideoFrameProviderClientImpl>
-    VideoFrameProviderClientImpl::Create(
-        VideoFrameProvider* provider) {
-  return make_scoped_refptr(
-      new VideoFrameProviderClientImpl(provider));
+VideoFrameProviderClientImpl::Create(VideoFrameProvider* provider) {
+  return make_scoped_refptr(new VideoFrameProviderClientImpl(provider));
 }
 
-VideoFrameProviderClientImpl::~VideoFrameProviderClientImpl() {}
-
 VideoFrameProviderClientImpl::VideoFrameProviderClientImpl(
     VideoFrameProvider* provider)
-    : active_video_layer_(nullptr), provider_(provider) {
+    : provider_(provider), active_video_layer_(nullptr), stopped_(false) {
   // This only happens during a commit on the compositor thread while the main
   // thread is blocked. That makes this a thread-safe call to set the video
   // frame provider client that does not require a lock. The same is true of
@@ -39,6 +35,16 @@ VideoFrameProviderClientImpl::VideoFrameProviderClientImpl(
       0.0, 0.0, 0.0, 1.0);
 }
 
+VideoFrameProviderClientImpl::~VideoFrameProviderClientImpl() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(stopped_);
+}
+
+VideoLayerImpl* VideoFrameProviderClientImpl::ActiveVideoLayer() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return active_video_layer_;
+}
+
 void VideoFrameProviderClientImpl::SetActiveVideoLayer(
     VideoLayerImpl* video_layer) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -47,19 +53,19 @@ void VideoFrameProviderClientImpl::SetActiveVideoLayer(
 }
 
 void VideoFrameProviderClientImpl::Stop() {
-  // It's called when the main thread is blocked, so lock isn't needed.
-  if (!provider_)
-    return;
   DCHECK(thread_checker_.CalledOnValidThread());
-  provider_->SetVideoFrameProviderClient(nullptr);
-  provider_ = nullptr;
+  // It's called when the main thread is blocked, so lock isn't needed.
+  if (provider_) {
+    provider_->SetVideoFrameProviderClient(nullptr);
+    provider_ = nullptr;
+  }
+  active_video_layer_ = nullptr;
+  stopped_ = true;
 }
 
-bool VideoFrameProviderClientImpl::Stopped() {
+bool VideoFrameProviderClientImpl::Stopped() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  // |provider_| is changed while the main thread is blocked, and not changed
-  // thereafter, so lock isn't needed.
-  return !provider_;
+  return stopped_;
 }
 
 scoped_refptr<media::VideoFrame>
@@ -85,6 +91,12 @@ void VideoFrameProviderClientImpl::ReleaseLock() {
   provider_lock_.Release();
 }
 
+const gfx::Transform& VideoFrameProviderClientImpl::StreamTextureMatrix()
+    const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return stream_texture_matrix_;
+}
+
 void VideoFrameProviderClientImpl::StopUsingProvider() {
   // Block the provider from shutting down until this client is done
   // using the frame.

cc/layers/video_frame_provider_client_impl.h

@@ -8,6 +8,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
+#include "cc/base/cc_export.h"
 #include "cc/layers/video_frame_provider.h"
 #include "ui/gfx/transform.h"
 
@@ -16,42 +17,53 @@ namespace media { class VideoFrame; }
 namespace cc {
 class VideoLayerImpl;
 
-class VideoFrameProviderClientImpl
+// VideoFrameProviderClientImpl liasons with the VideoFrameProvider and the
+// VideoLayer. It receives updates from the provider and updates the layer as a
+// result. It also allows the layer to access the video frame that the provider
+// has.
+class CC_EXPORT VideoFrameProviderClientImpl
     : public VideoFrameProvider::Client,
       public base::RefCounted<VideoFrameProviderClientImpl> {
  public:
+  // Must be created on the impl thread while the main thread is blocked.
   static scoped_refptr<VideoFrameProviderClientImpl> Create(
       VideoFrameProvider* provider);
 
-  VideoLayerImpl* active_video_layer() { return active_video_layer_; }
+  VideoLayerImpl* ActiveVideoLayer() const;
   void SetActiveVideoLayer(VideoLayerImpl* video_layer);
 
+  bool Stopped() const;
+  // Must be called on the impl thread while the main thread is blocked.
   void Stop();
-  bool Stopped();
 
   scoped_refptr<media::VideoFrame> AcquireLockAndCurrentFrame();
   void PutCurrentFrame(const scoped_refptr<media::VideoFrame>& frame);
   void ReleaseLock();
-  const gfx::Transform& stream_texture_matrix() const {
-    return stream_texture_matrix_;
-  }
 
-  // VideoFrameProvider::Client implementation. These methods are all callable
-  // on any thread.
+  const gfx::Transform& StreamTextureMatrix() const;
+
+  // VideoFrameProvider::Client implementation.
+  // Called on the main thread.
   void StopUsingProvider() override;
+  // Called on the impl thread.
   void DidReceiveFrame() override;
   void DidUpdateMatrix(const float* matrix) override;
 
  private:
-  explicit VideoFrameProviderClientImpl(VideoFrameProvider* provider);
   friend class base::RefCounted<VideoFrameProviderClientImpl>;
+
+  explicit VideoFrameProviderClientImpl(VideoFrameProvider* provider);
   ~VideoFrameProviderClientImpl() override;
 
+  VideoFrameProvider* provider_;
   VideoLayerImpl* active_video_layer_;
+  bool stopped_;
 
-  // Guards the destruction of provider_ and the frame that it provides
+  // Since the provider lives on another thread, it can be destroyed while the
+  // frame controller are accessing its frame. Before being destroyed the
+  // provider calls StopUsingProvider. provider_lock_ blocks StopUsingProvider
+  // from returning until the frame controller is done using the frame.
   base::Lock provider_lock_;
-  VideoFrameProvider* provider_;
   base::ThreadChecker thread_checker_;
 
   gfx::Transform stream_texture_matrix_;

cc/layers/video_layer_impl.cc

@@ -30,18 +30,23 @@ scoped_ptr<VideoLayerImpl> VideoLayerImpl::Create(
     int id,
     VideoFrameProvider* provider,
     media::VideoRotation video_rotation) {
-  scoped_ptr<VideoLayerImpl> layer(
-      new VideoLayerImpl(tree_impl, id, video_rotation));
-  layer->SetProviderClientImpl(VideoFrameProviderClientImpl::Create(provider));
-  DCHECK(tree_impl->proxy()->IsImplThread());
   DCHECK(tree_impl->proxy()->IsMainThreadBlocked());
-  return layer.Pass();
+  DCHECK(tree_impl->proxy()->IsImplThread());
+
+  scoped_refptr<VideoFrameProviderClientImpl> provider_client_impl =
+      VideoFrameProviderClientImpl::Create(provider);
+
+  return make_scoped_ptr(
+      new VideoLayerImpl(tree_impl, id, provider_client_impl, video_rotation));
 }
 
-VideoLayerImpl::VideoLayerImpl(LayerTreeImpl* tree_impl,
-                               int id,
-                               media::VideoRotation video_rotation)
+VideoLayerImpl::VideoLayerImpl(
+    LayerTreeImpl* tree_impl,
+    int id,
+    scoped_refptr<VideoFrameProviderClientImpl> provider_client_impl,
+    media::VideoRotation video_rotation)
     : LayerImpl(tree_impl, id),
+      provider_client_impl_(provider_client_impl),
       frame_(nullptr),
       video_rotation_(video_rotation) {
 }
@@ -61,14 +66,8 @@ VideoLayerImpl::~VideoLayerImpl() {
 
 scoped_ptr<LayerImpl> VideoLayerImpl::CreateLayerImpl(
     LayerTreeImpl* tree_impl) {
-  return make_scoped_ptr(new VideoLayerImpl(tree_impl, id(), video_rotation_));
-}
-
-void VideoLayerImpl::PushPropertiesTo(LayerImpl* layer) {
-  LayerImpl::PushPropertiesTo(layer);
-
-  VideoLayerImpl* other = static_cast<VideoLayerImpl*>(layer);
-  other->SetProviderClientImpl(provider_client_impl_);
+  return make_scoped_ptr(new VideoLayerImpl(
+      tree_impl, id(), provider_client_impl_, video_rotation_));
 }
 
 void VideoLayerImpl::DidBecomeActive() {
@@ -275,12 +274,9 @@ void VideoLayerImpl::AppendQuads(RenderPass* render_pass,
       StreamVideoDrawQuad* stream_video_quad =
           render_pass->CreateAndAppendDrawQuad<StreamVideoDrawQuad>();
       stream_video_quad->SetNew(
-          shared_quad_state,
-          quad_rect,
-          opaque_rect,
-          visible_quad_rect,
+          shared_quad_state, quad_rect, opaque_rect, visible_quad_rect,
           frame_resources_[0],
-          scale * provider_client_impl_->stream_texture_matrix());
+          scale * provider_client_impl_->StreamTextureMatrix());
       break;
     }
     case VideoFrameExternalResources::IO_SURFACE: {
@@ -364,11 +360,6 @@ void VideoLayerImpl::SetNeedsRedraw() {
   layer_tree_impl()->SetNeedsRedraw();
 }
 
-void VideoLayerImpl::SetProviderClientImpl(
-    scoped_refptr<VideoFrameProviderClientImpl> provider_client_impl) {
-  provider_client_impl_ = provider_client_impl;
-}
-
 const char* VideoLayerImpl::LayerTypeAsString() const {
   return "cc::VideoLayerImpl";
 }

cc/layers/video_layer_impl.h

@@ -23,6 +23,8 @@ class VideoFrameProviderClientImpl;
 
 class CC_EXPORT VideoLayerImpl : public LayerImpl {
  public:
+  // Must be called on the impl thread while the main thread is blocked. This is
+  // so that |provider| stays alive while this is being created.
   static scoped_ptr<VideoLayerImpl> Create(LayerTreeImpl* tree_impl,
                                            int id,
                                            VideoFrameProvider* provider,
@@ -31,7 +33,6 @@ class CC_EXPORT VideoLayerImpl : public LayerImpl {
 
   // LayerImpl implementation.
   scoped_ptr<LayerImpl> CreateLayerImpl(LayerTreeImpl* tree_impl) override;
-  void PushPropertiesTo(LayerImpl* layer) override;
   bool WillDraw(DrawMode draw_mode,
                 ResourceProvider* resource_provider) override;
   void AppendQuads(RenderPass* render_pass,
@@ -41,16 +42,14 @@ class CC_EXPORT VideoLayerImpl : public LayerImpl {
   void ReleaseResources() override;
 
   void SetNeedsRedraw();
-
-  void SetProviderClientImpl(
-      scoped_refptr<VideoFrameProviderClientImpl> provider_client_impl);
-
   media::VideoRotation video_rotation() const { return video_rotation_; }
 
  private:
-  VideoLayerImpl(LayerTreeImpl* tree_impl,
-                 int id,
-                 media::VideoRotation video_rotation);
+  VideoLayerImpl(
+      LayerTreeImpl* tree_impl,
+      int id,
+      scoped_refptr<VideoFrameProviderClientImpl> provider_client_impl,
+      media::VideoRotation video_rotation);
 
   const char* LayerTypeAsString() const override;
 

cc/layers/video_layer_impl_unittest.cc

@@ -17,12 +17,22 @@
 namespace cc {
 namespace {
 
+// NOTE: We cannot use DebugScopedSetImplThreadAndMainThreadBlocked in these
+// tests because it gets destroyed before the VideoLayerImpl is destroyed. This
+// causes a DCHECK in VideoLayerImpl's destructor to fail.
+static void DebugSetImplThreadAndMainThreadBlocked(Proxy* proxy) {
+#if DCHECK_IS_ON()
+  proxy->SetCurrentThreadIsImplThread(true);
+  proxy->SetMainThreadBlocked(true);
+#endif
+}
+
 TEST(VideoLayerImplTest, Occlusion) {
   gfx::Size layer_size(1000, 1000);
   gfx::Size viewport_size(1000, 1000);
 
   LayerTestCommon::LayerImplTest impl;
-  DebugScopedSetImplThreadAndMainThreadBlocked thread(impl.proxy());
+  DebugSetImplThreadAndMainThreadBlocked(impl.proxy());
 
   scoped_refptr<media::VideoFrame> video_frame =
       media::VideoFrame::CreateFrame(media::VideoFrame::YV12,
@@ -76,7 +86,7 @@ TEST(VideoLayerImplTest, Occlusion) {
 
 TEST(VideoLayerImplTest, DidBecomeActiveShouldSetActiveVideoLayer) {
   LayerTestCommon::LayerImplTest impl;
-  DebugScopedSetImplThreadAndMainThreadBlocked thread(impl.proxy());
+  DebugSetImplThreadAndMainThreadBlocked(impl.proxy());
 
   FakeVideoFrameProvider provider;
   VideoLayerImpl* video_layer_impl =
@@ -85,10 +95,10 @@ TEST(VideoLayerImplTest, DidBecomeActiveShouldSetActiveVideoLayer) {
   VideoFrameProviderClientImpl* client =
       static_cast<VideoFrameProviderClientImpl*>(provider.client());
   ASSERT_TRUE(client);
-  EXPECT_FALSE(client->active_video_layer());
 
+  EXPECT_FALSE(client->ActiveVideoLayer());
   video_layer_impl->DidBecomeActive();
-  EXPECT_EQ(video_layer_impl, client->active_video_layer());
+  EXPECT_EQ(video_layer_impl, client->ActiveVideoLayer());
 }
 
 TEST(VideoLayerImplTest, Rotated0) {
@@ -96,7 +106,7 @@ TEST(VideoLayerImplTest, Rotated0) {
   gfx::Size viewport_size(1000, 500);
 
   LayerTestCommon::LayerImplTest impl;
-  DebugScopedSetImplThreadAndMainThreadBlocked thread(impl.proxy());
+  DebugSetImplThreadAndMainThreadBlocked(impl.proxy());
 
   scoped_refptr<media::VideoFrame> video_frame =
       media::VideoFrame::CreateFrame(media::VideoFrame::YV12,
@@ -132,7 +142,7 @@ TEST(VideoLayerImplTest, Rotated90) {
   gfx::Size viewport_size(1000, 500);
 
   LayerTestCommon::LayerImplTest impl;
-  DebugScopedSetImplThreadAndMainThreadBlocked thread(impl.proxy());
+  DebugSetImplThreadAndMainThreadBlocked(impl.proxy());
 
   scoped_refptr<media::VideoFrame> video_frame =
       media::VideoFrame::CreateFrame(media::VideoFrame::YV12,
@@ -168,7 +178,7 @@ TEST(VideoLayerImplTest, Rotated180) {
   gfx::Size viewport_size(1000, 500);
 
   LayerTestCommon::LayerImplTest impl;
-  DebugScopedSetImplThreadAndMainThreadBlocked thread(impl.proxy());
+  DebugSetImplThreadAndMainThreadBlocked(impl.proxy());
 
   scoped_refptr<media::VideoFrame> video_frame =
       media::VideoFrame::CreateFrame(media::VideoFrame::YV12,
@@ -204,7 +214,7 @@ TEST(VideoLayerImplTest, Rotated270) {
   gfx::Size viewport_size(1000, 500);
 
   LayerTestCommon::LayerImplTest impl;
-  DebugScopedSetImplThreadAndMainThreadBlocked thread(impl.proxy());
+  DebugSetImplThreadAndMainThreadBlocked(impl.proxy());
 
   scoped_refptr<media::VideoFrame> video_frame =
       media::VideoFrame::CreateFrame(media::VideoFrame::YV12,