Closed Bug 1136827 Opened 6 years ago Closed 6 years ago

Run MDSM::DecodeError on the state machine thread

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

(3 files, 1 obsolete file)

Followup from bug 1135785 comment 9. I want to do this separately to reduce the risk of weird shutdown failures.
At this point, all the callers actually call it on the state machine thread,
where it belongs.
Attachment #8582569 - Flags: review?(matt.woodrow)
MediaDecoder::DecodeError invokes MediaDecoder::Shutdown, which shuts down the
state machine. Given the sync dispatch, this means that this is basically already
what's happening.
Attachment #8582570 - Flags: review?(matt.woodrow)
Attachment #8582569 - Flags: review?(matt.woodrow) → review+
Attachment #8582570 - Flags: review?(matt.woodrow) → review+
I missed this one. I think it also might explain those remaining crashtest
failures on bug 1142336.
Attachment #8582859 - Flags: review?(matt.woodrow)
Attachment #8582859 - Attachment is obsolete: true
Attachment #8582859 - Flags: review?(matt.woodrow)
Attachment #8582887 - Flags: review?(jwwang)
Attachment #8582887 - Flags: review?(jwwang) → review+
Comment on attachment 8582887 [details] [diff] [review]
Part 0 - Call OnAudioSinkError on state machine thread. v2

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

::: dom/media/MediaDecoderStateMachine.h
@@ +713,5 @@
>    // and the sink is shutting down.
>    void OnAudioSinkComplete();
>  
>    // Called by the AudioSink to signal errors.
>    void OnAudioSinkError();

Btw, this could be made private, right?
(In reply to Bobby Holley (:bholley) from comment #7)
> Created attachment 8582887 [details] [diff] [review]
> Part 0 - Call OnAudioSinkError on state machine thread. v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2e7a5af2545

Looks fixed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0077c342f5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/69699566f385
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7e1f54979300

(In reply to JW Wang [:jwwang] from comment #8)
> >    // Called by the AudioSink to signal errors.
> >    void OnAudioSinkError();
> 
> Btw, this could be made private, right?

It's called synchronously in AudioSink init, when we're on the state machine thread. I figured I might as well leave that alone for now.
Flags: needinfo?(bobbyholley)
Comment on attachment 8582887 [details] [diff] [review]
Part 0 - Call OnAudioSinkError on state machine thread. v2

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

::: dom/media/MediaDecoderStateMachine.h
@@ +711,4 @@
>  
>    // Called by the AudioSink to signal that all outstanding work is complete
>    // and the sink is shutting down.
>    void OnAudioSinkComplete();

I wonder if we should do the same to OnAudioSinkComplete() which changes |mAudioCompleted|. If we allow state changes only in-between state machine cycles instead of in the middle of them, it would be easier to establish some kind of "stable state" and check more state invariant. Just a thought.
Depends on: 1147495
You need to log in before you can comment on or make changes to this bug.