Closed Bug 1145686 Opened 6 years ago Closed 6 years ago

Enforce that MDSM state changes only happen on the state machine task queue

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(4 files, 2 obsolete files)

Doing it any other way is totally insane. Once bug 1136873 lands I think we can fix things up so that this invariant holds.
Blocks: MediaMonitor
Depends on: 1136827
We assert in CheckInDecodeComplete as well for good measure, since that's the
last caller of SetState that doesn't already assert it.
Attachment #8582862 - Flags: review?(matt.woodrow)
Comment on attachment 8582861 [details] [diff] [review]
Part 2 - Make MDSM::StartBuffering happen on the state machine thread. v1

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

::: dom/media/MediaDecoder.cpp
@@ +1498,5 @@
>    if (mResource) {
>      mResource->Resume();
>    }
>    if (aForceBuffering) {
>      ReentrantMonitorAutoEnter mon(GetReentrantMonitor());

Do we still need this monitor? We should be able to read/write |mDecoderStateMachine| on the main thread without the monitor.

@@ +1502,5 @@
>      ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
>      if (mDecoderStateMachine) {
> +      RefPtr<nsRunnable> r =
> +        NS_NewRunnableMethod(mDecoderStateMachine, &MediaDecoderStateMachine::StartBuffering);
> +      mDecoderStateMachine->TaskQueue()->Dispatch(r.forget());

I would prefer to do the dispatch inside StartBuffering() to hide the details about threading model employed by MDSM. As far as MediaDecoder can see, MediaDecoderStateMachine::StartBuffering() should be a thread-safe function.
Attachment #8582861 - Flags: feedback+
Attachment #8582860 - Attachment is obsolete: true
Attachment #8582860 - Flags: review?(matt.woodrow)
Attachment #8582880 - Flags: review?(jwwang)
Attachment #8582861 - Attachment is obsolete: true
Attachment #8582861 - Flags: review?(matt.woodrow)
Attachment #8582881 - Flags: review?(jwwang)
Comment on attachment 8582862 [details] [diff] [review]
Part 3 - Assert state machine thread for SetState. v1

In general, feel free to do a drive-by r+ on state machine patches. :-)
Attachment #8582862 - Flags: review?(matt.woodrow) → review?(jwwang)
Attachment #8582880 - Flags: review?(jwwang) → review+
Attachment #8582881 - Flags: review?(jwwang) → review+
Attachment #8582862 - Flags: review?(jwwang) → review+
Comment on attachment 8582898 [details] [diff] [review]
Part 4 - Dispatch all audiosink notifications asynchronously. v1

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

LGTM. Btw, MediaDecoder::UpdatePlaybackOffset() can now assert instead of acquiring the monitor. Stricter check makes it less likely to go unexpected.
Attachment #8582898 - Flags: review+
Blocks: 1148571
Depends on: 1144519
So I did a try push of all of these patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d40c2e06cd66

There was one failure in test_loop.html, which I confirmed to be caused by the topmost patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1b04b6ad68
https://treeherder.mozilla.org/#/jobs?repo=try&revision=855887ee7041

I split the topmost patch out into bug 1148571, and am going to push the rest of the patches here.
(the topmost patch was an extra "nice to have" suggestion by jww, and not really intrinsic to this bug to begin with)
You need to log in before you can comment on or make changes to this bug.