1025864 - Node parameters in the inspector are not updated often enough

Status: RESOLVED WONTFIX

(DevTools Graveyard :: Web Audio Editor, defect)

Description

[Soledad Penades [:sole] [:spenades]]

It might be interesting to implement some sort of "Observe" in nodes to keep track of actual parameter values when a given node is being inspected.

Currently when a node is being inspected its values are "static" so if they are changed via code they are not updated in the inspector until the node is clicked again (which I guess prompts a refresh on the properties inspector)

Suppose I am inspecting an oscillator and its frequency is 440.
If I then go to the console and change its frequency to 220 the value will transition to that and the effects will be audible, but the inspector will still show 440.
Only if I click again over the node I'll get the new value 220.

Comment 1

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

Ah good idea, one of the reasons for moving to only being able to see one node at a time was this -- this should be relatively straight forward!

Comment 2

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

After playing with this for a bit, the call-watcher instrumentation won't work for AudioParam (and escalated properties like `frequency`) and implementing an instrumenter in audionodeactors won't work as these don't have built in setters, unless we proxy the entire object, which sounds scary. Still thinking about possible solutions that don't have unintended side effects

Comment 3

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

Attachment: 1025864-observe-audioparams.patch

Some progress I've made.

Since AudioParams can be changed via automation, a setter would not capture these values, and observer/proxies would be too cludgey to continue to support things like automation as well, so the only other option (AFAICT) is polling.

This provides AudioNodeActor methods to decide whether or not we should be polling on the server for changes, emitting to the actor fronts. The devtool GUI turns on polling for the active nodes, and turns the previously selected node off, so we're only polling for the node of interest (on the server, minimizing RDP/x-boundary calls), and emitting to the front if a change was detected.

Problem with this currently is this fails for GC (only in e10s), when we have a node selected (so polling is on), this node will never get GC'd, due to accessing the underlying audio node's WeakRef in any capacity, within a timer.

Comment 4

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

Attachment: 1025864-observe-audioparams.patch

This got larger than expected. Lots of edge cases to test. Polling most performant option. Had to do some magic to have it work in e10s.

Comment 5

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

try: https://tbpl.mozilla.org/?tree=Try&rev=45e580d837c7

Comment 6

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

Attachment: 1025864-observe-audioparams.patch

Fixed the nullifying protocol.js Option issue, r?ing dcamp as well

Comment 7

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

try: https://tbpl.mozilla.org/?tree=Try&rev=e337780f139c

Comment 8

[Dave Camp (:dcamp)]

Comment on attachment 8470997 [details] [diff] [review]
1025864-observe-audioparams.patch

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

::: toolkit/devtools/server/protocol.js
@@ +447,5 @@
>      Arg.prototype.initialize.call(this, index, type)
>    },
>  
>    write: function(arg, ctx, name) {
> +    let v = arg[name];

Will this generate an exception if arg is actually undefined?  It's usually ok to not provide an options object at all.

Comment 9

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

Attachment: 1025864-observe-audioparams.patch

Yup, you are right -- I jumped the gun. Should be fixed here.

Comment 10

[Dave Camp (:dcamp)]

Comment on attachment 8471016 [details] [diff] [review]
1025864-observe-audioparams.patch

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

r+ for protocol.js change.

Comment 11

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

Try
https://tbpl.mozilla.org/?tree=Try&rev=af7d7640188c
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=af7d7640188c

Comment 12

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

handling protocol.js changes in bug 1052118

Comment 13

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

Attachment: 1025864-observe-audioparams.patch

Removed protocol.js changes here

Comment 14

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

Try, now with protocol changes landed in m-c https://tbpl.mozilla.org/?tree=Try&rev=ca469d95440d

Comment 15

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

https://tbpl.mozilla.org/?tree=Try&rev=4a30cc3e9a92

Comment 16

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

Try up and running: https://tbpl.mozilla.org/?tree=Try&rev=4fc314f9b661

Comment 17

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

Comment on attachment 8471198 [details] [diff] [review]
1025864-observe-audioparams.patch

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

Lot of code to digest, but pretty.

::: toolkit/devtools/server/actors/webaudio.js
@@ +263,5 @@
>    }, {
>      response: { params: RetVal("json") }
> +  }),
> +
> +  isAlive: function () {

Nit: maybe comment this function? When is this dead? (I know, GC, CC etc., but readers of this code may be confused).

Comment 18

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

new try to fix bug 1035820: https://tbpl.mozilla.org/?tree=Try&rev=745f83ec7ca0

Comment 19

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

i am a bad fixer https://tbpl.mozilla.org/?tree=Try&rev=a769e9499acc

Comment 20

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

Attachment: 1025864-observe-audioparams.patch

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1035820 with no end in sight.
https://hg.mozilla.org/integration/fx-team/rev/10346f72cdcb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/10346f72cdcb

Comment 25

[Patrick Brosset <:pbro>]

Mass-closing inactive (2 years+) bugs on unmaintained devtools components.