Closed Bug 1231939 Opened 4 years ago Closed 4 years ago

Avoid shutdown Omx taskqueue in the same taskqueue

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file, 3 obsolete files)

OmxDataDecoder's reference count could be 0 in the Omx TaskQueue, which causes a deadlock in destructor when it tried to shutdown Omx TaskQueue.
Attachment #8697554 - Flags: review?(jyavenard)
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.
Attachment #8697554 - Attachment is obsolete: true
Attachment #8697554 - Flags: review?(jyavenard)
Attachment #8697848 - Flags: review?(jyavenard)
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.
Attachment #8697848 - Flags: review?(jyavenard) → review?(cpearce)
(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.
Flags: needinfo?(ayang)
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.
Attachment #8697848 - Attachment is obsolete: true
Attachment #8697848 - Flags: review?(cpearce)
Flags: needinfo?(ayang)
Attachment #8699275 - Flags: review?(cpearce)
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 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.
Attachment #8699275 - Flags: review?(cpearce) → review+
Blocks: 1224889
(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.
Attachment #8699275 - Attachment is obsolete: true
Attachment #8699433 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f359f98eb89b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.