From 15775ce75dbfff85a03c4f9dc5bcfff3b155be4e Mon Sep 17 00:00:00 2001 From: "ricea@chromium.org" Date: Wed, 29 Jan 2014 11:12:54 +0000 Subject: 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 --- .../test/fake_gpu_video_accelerator_factories.cc | 4 +++ .../test/fake_gpu_video_accelerator_factories.h | 6 +++- media/filters/gpu_video_accelerator_factories.h | 18 +++++------ media/filters/gpu_video_decoder.cc | 36 +++------------------- .../filters/mock_gpu_video_accelerator_factories.h | 4 ++- 5 files changed, 26 insertions(+), 42 deletions(-) (limited to 'media') 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(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 CreateVideoDecodeAccelerator( VideoCodecProfile profile, VideoDecodeAccelerator::Client* client) OVERRIDE; + virtual void Abort() OVERRIDE {} + + virtual bool IsAborted() OVERRIDE; + private: friend class base::RefCountedThreadSafe; 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 { 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 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; 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& 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& 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()); + MOCK_METHOD0(Abort, void()); + MOCK_METHOD0(IsAborted, bool()); virtual scoped_ptr CreateVideoDecodeAccelerator( VideoCodecProfile profile, -- cgit v1.1