1136827 - Run MDSM::DecodeError on the state machine thread

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

Followup from bug 1135785 comment 9. I want to do this separately to reduce the risk of weird shutdown failures.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop proxying DecodeError to the decode thread. v1

At this point, all the callers actually call it on the state machine thread,
where it belongs.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop synchronously dispatching MediaDecoder::DecodeError from MDSM::DecodeError. v1

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.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #1)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d80a60c55f9c

I was worried about those ICS emulator reds, but it looks like they happen on a blank try push too:

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb419228c1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/998f44ed19fb

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a4844f0ccd for crashtest assertions:

https://treeherder.mozilla.org/logviewer.html#?job_id=8011526&repo=mozilla-inbound

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 0 - Call OnAudioSinkError on state machine thread. v1

I missed this one. I think it also might explain those remaining crashtest
failures on bug 1142336.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 0 - Call OnAudioSinkError on state machine thread. v2

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 9

[Bobby Holley (:bholley)]

(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.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b0077c342f5
https://hg.mozilla.org/mozilla-central/rev/69699566f385
https://hg.mozilla.org/mozilla-central/rev/7e1f54979300