Immediately disconnecting audionode is not reflected in graph

RESOLVED FIXED in Firefox 43

Status

()

Firefox
Developer Tools: Web Audio Editor
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

(Blocks: 2 bugs)

37 Branch
Firefox 43
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [polish-backlog])

Attachments

(1 attachment, 5 obsolete attachments)

http://jsbin.com/lavuvakoka/1/edit

The gain node should be disconnected from the destination node here.
This is due to the client handling both onConnectNode and onDisconnectNode asynchronously, and the connect node taking longer than disconnecting (due to looking up 2 nodes rather than just one), so the disconnect node gets resolved first.

There are other scenarios that this could come up in, and this is a pretty important fix.
Created attachment 8575088 [details] [diff] [review]
1141261-queue-wae.patch

This queues all events from the server and executes them in order to prevent the race condition from occurring. This is a pretty big change and there were some side effects -- graph changed to use throttle rather than debounce to render to prevent delayed rendering. Still some issues in rendering that I'm figuring out, but all tests passing.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Currently, taking an extreme demo (Chris Wilson's vocoder[0] is always my fav), currently all the events are processed relatively quickly and the rendering is -debounced-, so once all events are processed and events stop coming in for 200ms, the graph draws. It's relatively good for this demo.

With this change, as events take slightly longer to process in extreme circumstances, using debounce leads to too long lag in rendering with debounce. Using -throttle- instead of debounce (so renders at most every 1000ms, does not wait for the calls to stop), works when the demo starts, renders more than half of the graph, and then the speed of having to re-render the entire graph slows down the event processing from the queue, rerendering on every audio node event, one per second. I think there's about 800 events on this demo, and half way through, they start processing 1 per second, so that's pretty much unusable (over 10 minutes to render everything!)

Gotta think about this some more.

[0] https://webaudiodemos.appspot.com/Vocoder/index.html
Removed some async actor event handlers as they're not necessary once the events come in order, which just leaves the initial event (node creation) async, as we have to fetch information about it, that's fine.

This makes graph rendering issues more noticible:

* using `debounce`, if there's a large stream of events, there'll be no rerendering during that stream. Noticible when looking at the vocoder demo as there's lots of events up front. Also noticible on any synth that creates nodes -- just press keys which will keep queueing up events, causing no rendering to ever occur until releasing keys.

* using `throttle` results in drawing at most every X ms, but doesn't take into account draw's render time sync.

We need a `throttle`, but a timer that resets after draw completes.
Created attachment 8575386 [details] [diff] [review]
1141261-queue-wae.patch

This was a fun one. Will need to add "smart" throttling of the canvas renderer based on the queue's backlog (if 500+ events queued, drain a good handful of them and render, etc.) to handle both complex and simple scenarios with good response time.
Attachment #8575088 - Attachment is obsolete: true
Attachment #8575386 - Flags: review?(vporof)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbca0535d574
(Assignee)

Updated

3 years ago
Blocks: 1134046, 1141638
Comment on attachment 8575386 [details] [diff] [review]
1141261-queue-wae.patch

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

k

::: browser/devtools/webaudioeditor/controller.js
@@ +43,5 @@
>     */
>    initialize: Task.async(function* () {
> +    // Create a queue to manage all the events from the
> +    // front so they can be executed in order
> +    this.queue = new Queue();

For a moment here I got excited thinking Queue is some sort of hot new JS built-in. Then I had a sad, but such is life.
Attachment #8575386 - Flags: review?(vporof) → review+
Created attachment 8583513 [details] [diff] [review]
1141261-queue-wae.patch

Rebased, new try push
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e2d7af47c19
Attachment #8575386 - Attachment is obsolete: true
Attachment #8583513 - Flags: review+
Created attachment 8585676 [details] [diff] [review]
1141261-queue-wae.patch

Was not properly clearing out queue on destruction -- fixed, should fix leaks on try. Holding off until Fx40 too, because bug 1141638 should be checked out in coordination with this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ab7bb59842a
Attachment #8583513 - Attachment is obsolete: true
Attachment #8585676 - Flags: review+
(Assignee)

Updated

3 years ago
No longer blocks: 1134046
https://hg.mozilla.org/integration/fx-team/rev/316158d4a13e
Whiteboard: [fixed-in-fx-team][devedition-40]

Comment 11

3 years ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/4ebaf79e19ff
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3034759&repo=fx-team
Ah looks like similar to bug 1138576, but slightly different, I guess this patch made it more consistent. I'll check it out, thanks!
Whiteboard: [fixed-in-fx-team][devedition-40] → [devedition-40]
Priority: -- → P2
Whiteboard: [devedition-40] → [polish-backlog]
Created attachment 8651546 [details] [diff] [review]
1141261-queue-wae.patch

Rebasing, seeing if bug 1161072 clears up that disconnection failure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b19721f265d
Attachment #8585676 - Attachment is obsolete: true
Attachment #8651546 - Flags: review+
(Assignee)

Updated

2 years ago
Blocks: 1125600
latest try push from the other week: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a6108b3d00e
(Assignee)

Updated

2 years ago
Depends on: 1204595
Created attachment 8661542 [details] [diff] [review]
1141261-audionode-dc.patch

Looks like bug 1204595 fixes this without the need for a queueing system. Here are the tests that ensure that.

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad9e78e8ca46
Attachment #8651546 - Attachment is obsolete: true
Attachment #8661542 - Flags: review?(vporof)
Attachment #8661542 - Flags: review?(vporof) → review+

Comment 17

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/e39cb56d2a63
https://hg.mozilla.org/mozilla-central/rev/e39cb56d2a63
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.