cc: Don't DeleteResource while holding write lock.

When the context is lost and we can't generate a mailbox for a
resource, we delete that resource. However currently we are already
optimistically holding a write lock on the resource.

Instead, only take the write lock once we have generated the mailbox
so we don't hold it when we delete the resource.

Tests:
VideoResourceUpdaterTest.SoftwareFrame
VideoResourceUpdaterTest.LostContextForSoftwareFrame

R=piman@chromium.org
BUG=260361

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@211815 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 103 insertions, 5 deletions

diff --git a/cc/cc_tests.gyp b/cc/cc_tests.gyp
index d4c4586..ff5b40b 100644
--- a/cc/cc_tests.gyp
+++ b/cc/cc_tests.gyp
@@ -60,6 +60,7 @@
       'resources/scoped_resource_unittest.cc',
       'resources/tile_manager_unittest.cc',
       'resources/tile_priority_unittest.cc',
+      'resources/video_resource_updater_unittest.cc',
       'resources/worker_pool_unittest.cc',
       'scheduler/delay_based_time_source_unittest.cc',
       'scheduler/frame_rate_controller_unittest.cc',
diff --git a/cc/resources/video_resource_updater.cc b/cc/resources/video_resource_updater.cc
index 1b60d0b..4239e98 100644
--- a/cc/resources/video_resource_updater.cc
+++ b/cc/resources/video_resource_updater.cc
@@ -197,9 +197,6 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       DCHECK(mailbox.IsZero());
 
       if (!software_compositor) {
-        ResourceProvider::ScopedWriteLockGL lock(
-            resource_provider_, resource_id);
-
         WebKit::WebGraphicsContext3D* context =
             resource_provider_->GraphicsContext3D();
         DCHECK(context);
@@ -209,6 +206,8 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
           resource_provider_->DeleteResource(resource_id);
           resource_id = 0;
         } else {
+          ResourceProvider::ScopedWriteLockGL lock(
+              resource_provider_, resource_id);
           GLC(context, context->bindTexture(GL_TEXTURE_2D, lock.texture_id()));
           GLC(context, context->produceTextureCHROMIUM(GL_TEXTURE_2D,
                                                        mailbox.name));
diff --git a/cc/resources/video_resource_updater.h b/cc/resources/video_resource_updater.h
index 68d7cfd..9be79d1 100644
--- a/cc/resources/video_resource_updater.h
+++ b/cc/resources/video_resource_updater.h
@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "cc/base/cc_export.h"
 #include "cc/resources/texture_mailbox.h"
 #include "ui/gfx/size.h"
 
@@ -22,7 +23,7 @@ class VideoFrame;
 namespace cc {
 class ResourceProvider;
 
-class VideoFrameExternalResources {
+class CC_EXPORT VideoFrameExternalResources {
  public:
   // Specifies what type of data is contained in the mailboxes, as well as how
   // many mailboxes will be present.
@@ -57,7 +58,7 @@ class VideoFrameExternalResources {
 
 // VideoResourceUpdater is by the video system to produce frame content as
 // resources consumable by the compositor.
-class VideoResourceUpdater
+class CC_EXPORT VideoResourceUpdater
     : public base::SupportsWeakPtr<VideoResourceUpdater> {
  public:
   explicit VideoResourceUpdater(ResourceProvider* resource_provider);
diff --git a/cc/resources/video_resource_updater_unittest.cc b/cc/resources/video_resource_updater_unittest.cc
new file mode 100644
index 0000000..562b953
--- /dev/null
+++ b/cc/resources/video_resource_updater_unittest.cc
@@ -0,0 +1,79 @@
+// 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 "base/memory/shared_memory.h"
+#include "cc/resources/resource_provider.h"
+#include "cc/test/fake_output_surface.h"
+#include "cc/test/test_web_graphics_context_3d.h"
+#include "media/base/video_frame.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cc {
+namespace {
+
+class VideoResourceUpdaterTest : public testing::Test {
+ protected:
+  VideoResourceUpdaterTest() {
+    scoped_ptr<TestWebGraphicsContext3D> context3d =
+        TestWebGraphicsContext3D::Create();
+    context3d_ = context3d.get();
+
+    output_surface3d_ = FakeOutputSurface::Create3d(
+        context3d.PassAs<WebKit::WebGraphicsContext3D>());
+    resource_provider3d_ =
+        ResourceProvider::Create(output_surface3d_.get(), 0);
+  }
+
+  scoped_refptr<media::VideoFrame> CreateTestYUVVideoFrame() {
+    gfx::Size size(10, 10);
+    static uint8 y_data[10] = { 0 };
+    static uint8 u_data[5] = { 0 };
+    static uint8 v_data[5] = { 0 };
+
+    return media::VideoFrame::WrapExternalYuvData(
+        media::VideoFrame::YV16,  // format
+        size,  // coded_size
+        gfx::Rect(size),  // visible_rect
+        size,  // natural_size
+        size.width(),  // y_stride
+        size.width() / 2,  // u_stride
+        size.width() / 2,  // v_stride
+        y_data,  // y_data
+        u_data,  // u_data
+        v_data,  // v_data
+        base::TimeDelta(),  // timestamp,
+        base::SharedMemory::NULLHandle(),  // shm_handle
+        base::Closure());  // no_longer_needed_cb
+  }
+
+  TestWebGraphicsContext3D* context3d_;
+  scoped_ptr<FakeOutputSurface> output_surface3d_;
+  scoped_ptr<ResourceProvider> resource_provider3d_;
+};
+
+TEST_F(VideoResourceUpdaterTest, SoftwareFrame) {
+  VideoResourceUpdater updater(resource_provider3d_.get());
+  scoped_refptr<media::VideoFrame> video_frame = CreateTestYUVVideoFrame();
+
+  VideoFrameExternalResources resources =
+      updater.CreateExternalResourcesFromVideoFrame(video_frame);
+  EXPECT_EQ(VideoFrameExternalResources::YUV_RESOURCE, resources.type);
+}
+
+TEST_F(VideoResourceUpdaterTest, LostContextForSoftwareFrame) {
+  VideoResourceUpdater updater(resource_provider3d_.get());
+  scoped_refptr<media::VideoFrame> video_frame = CreateTestYUVVideoFrame();
+
+  // Fail while creating the mailbox for the second YUV plane.
+  context3d_->set_times_gen_mailbox_succeeds(1);
+
+  VideoFrameExternalResources resources =
+      updater.CreateExternalResourcesFromVideoFrame(video_frame);
+  EXPECT_EQ(VideoFrameExternalResources::NONE, resources.type);
+}
+
+}  // namespace
+}  // namespace cc
diff --git a/cc/test/test_web_graphics_context_3d.cc b/cc/test/test_web_graphics_context_3d.cc
index f5ebb6b..1e17f91 100644
--- a/cc/test/test_web_graphics_context_3d.cc
+++ b/cc/test/test_web_graphics_context_3d.cc
@@ -47,6 +47,7 @@ TestWebGraphicsContext3D::TestWebGraphicsContext3D()
       times_make_current_succeeds_(-1),
       times_bind_texture_succeeds_(-1),
       times_end_query_succeeds_(-1),
+      times_gen_mailbox_succeeds_(-1),
       context_lost_(false),
       times_map_image_chromium_succeeds_(-1),
       times_map_buffer_chromium_succeeds_(-1),
@@ -74,6 +75,7 @@ TestWebGraphicsContext3D::TestWebGraphicsContext3D(
       times_make_current_succeeds_(-1),
       times_bind_texture_succeeds_(-1),
       times_end_query_succeeds_(-1),
+      times_gen_mailbox_succeeds_(-1),
       context_lost_(false),
       times_map_image_chromium_succeeds_(-1),
       times_map_buffer_chromium_succeeds_(-1),
@@ -329,6 +331,18 @@ void TestWebGraphicsContext3D::getIntegerv(
 }
 
 void TestWebGraphicsContext3D::genMailboxCHROMIUM(WebKit::WGC3Dbyte* mailbox) {
+  if (times_gen_mailbox_succeeds_ >= 0) {
+    if (!times_gen_mailbox_succeeds_) {
+      loseContextCHROMIUM(GL_GUILTY_CONTEXT_RESET_ARB,
+                          GL_INNOCENT_CONTEXT_RESET_ARB);
+    }
+    --times_gen_mailbox_succeeds_;
+  }
+  if (context_lost_) {
+    memset(mailbox, 0, 64);
+    return;
+  }
+
   static char mailbox_name1 = '1';
   static char mailbox_name2 = '1';
   mailbox[0] = mailbox_name1;
diff --git a/cc/test/test_web_graphics_context_3d.h b/cc/test/test_web_graphics_context_3d.h
index 086ab0f..5952e36 100644
--- a/cc/test/test_web_graphics_context_3d.h
+++ b/cc/test/test_web_graphics_context_3d.h
@@ -158,6 +158,9 @@ class TestWebGraphicsContext3D : public FakeWebGraphicsContext3D {
   void set_times_end_query_succeeds(int times) {
     times_end_query_succeeds_ = times;
   }
+  void set_times_gen_mailbox_succeeds(int times) {
+    times_gen_mailbox_succeeds_ = times;
+  }
 
   // When set, mapImageCHROMIUM and mapBufferCHROMIUM will return NULL after
   // this many times.
@@ -222,6 +225,7 @@ class TestWebGraphicsContext3D : public FakeWebGraphicsContext3D {
   int times_make_current_succeeds_;
   int times_bind_texture_succeeds_;
   int times_end_query_succeeds_;
+  int times_gen_mailbox_succeeds_;
   bool context_lost_;
   int times_map_image_chromium_succeeds_;
   int times_map_buffer_chromium_succeeds_;