Run MDSM::DecodeError on the state machine thread

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla39
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Followup from bug 1135785 comment 9. I want to do this separately to reduce the risk of weird shutdown failures.
Blocks: MediaMonitor
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+
Blocks: 1145686
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.