1269741 - Allow resuming a context that has been suspended in the same event loop turn

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

There is an optimization that tried to synchronously resolve the "resume"  [0]promise without sending it down the MSG, but it means that if the AudioContext has been suspended in the same event loop turn (which means the state has not been propagated back), then we return early where we should not.

We now have a main thread flag that lets us know which one has been called last, so we can use it.

Also there is the opposite case in the opposite direction.

https://dxr.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09/dom/media/webaudio/AudioContext.cpp#916

Comment 1

[Paul Adenot (:padenot)]

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

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1269741 - Test.

Review commit: https://reviewboard.mozilla.org/r/50419/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50419/

Comment 3

[Paul Adenot (:padenot)]

Attachment: MozReview Request: Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run. r?karlt

Review commit: https://reviewboard.mozilla.org/r/50417/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50417/

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Paul - made a guess as to priority

Comment 5

[Karl Tomlinson (:karlt)]

Sorry about the slow response here.
I think I'm going to need to have another look tomorrow to understand how the threads are interacting here.

Comment 6

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/50417/#review57184

::: dom/media/webaudio/AudioContext.cpp:869
(Diff revision 1)
> -  if (mAudioContextState == AudioContextState::Suspended) {
> +  if (mAudioContextState == AudioContextState::Suspended &&
> +      mSuspendCalled) {

It seems that the problem here is caused by bypassing the graph communication.
The proposed solution is to change the conditions tested when deciding whether
to bypass the graph communication.  The new conditions select out one
situation where this would cause problems, but leaves other inconsistencies
remaining.  I think it would be better to never bypass the graph
communication.  Even if there is a situation where we know it won't cause any
problems, I doubt the optimization is needed.

Can these shortcuts just be removed?

Consider the following scenarios with this patch:

Scenario I:

1. While the graph is running with state == running,
   suspend() is called and returns promise A.
2. suspend() is called again and returns promise B.
3. state transitions to suspended.
4. promise A is fulfilled.
5. suspend() is called again and returns promise C.

Expected:
6. promise B is fulfilled.
7. promise C is fulfilled.

Actual:
6. promise C is fulfilled.
7. promise B is fulfilled.

Scenario II:

1. While the graph is running with state == running,
   suspend() is called and returns promise A.
2. resume() is called and returns promise B.
3. resume() is called and returns promise C.

Expected

4. state transitions to suspended.
5. promise A is fulfilled.
6. state transitions to running.
7. promise B is fulfilled.
8. promise C is fulfilled. 

Actual

4. promise C is fulfilled.
5. state transitions to suspended.
6. promise A is fulfilled.
7. state transitions to running.
8. promise B is fulfilled.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8748613 [details]
MozReview Request: Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run. r?karlt

https://reviewboard.mozilla.org/r/50417/#review57186

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8748612 [details]
Bug 1269741 - Test.

https://reviewboard.mozilla.org/r/50419/#review57188

::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:371
(Diff revision 1)
>  }
>  
> +function testSuspendResumeEventLoop() {
> +  var ac = new AudioContext();
> +  var lastCurrentTime = ac.currentTime;
> +  var intervalHandle;

I guess intervalHandle is not required.

::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:372
(Diff revision 1)
> +  var tries = 20;
> +  var equal = 10;
> +  // Check the time still increases
> +  function check() {
> +    var currentCurrentTime = ac.currentTime;
> +    tries--;
> +    if (lastCurrentTime == currentCurrentTime) {
> +      equal--;
> +      if (!equal) {
> +        ok(false, "The AudioContext did not resume.");
> +        finish();
> +        return;
> +      }
> +    }
> +    if (!tries) {
> +      ok(true, "The AudioContext did resume.");
> +      finish();
> +      return;
> +    }

IIUC this is checking that time advances 20 times before there are 10 iterations where it doesn't advance.

The timeouts are chosen so that the expected advance before the second onstatechange does not trip the 20 count, and so that currentTime is likely to usually advance in one iteration.

What do you think about instead starting and stopping an OscillatorNode quickly after suspend() and looking for the ended event?

That would remove all question about whether the advance happened before the suspend was effected and whether currentTime will be updated frequently enough.

If the ended event is not dispatched then the test would fail due to the default test timeout.

::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:400
(Diff revision 1)
> +    ac.onstatechange = null;
> +
> +    ok(ac.state == "running", "initial state is running");
> +    ac.suspend();
> +    ac.resume();
> +    intervalHandle = setTimeout(check, 100);

intervalHandle set here but not used.

Comment 9

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/50419/#review57188

> IIUC this is checking that time advances 20 times before there are 10 iterations where it doesn't advance.
> 
> The timeouts are chosen so that the expected advance before the second onstatechange does not trip the 20 count, and so that currentTime is likely to usually advance in one iteration.
> 
> What do you think about instead starting and stopping an OscillatorNode quickly after suspend() and looking for the ended event?
> 
> That would remove all question about whether the advance happened before the suspend was effected and whether currentTime will be updated frequently enough.
> 
> If the ended event is not dispatched then the test would fail due to the default test timeout.

Or perhaps better than stop() on an OscillatorNode would be an AudioBufferSourceNode with a short buffer.
That would avoid any problems picking a stop() time if 0 doesn't work.
Or perhaps simplest is to create a ScriptProcessorNode after suspend.

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8748612 [details]
Bug 1269741 - Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50419/diff/1-2/

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8748612 [details]
Bug 1269741 - Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50419/diff/2-3/

Comment 12

[Paul Adenot (:padenot)]

Attachment: Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run.

Review commit: https://reviewboard.mozilla.org/r/60790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60790/

Comment 13

[Paul Adenot (:padenot)]

You suggestion works fine indeed (both in the test and in the implementation), thanks !

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8748612 [details]
Bug 1269741 - Test.

https://reviewboard.mozilla.org/r/50419/#review57614

::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:368
(Diff revision 3)
> +  var lastCurrentTime = ac.currentTime;
> +  var tries = 20;
> +  var equal = 10;

These variables can be removed now.

::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:385
(Diff revision 3)
> +
> +    ok(ac.state == "running", "initial state is running");
> +    ac.suspend();
> +    source.start();
> +    ac.resume();
> +    lastCurrentTime = ac.currentTime;

And this line.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8765373 [details]
Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run.

https://reviewboard.mozilla.org/r/60790/#review57616

Thank you, Paul!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc90d9d43123
Test. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0517746549
Allow resuming a suspended AudioContext in the same event loop run. r=karlt

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/cc90d9d43123
https://hg.mozilla.org/mozilla-central/rev/7b0517746549