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

RESOLVED FIXED in Firefox 50

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

(Blocks 1 bug)

45 Branch
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 2

3 years ago
Posted file Bug 1269741 - Test.
Review commit: https://reviewboard.mozilla.org/r/50419/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50419/
Attachment #8748612 - Flags: review?(karlt)
Attachment #8748613 - Flags: review?(karlt)
(Assignee)

Updated

3 years ago
Assignee: nobody → padenot
Paul - made a guess as to priority
Rank: 25
Priority: -- → P2
(Assignee)

Updated

3 years ago
Blocks: 1148354
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.
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 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
Attachment #8748613 - Flags: review?(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.
Attachment #8748612 - Flags: review?(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.
(Assignee)

Comment 10

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50419/diff/1-2/
Attachment #8748612 - Attachment description: MozReview Request: Bug 1269741 - Test. r?karlt → Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run.
Attachment #8748612 - Flags: review?(karlt)
(Assignee)

Updated

3 years ago
Attachment #8748613 - Attachment is obsolete: true
(Assignee)

Comment 11

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50419/diff/2-3/
Attachment #8748612 - Attachment description: Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run. → Bug 1269741 - Test.
(Assignee)

Comment 13

3 years ago
You suggestion works fine indeed (both in the test and in the implementation), thanks !
Attachment #8748612 - Flags: review?(karlt) → review+
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 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!
Attachment #8765373 - Flags: review?(karlt) → review+

Comment 16

3 years ago
Pushed by paul@paul.cx:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc90d9d43123
https://hg.mozilla.org/mozilla-central/rev/7b0517746549
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
No longer blocks: 1209598
Duplicate of this bug: 1209598
(Assignee)

Updated

3 years ago
Blocks: 1266646
(Assignee)

Updated

3 years ago
No longer blocks: 1266646
(Assignee)

Updated

3 years ago
Blocks: 1285060
Blocks: 1259831
You need to log in before you can comment on or make changes to this bug.