Skip to content

Commit

Permalink
Fix a Chrome renderer hang which would occur in the media stack while…
Browse files Browse the repository at this point in the history 
… running WebGL tests.

This was a regression introduced by my fix for bug https://code.google.com/p/chromium/issues/detail?id=179551
The hang occurs because this change introduced a common event object which is used for indicating when
asynchronous tasks posted by the RendererGpuVideoDecoderFactories complete. This breaks down for the
AsyncCreateSharedMemory task which is posted to the renderer main thread causing a hang in the following case:-
1. The main thread initiates an async task like AsyncReadPixels.
2. CreateSharedMemory is called. This waits for the AsyncCreateSharedMemory task to complete.
3. AsyncReadPixels gets called. It sets the async_waiter_ event.
4. AsyncCreateSharedMemory gets called. It sets the async_waiter_ event.
5. The main thread wakes up. The async_waiter_ auto reset event gets reset.

The CreateSharedMemory function in step 2 waits forever for the task to finish.

Fix is to add another event object render_thread_async_waiter_ to the RendererGpuVideoDecoderFactories class.
This is used to signal completion of tasks posted to the renderer thread.

Fixes bug https://code.google.com/p/chromium/issues/detail?id=233673

I verified that bug 179551 does not recur with this change.

BUG=233673
R=fischman

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@195944 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
ananta@chromium.org committed Apr 23, 2013
1 parent 26b81db commit ed2db2a
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 15 deletions.
31 changes: 18 additions & 13 deletions content/renderer/media/renderer_gpu_video_decoder_factories.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ RendererGpuVideoDecoderFactories::RendererGpuVideoDecoderFactories(
: message_loop_(message_loop),
gpu_channel_host_(gpu_channel_host),
aborted_waiter_(true, false),
async_waiter_(false, false) {
compositor_loop_async_waiter_(false, false),
render_thread_async_waiter_(false, false) {
if (message_loop_->BelongsToCurrentThread()) {
AsyncGetContext(context);
return;
Expand All @@ -42,7 +43,7 @@ RendererGpuVideoDecoderFactories::RendererGpuVideoDecoderFactories(
// which can only happen after this function returns, so our PostTask will
// run first.
context));
async_waiter_.Wait();
compositor_loop_async_waiter_.Wait();
}

void RendererGpuVideoDecoderFactories::AsyncGetContext(
Expand All @@ -55,7 +56,7 @@ void RendererGpuVideoDecoderFactories::AsyncGetContext(
context_->insertEventMarkerEXT("GpuVDAContext3D");
}
}
async_waiter_.Signal();
compositor_loop_async_waiter_.Signal();
}

media::VideoDecodeAccelerator*
Expand All @@ -69,7 +70,8 @@ RendererGpuVideoDecoderFactories::CreateVideoDecodeAccelerator(
&RendererGpuVideoDecoderFactories::AsyncCreateVideoDecodeAccelerator,
this, profile, client));

base::WaitableEvent* objects[] = {&aborted_waiter_, &async_waiter_};
base::WaitableEvent* objects[] = {&aborted_waiter_,
&compositor_loop_async_waiter_};
if (base::WaitableEvent::WaitMany(objects, arraysize(objects)) == 0) {
// If we are aborting and the VDA is created by the
// AsyncCreateVideoDecodeAccelerator() function later we need to ensure
Expand All @@ -92,7 +94,7 @@ void RendererGpuVideoDecoderFactories::AsyncCreateVideoDecodeAccelerator(
context_->GetCommandBufferProxy()->GetRouteID(),
profile, client));
}
async_waiter_.Signal();
compositor_loop_async_waiter_.Signal();
}

bool RendererGpuVideoDecoderFactories::CreateTextures(
Expand All @@ -104,7 +106,8 @@ bool RendererGpuVideoDecoderFactories::CreateTextures(
&RendererGpuVideoDecoderFactories::AsyncCreateTextures, this,
count, size, texture_target));

base::WaitableEvent* objects[] = {&aborted_waiter_, &async_waiter_};
base::WaitableEvent* objects[] = {&aborted_waiter_,
&compositor_loop_async_waiter_};
if (base::WaitableEvent::WaitMany(objects, arraysize(objects)) == 0)
return false;
texture_ids->swap(created_textures_);
Expand All @@ -117,7 +120,7 @@ void RendererGpuVideoDecoderFactories::AsyncCreateTextures(
DCHECK(texture_target);

if (!context_) {
async_waiter_.Signal();
compositor_loop_async_waiter_.Signal();
return;
}
gpu::gles2::GLES2Implementation* gles2 = context_->GetImplementation();
Expand All @@ -141,7 +144,7 @@ void RendererGpuVideoDecoderFactories::AsyncCreateTextures(
// reused, this should not be unacceptably expensive.
gles2->Flush();
DCHECK_EQ(gles2->GetError(), static_cast<GLenum>(GL_NO_ERROR));
async_waiter_.Signal();
compositor_loop_async_waiter_.Signal();
}

void RendererGpuVideoDecoderFactories::DeleteTexture(uint32 texture_id) {
Expand Down Expand Up @@ -173,7 +176,8 @@ void RendererGpuVideoDecoderFactories::ReadPixels(
message_loop_->PostTask(FROM_HERE, base::Bind(
&RendererGpuVideoDecoderFactories::AsyncReadPixels, this,
texture_id, texture_target, size));
base::WaitableEvent* objects[] = {&aborted_waiter_, &async_waiter_};
base::WaitableEvent* objects[] = {&aborted_waiter_,
&compositor_loop_async_waiter_};
if (base::WaitableEvent::WaitMany(objects, arraysize(objects)) == 0)
return;
} else {
Expand All @@ -186,7 +190,7 @@ void RendererGpuVideoDecoderFactories::AsyncReadPixels(
uint32 texture_id, uint32 texture_target, const gfx::Size& size) {
DCHECK(message_loop_->BelongsToCurrentThread());
if (!context_) {
async_waiter_.Signal();
compositor_loop_async_waiter_.Signal();
return;
}

Expand All @@ -213,7 +217,7 @@ void RendererGpuVideoDecoderFactories::AsyncReadPixels(
gles2->DeleteFramebuffers(1, &fb);
gles2->DeleteTextures(1, &tmp_texture);
DCHECK_EQ(gles2->GetError(), static_cast<GLenum>(GL_NO_ERROR));
async_waiter_.Signal();
compositor_loop_async_waiter_.Signal();
}

base::SharedMemory* RendererGpuVideoDecoderFactories::CreateSharedMemory(
Expand All @@ -224,7 +228,8 @@ base::SharedMemory* RendererGpuVideoDecoderFactories::CreateSharedMemory(
&RendererGpuVideoDecoderFactories::AsyncCreateSharedMemory, this,
size));

base::WaitableEvent* objects[] = {&aborted_waiter_, &async_waiter_};
base::WaitableEvent* objects[] = {&aborted_waiter_,
&render_thread_async_waiter_};
if (base::WaitableEvent::WaitMany(objects, arraysize(objects)) == 0)
return NULL;
return shared_memory_segment_.release();
Expand All @@ -236,7 +241,7 @@ void RendererGpuVideoDecoderFactories::AsyncCreateSharedMemory(size_t size) {

shared_memory_segment_.reset(
ChildThread::current()->AllocateSharedMemory(size));
async_waiter_.Signal();
render_thread_async_waiter_.Signal();
}

scoped_refptr<base::MessageLoopProxy>
Expand Down
9 changes: 7 additions & 2 deletions content/renderer/media/renderer_gpu_video_decoder_factories.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,14 @@ class CONTENT_EXPORT RendererGpuVideoDecoderFactories
// This event is signaled if we have been asked to Abort().
base::WaitableEvent aborted_waiter_;

// This event is signaled by asynchronous tasks to indicate their completion.
// This event is signaled by asynchronous tasks posted to the compositor
// message loop to indicate their completion.
// e.g. AsyncCreateVideoDecodeAccelerator()/AsyncCreateTextures() etc.
base::WaitableEvent async_waiter_;
base::WaitableEvent compositor_loop_async_waiter_;

// This event is signaled by asynchronous tasks posted to the renderer thread
// message loop to indicate their completion. e.g. AsyncCreateSharedMemory.
base::WaitableEvent render_thread_async_waiter_;

// The vda returned by the CreateVideoAcclelerator function.
scoped_ptr<media::VideoDecodeAccelerator> vda_;
Expand Down

0 comments on commit ed2db2a

Please sign in to comment.