Closed Bug 1153370 Opened 7 years ago Closed 7 years ago

Miscellaneous followup improvements to media dispatch architecture

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(4 files)

jww had several good suggestions in bug 1151656. I'll implement them here.
We can do this now that we're not manually nulling out mRunningThread anymore.
Attachment #8591098 - Flags: review?(jwwang)
This reduces the potential for deadlocks.
Attachment #8591099 - Flags: review?(jwwang)
Attachment #8591097 - Flags: review?(jwwang) → review+
Attachment #8591098 - Flags: review?(jwwang) → review+
Attachment #8591099 - Flags: review?(jwwang) → review+
Comment on attachment 8591096 [details] [diff] [review]
Part 1 - Remove AbstractThread::Create. v1

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

::: dom/media/AbstractThread.h
@@ +25,5 @@
>   *
> + * At present, the supported AbstractThread implementations are MediaTaskQueue
> + * and AbstractThread::MainThread. If you add support for another thread that is
> + * not the MainThread, you'll need to figure out how to make it unique such that
> + * comparing AbstractThread pointers is equivalent to comparing nsIThread pointers.

I am still concerned with the AbstractThread equality for the MediaTaskQueue which has no such nsIThread pointer at all.

Quote from TaskDispatcher.h
"At present, this is primarily useful to ensure that mirrored state gets updated atomically - but there may be other applications as well."

I still have no idea how this atomicity will play out as MediaTaskQueue is concerned. Things should get clear as more code uses TailDispatcher.
Attachment #8591096 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #6)
> > + * At present, the supported AbstractThread implementations are MediaTaskQueue
> > + * and AbstractThread::MainThread. If you add support for another thread that is
> > + * not the MainThread, you'll need to figure out how to make it unique such that
> > + * comparing AbstractThread pointers is equivalent to comparing nsIThread pointers.
> 
> I am still concerned with the AbstractThread equality for the MediaTaskQueue
> which has no such nsIThread pointer at all.

What is the problem, exactly? MediaTaskQueue just inherits AbstractThread, so we get pointer-uniqueness by default, and a MediaTaskQueue* will never compare equal to the main thread singleton.
You need to log in before you can comment on or make changes to this bug.