1141261 - Immediately disconnecting audionode is not reflected in graph

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect, P2)

Description

[Jordan Santell [:jsantell] [@jsantell]]

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

The gain node should be disconnected from the destination node here.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 7

[Victor Porof [:vporof][:vp]]

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.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1141261-queue-wae.patch

Rebased, new try push
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e2d7af47c19

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

https://hg.mozilla.org/integration/fx-team/rev/316158d4a13e

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/4ebaf79e19ff

Comment 12

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3034759&repo=fx-team

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Ah looks like similar to bug 1138576, but slightly different, I guess this patch made it more consistent. I'll check it out, thanks!

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1141261-queue-wae.patch

Rebasing, seeing if bug 1161072 clears up that disconnection failure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b19721f265d

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

latest try push from the other week: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a6108b3d00e

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e39cb56d2a63

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/e39cb56d2a63