Closed Bug 1094678 Opened 5 years ago Closed 5 years ago

MediaCodecReader need to use the main thread to create SharedThreadPool.

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: brsun, Assigned: bechen)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently MediaCodecReader::CreateTaskQueues() does not use the main thread to call SharedThreadPool::Get(), which would result crashes in debug build.
Assignee: nobody → bechen
Attached patch bug-1094678.v01.patch (obsolete) — Splinter Review
Sync dispatch to main thread.
Attachment #8539959 - Flags: review?(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|.
Attachment #8539959 - Flags: review?(brsun) → review?(cpearce)
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.
Attachment #8539959 - Flags: review?(cpearce) → review-
Call CreateMediaDecodeTaskQueue() to create the TaskQueues.

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85da090730bb
Attachment #8539959 - Attachment is obsolete: true
Attachment #8540580 - Flags: review?(cpearce)
Attachment #8540580 - Flags: review?(brsun)
Attachment #8540580 - Flags: review?(cpearce) → review+
Attachment #8540580 - Flags: review?(brsun) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5141b939184
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.