Closed Bug 1141261 Opened 5 years ago Closed 4 years ago

Immediately disconnecting audionode is not reflected in graph


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

37 Branch


(firefox43 fixed)

Firefox 43
Tracking Status
firefox43 --- fixed


(Reporter: jsantell, Assigned: jsantell)



(Whiteboard: [polish-backlog])


(1 file, 5 obsolete files)

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.
Attached patch 1141261-queue-wae.patch (obsolete) — Splinter Review
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
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.

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.
Attached patch 1141261-queue-wae.patch (obsolete) — Splinter Review
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)
Comment on attachment 8575386 [details] [diff] [review]

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


::: 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+
Attached patch 1141261-queue-wae.patch (obsolete) — Splinter Review
Rebased, new try push
Attachment #8575386 - Attachment is obsolete: true
Attachment #8583513 - Flags: review+
Attached patch 1141261-queue-wae.patch (obsolete) — Splinter Review
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.
Attachment #8583513 - Attachment is obsolete: true
Attachment #8585676 - Flags: review+
No longer blocks: 1134046
Whiteboard: [fixed-in-fx-team][devedition-40]
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]
Attached patch 1141261-queue-wae.patch (obsolete) — Splinter Review
Rebasing, seeing if bug 1161072 clears up that disconnection failure.
Attachment #8585676 - Attachment is obsolete: true
Attachment #8651546 - Flags: review+
Depends on: 1204595
Looks like bug 1204595 fixes this without the need for a queueing system. Here are the tests that ensure that.
Attachment #8651546 - Attachment is obsolete: true
Attachment #8661542 - Flags: review?(vporof)
Attachment #8661542 - Flags: review?(vporof) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.