Revert of Revert of Revert of Remove threading from RendererGpuVideoAcceleratorFactories (https://codereview.chromium.org/135393004/)

Reason for revert:
It turned out the reverting the patchset was effective after all, so I am reverting the revert of the revert.

Original issue's description:
> Revert of Revert of Remove threading from RendererGpuVideoAcceleratorFactories (https://codereview.chromium.org/145103004/)
> 
> Reason for revert:
> Reverting the revert as it did not fix the failures.
> 
> Original issue's description:
> > Revert of Remove threading from RendererGpuVideoAcceleratorFactories (https://codereview.chromium.org/27420004/)
> > 
> > Reason for revert:
> > Sorry, it looks like you broke the Win 7 Tests (dbg)(2) bot. http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%282%29/builds/19785
> > 
> > Original issue's description:
> > > This change removes all the threading considerations from
> > > GpuVideoAcceleratorFactories (and its implementation,
> > > RendererGpuVideoAcceleratorFactories).  Most notably, it removes Abort() and
> > > associated functions and state.  And with the removal of Abort() and friends,
> > > we can also remove its Clone() interface.
> > > 
> > > All of the previously abortable operations on the RGVAF (with the exception of
> > > ReadPixels()) can be made non-abortable, with no functional difference, due to
> > > the way the users of RGVAF function.  These three users are
> > > WebMediaPlayerImpl/GpuVideoDecoder, RTCVideoDecoder, and RTCVideoEncoder, and
> > > they can be made non-abortable because:
> > > 
> > > WebMediaPlayerImpl/GpuVideoDecoder:
> > > * Abort() is called from WebMediaPlayerImpl::Destroy().  It has no effect, as:
> > >   * All the RGVAF entry points are called from the the RGVAF message loop
> > >     from GpuVideoDecoder (except for ReadPixels()), so the Abort() has no
> > >     effect on them.
> > > 
> > > RTCVideoDecoder:
> > > * Abort() is called from RTCVideoDecoder::WillDestroyCurrentMessageLoop() for
> > >   the RGVAF message loop.  It has no effect, as:
> > >   * Amost all the RGVAF entry points are called from the RGVAF message loop
> > >     (except for ReadPixels()), so Abort() has no effect on them.
> > >   * The other exception is CreateVideoDecodeAccelerator(), which is called from
> > >     RTC's main thread.  But as the Abort() is called from
> > >     WillDestroyCurrentMessageLoop() for the RGVAF message loop itself, it is
> > >     guaranteed to occur after any tasks posted to the RGVAF message loop by
> > >     CreateVideoDecodeAccelerator() has completed, and so the Abort() has no
> > >     effect.
> > > 
> > > RTCVideoEncoder:
> > > * Abort() is called from RTCVideoDecoder::Release().  It has no effect, as:
> > >   * All the RGVAF entry points are called from the RGVAF message loop.
> > > 
> > > The only functional difference remaining is that making ReadPixels()
> > > non-abortable.  This is preferable, as as long as a completed video accelerator
> > > texture is available, it should be readable.  We also specify that all calls to
> > > ReadPixels must be made on the RGVAF message loop, like all other entry points,
> > > and leave it up to the users of ReadPixels() to handle thread trampolining if
> > > necessary.
> > > 
> > > BUG=306333
> > > TEST=local build, run on CrOS snow; build, run unittests on desktop Linux
> > > 
> > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247480
> > 
> > TBR=fischman@chromium.org,wuchengli@chromium.org,jamesr@chromium.org,jam@chromium.org,hshi@chromium.org,sheu@chromium.org
> > NOTREECHECKS=true
> > NOTRY=true
> > BUG=306333
> > 
> > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247655
> 
> TBR=fischman@chromium.org,wuchengli@chromium.org,jamesr@chromium.org,jam@chromium.org,hshi@chromium.org,sheu@chromium.org
> NOTREECHECKS=true
> NOTRY=true
> BUG=306333
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247662

TBR=fischman@chromium.org,wuchengli@chromium.org,jamesr@chromium.org,jam@chromium.org,hshi@chromium.org,sheu@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=306333

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

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

5 files changed, 26 insertions, 42 deletions

diff --git a/media/cast/test/fake_gpu_video_accelerator_factories.cc b/media/cast/test/fake_gpu_video_accelerator_factories.cc
index 651ee88c..7e7c092 100644
--- a/media/cast/test/fake_gpu_video_accelerator_factories.cc
+++ b/media/cast/test/fake_gpu_video_accelerator_factories.cc
@@ -58,6 +58,10 @@ FakeGpuVideoAcceleratorFactories::CreateVideoDecodeAccelerator(
       static_cast<media::VideoDecodeAccelerator*>(NULL));
 }
 
+bool FakeGpuVideoAcceleratorFactories::IsAborted() {
+  return false;
+}
+
 }  // namespace test
 }  // namespace cast
 }  // namespace media
diff --git a/media/cast/test/fake_gpu_video_accelerator_factories.h b/media/cast/test/fake_gpu_video_accelerator_factories.h
index aa43530..c4ed825 100644
--- a/media/cast/test/fake_gpu_video_accelerator_factories.h
+++ b/media/cast/test/fake_gpu_video_accelerator_factories.h
@@ -39,13 +39,17 @@ class FakeGpuVideoAcceleratorFactories : public GpuVideoAcceleratorFactories {
   virtual void WaitSyncPoint(uint32 sync_point) OVERRIDE {}
 
   virtual void ReadPixels(uint32 texture_id,
-                          const gfx::Rect& visible_rect,
+                          const gfx::Size& size,
                           const SkBitmap& pixels) OVERRIDE {};
 
   virtual scoped_ptr<VideoDecodeAccelerator> CreateVideoDecodeAccelerator(
       VideoCodecProfile profile,
       VideoDecodeAccelerator::Client* client) OVERRIDE;
 
+  virtual void Abort() OVERRIDE {}
+
+  virtual bool IsAborted() OVERRIDE;
+
  private:
   friend class base::RefCountedThreadSafe<FakeGpuVideoAcceleratorFactories>;
   virtual ~FakeGpuVideoAcceleratorFactories();
diff --git a/media/filters/gpu_video_accelerator_factories.h b/media/filters/gpu_video_accelerator_factories.h
index 88d74b5..b6de58f 100644
--- a/media/filters/gpu_video_accelerator_factories.h
+++ b/media/filters/gpu_video_accelerator_factories.h
@@ -21,12 +21,6 @@ namespace media {
 
 // Helper interface for specifying factories needed to instantiate a hardware
 // video accelerator.
-// Threading model:
-// * The GpuVideoAcceleratorFactories may be constructed on any thread.
-// * The GpuVideoAcceleratorFactories has an associated message loop, which may
-//   be retrieved as |GetMessageLoop()|.
-// * All calls to the Factories after construction must be made on its message
-//   loop.
 class MEDIA_EXPORT GpuVideoAcceleratorFactories
     : public base::RefCountedThreadSafe<GpuVideoAcceleratorFactories> {
  public:
@@ -49,10 +43,9 @@ class MEDIA_EXPORT GpuVideoAcceleratorFactories
 
   virtual void WaitSyncPoint(uint32 sync_point) = 0;
 
-  // Read pixels within |visible_rect| boundaries from a native texture and
-  // store into |pixels| as RGBA.
+  // Read pixels from a native texture and store into |pixels| as RGBA.
   virtual void ReadPixels(uint32 texture_id,
-                          const gfx::Rect& visible_rect,
+                          const gfx::Size& size,
                           const SkBitmap& pixels) = 0;
 
   // Allocate & return a shared memory segment.  Caller is responsible for
@@ -62,6 +55,13 @@ class MEDIA_EXPORT GpuVideoAcceleratorFactories
   // Returns the task runner the video accelerator runs on.
   virtual scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0;
 
+  // Abort any outstanding factory operations and error any future
+  // attempts at factory operations
+  virtual void Abort() = 0;
+
+  // Returns true if Abort() has been called.
+  virtual bool IsAborted() = 0;
+
  protected:
   friend class base::RefCountedThreadSafe<GpuVideoAcceleratorFactories>;
   virtual ~GpuVideoAcceleratorFactories();
diff --git a/media/filters/gpu_video_decoder.cc b/media/filters/gpu_video_decoder.cc
index a0aede6..b96ff43 100644
--- a/media/filters/gpu_video_decoder.cc
+++ b/media/filters/gpu_video_decoder.cc
@@ -12,7 +12,6 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
-#include "base/synchronization/waitable_event.h"
 #include "base/task_runner_util.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/decoder_buffer.h"
@@ -21,7 +20,6 @@
 #include "media/base/pipeline_status.h"
 #include "media/base/video_decoder_config.h"
 #include "media/filters/gpu_video_accelerator_factories.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 
 namespace media {
 
@@ -74,7 +72,7 @@ void GpuVideoDecoder::Reset(const base::Closure& closure)  {
   DVLOG(3) << "Reset()";
   DCheckGpuVideoAcceleratorFactoriesTaskRunnerIsCurrent();
 
-  if (state_ == kDrainingDecoder) {
+  if (state_ == kDrainingDecoder && !factories_->IsAborted()) {
     base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
         &GpuVideoDecoder::Reset, weak_this_, closure));
     // NOTE: if we're deferring Reset() until a Flush() completes, return
@@ -422,33 +420,6 @@ void GpuVideoDecoder::DismissPictureBuffer(int32 id) {
   }
 }
 
-static void ReadPixelsSyncInner(
-    const scoped_refptr<media::GpuVideoAcceleratorFactories>& factories,
-    uint32 texture_id,
-    const gfx::Rect& visible_rect,
-    const SkBitmap& pixels,
-    base::WaitableEvent* event) {
-  factories->ReadPixels(texture_id, visible_rect, pixels);
-  event->Signal();
-}
-
-static void ReadPixelsSync(
-    const scoped_refptr<media::GpuVideoAcceleratorFactories>& factories,
-    uint32 texture_id,
-    const gfx::Rect& visible_rect,
-    const SkBitmap& pixels) {
-  base::WaitableEvent event(true, false);
-  if (!factories->GetTaskRunner()->PostTask(FROM_HERE,
-                                            base::Bind(&ReadPixelsSyncInner,
-                                                       factories,
-                                                       texture_id,
-                                                       visible_rect,
-                                                       pixels,
-                                                       &event)))
-    return;
-  event.Wait();
-}
-
 void GpuVideoDecoder::PictureReady(const media::Picture& picture) {
   DVLOG(3) << "PictureReady()";
   DCheckGpuVideoAcceleratorFactoriesTaskRunnerIsCurrent();
@@ -482,7 +453,10 @@ void GpuVideoDecoder::PictureReady(const media::Picture& picture) {
       visible_rect,
       natural_size,
       timestamp,
-      base::Bind(&ReadPixelsSync, factories_, pb.texture_id(), visible_rect),
+      base::Bind(&GpuVideoAcceleratorFactories::ReadPixels,
+                 factories_,
+                 pb.texture_id(),
+                 gfx::Size(visible_rect.width(), visible_rect.height())),
       base::Closure()));
   CHECK_GT(available_pictures_, 0);
   --available_pictures_;
diff --git a/media/filters/mock_gpu_video_accelerator_factories.h b/media/filters/mock_gpu_video_accelerator_factories.h
index 91a45e8..aee918a 100644
--- a/media/filters/mock_gpu_video_accelerator_factories.h
+++ b/media/filters/mock_gpu_video_accelerator_factories.h
@@ -44,10 +44,12 @@ class MockGpuVideoAcceleratorFactories : public GpuVideoAcceleratorFactories {
   MOCK_METHOD1(WaitSyncPoint, void(uint32 sync_point));
   MOCK_METHOD3(ReadPixels,
                void(uint32 texture_id,
-                    const gfx::Rect& visible_rect,
+                    const gfx::Size& size,
                     const SkBitmap& pixels));
   MOCK_METHOD1(CreateSharedMemory, base::SharedMemory*(size_t size));
   MOCK_METHOD0(GetTaskRunner, scoped_refptr<base::SingleThreadTaskRunner>());
+  MOCK_METHOD0(Abort, void());
+  MOCK_METHOD0(IsAborted, bool());
 
   virtual scoped_ptr<VideoDecodeAccelerator> CreateVideoDecodeAccelerator(
       VideoCodecProfile profile,