1231939 - Avoid shutdown Omx taskqueue in the same taskqueue

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Alfredo Yang (:alfredo)]

OmxDataDecoder's reference count could be 0 in the Omx TaskQueue, which causes a deadlock in destructor when it tried to shutdown Omx TaskQueue.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: shutdown_taskqueue_at_other_taskqueue

Comment 2

[Alfredo Yang (:alfredo)]

Comment on attachment 8697554 [details] [diff] [review]
shutdown_taskqueue_at_other_taskqueue

Unfortunately, it causes another problem. The DoAsyncShutdown() could be running when reader taskqueue is shutdown. And the dispatch to reader TaskQueue is failed.

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: make_shutdown_sync_in_reader_taskqueue

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8697848 [details] [diff] [review]
make_shutdown_sync_in_reader_taskqueue

Review of attachment 8697848 [details] [diff] [review]:
-----------------------------------------------------------------

this looks messy to me and I don't understand why you have to wait for one task to complete before shutting down the other. when you could simply not dispatch if the other task queue has been shut down and be off with it

Passing review to cpearce.

::: dom/media/platforms/omx/OmxDataDecoder.cpp
@@ +220,5 @@
> +    lock.Wait();
> +  }
> +
> +  mOmxTaskQueue->BeginShutdown();
> +  mOmxTaskQueue->AwaitShutdownAndIdle();

do you need to wait here ?

@@ +423,5 @@
>  {
>    MOZ_ASSERT(mOmxTaskQueue->IsCurrentThreadIn());
>    MOZ_ASSERT(mOmxState == OMX_StateExecuting);
>  
>    // During the port setting changed, it is forbided to do any buffer operations.

aside: it is forbidden.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8697848 [details] [diff] [review]
> make_shutdown_sync_in_reader_taskqueue
> 

Patch does not apply cleanly. Alfredo, can you rebase this so it applies? Or tell me the patches/bugs that must be applied first? Thanks.

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: make_shutdown_sync_in_reader_taskqueue

Hi Chris,
Please apply patch at bug 1231353 first and the this patch.
The PDM shutdown() should be block but I implement it as async at first. It cause a runnable in Omx TaskQueue is not finished but the decoder reference is gone.

Comment 7

[Jean-Yves Avenard [:jya]]

Can't you simply keep a strong reference to the decoder instead? and change the reference held by the PDM to be a weak ptr instead seeing that the OMX task queue will always be last to be shutdown

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8699275 [details] [diff] [review]
make_shutdown_sync_in_reader_taskqueue

Review of attachment 8699275 [details] [diff] [review]:
-----------------------------------------------------------------

Someday we should make MediaDataDecoder::Shutdown() return a MozPromise. Then this code would be much cleaner.

r=cpearce

::: dom/media/platforms/omx/OmxDataDecoder.cpp
@@ +218,5 @@
> +  MonitorAutoLock lock(mMonitor);
> +  while (mShuttingDown) {
> +    lock.Wait();
> +  }
> +

{
  MonitorAutoLock lock(mMonitor);
  while (mShuttingDown) {
    lock.Wait();
  }
}

i.e. put a scope around the lock, so that you don't end up holding the lock when you call AwaitShutdownAndIdle(), which blocks the thread.

While I can't see how the current code could deadlock without this, it would be easy to change the code to add a deadlock in future. So putting the extra scope here makes the code more robust.

Comment 9

[Alfredo Yang (:alfredo)]

(In reply to Chris Pearce (:cpearce) from comment #8)
> 
> i.e. put a scope around the lock, so that you don't end up holding the lock
> when you call AwaitShutdownAndIdle(), which blocks the thread.
> 
> While I can't see how the current code could deadlock without this, it would
> be easy to change the code to add a deadlock in future. So putting the extra
> scope here makes the code more robust.

Thanks for comment. I'll fix it.

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: make_shutdown_sync_in_reader_taskqueue

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f359f98eb89b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f359f98eb89b