1094678 - MediaCodecReader need to use the main thread to create SharedThreadPool.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bruce Sun [:brsun]]

Currently MediaCodecReader::CreateTaskQueues() does not use the main thread to call SharedThreadPool::Get(), which would result crashes in debug build.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-1094678.v01.patch

Sync dispatch to main thread.

Comment 2

[Bruce Sun [:brsun]]

Comment on attachment 8539959 [details] [diff] [review]
bug-1094678.v01.patch

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

Looks good to me.

cpearce: would you mind sharing your comments as well?

::: dom/media/omx/MediaCodecReader.cpp
@@ +1291,5 @@
> +{
> +  nsCOMPtr<nsIRunnable> event =
> +    NS_NewRunnableMethod(this, &MediaCodecReader::CreateTaskQueuesTask);
> +
> +  NS_DispatchToMainThread(event, DISPATCH_SYNC);

nit: Most sync events that to be run on the main thread use |NS_DISPATCH_SYNC| instead of using |DISPATCH_SYNC|.

Comment 3

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

Comment on attachment 8539959 [details] [diff] [review]
bug-1094678.v01.patch

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

This is OK, but it would be better to use CreateMediaDecodeTaskQueue() from VideoUtils.h instead.

::: dom/media/omx/MediaCodecReader.cpp
@@ +1291,5 @@
> +{
> +  nsCOMPtr<nsIRunnable> event =
> +    NS_NewRunnableMethod(this, &MediaCodecReader::CreateTaskQueuesTask);
> +
> +  NS_DispatchToMainThread(event, DISPATCH_SYNC);

Yes, you should have used NS_DISPATCH_SYNC.

However, you should replace this with CreateMediaDecodeTaskQueue() from VideoUtils.h instead, rather than re-implementing your own one here. That creates a task queue in the "Media Decode" thread pool, but I think that's ok.

http://dxr.mozilla.org/mozilla-central/source/dom/media/VideoUtils.h#268

Optionally, you could pass the name of the SharedThreadPool to CreateMediaDecodeTaskQueue() if you really don't want to use the "Media Decode" thread pool here, but I don't see why we shouldn't use the "Media Decode" thread pool for everything related to decoding.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: bug-1094678.v02.patch

Call CreateMediaDecodeTaskQueue() to create the TaskQueues.

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85da090730bb

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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