Re: Audio Workers - please review

On Thu, Sep 11, 2014 at 6:03 AM, Chris Wilson <cwilso@google.com> wrote:

> On Wed, Sep 10, 2014 at 6:29 PM, Ehsan Akhgari <ehsan@mozilla.com> wrote:
>
>> I took a look at the merged version of this proposal and have some
>> feedback, I would appreciate if you can please let me know what you think.
>> As you know, I haven't been closely involved with the spec for a while, so
>> if the below reflects my ignorance on some of the previously discussed
>> matters, please accept my apologies in advance.
>>
>
> Not at all.  This is a fairly radical proposal, so I welcome discussion.
>
>> a) It is not really clear what should happen after you call terminate()
>> on AudioWorkerNode, since that seems to be orthogonal to whether the node
>> participates in the graph.
>>
>
> Noted.  I added a bit of text to further describe this.  In short: no, it
> no longer participates in the graph.
>

OK, I think we should remove terminate().  It seems that the exact same
effect can be achieved by AudioNode.disconnect().


>
>
>> b) The interaction between worker closing/termination and the audio
>> processing is not clear at all.  Specifically, dedicated workers typically
>> have a long time that is as long as the last pending task (assuming that
>> the main thread doesn't hold a reference to them), however, for these
>> workers, we need to keep firing audioprocess events.
>>
>
> I'm not sure why that is true.  The spec explicitly says when terminate()
> is called, that will cease firing of onaudioprocess.
>

Please see above.


>
>
>> c) How would different AudioWorkerNodes allocated through different
>> AudioContext objects interact with each other?  (Here is an example of
>> something that is not clear, should it be allowed to send an AudioParam
>> belonging to a node form context 1 to a worker node from context 2?  The
>> spec is currently silent about this.)
>>
>
> You are correct, it in.  In general, the spec is silent on how AudioNodes
> created from different AudioContext objects interact with each other.  I've
> known this was an issue, but had not ensured that we had an issue filed.
>  Now we do: https://github.com/WebAudio/web-audio-api/issues/351.  I
> don't think this is specific to Audio Workers.
>

Hmm.  So I'm a bit confused here.  I think I remember that the spec used to
prohibit connecting one node from one context to another one from a
different context, so that was what I had in mind when I wrote the above.
But checking the spec again, I don't see a mention of that.  Has that
changed in the spec, or is my memory faulty here?

At any rate, I agree that this is not very specific to worker nodes.


>
>
>> d) What is the exact semantics of the audioprocess events dispatched to
>> these workers?  Do they block running all audio processing on the audio
>> processing thread?
>>
>
> Yes.
>

OK, I think this could use clarification in the processing model.


> Note that because of the halting problem, we cannot even verify that the
>> worker script will ever terminate, let alone in a reasonable amount of
>> time, so in practice the UAs need to preempt the execution of the script.
>>
>
> Probably, as Alex mentioned - but I would expect the audio system to
> glitch in the meantime.  The point is to enable synchronous processing, and
> get rid of the inherent latency in script processing.
>

I think that is a good goal, but please see my reply to Alex on some things
that we can clarify there.


> It would be nice if this was somehow mentioned in the spec, at least in
>> terms of what the UA needs to do if it decides to abort the execution of
>> one of these scripts.
>>
>
> I'm not sure what to translate this to, in practical terms.
>

I gave two examples in my reply to Alex.


> e) What is the order in which these worker nodes receive the audioprocess
>> events (assuming obviously that one such node is not an indirect
>> input/output of the other)?  Note that with the currently specified API, I
>> think it is possible for one of these workers to send a MessagePort through
>> the main thread to another one, and therefore be able to communicate with
>> the other AudioWorkerNode workers through MessagePort.postMessage, so the
>> order of execution is observable to script.  (I admit that I don't have a
>> good solution for this -- but I also don't know what use cases for
>> transferring information back and forth between these nodes and the main
>> thread we're trying to address here.)
>>
>
> I don't think this is true; MessagePort is asynchronous, and the firing of
> onaudioprocess events would be synchronous (more to the point, the system
> would, I expect, resolve the entire audio graph for a block before
> processing messages).  I don't expect Audio Workers to communicate with
> each other, and I don't believe they can observe each others' behavior.
>

I'm not sure what you mean by MessagePort being asynchronous.  Obviously,
MessagePort.postMessage() is an asynchronous operation (I think that's what
you meant, please correct me if I'm wrong.)  What you describe above is my
understanding of the intention as well, but I don't think that's currently
specified.


>
>
>> 2. I feel very strongly against exposing this node type as a web worker.
>> I think that has a number of undesired side effects.  Here are some
>> examples:
>>
>> a) Even though the HTML web worker termination algorithm says nothing
>> about killing an underlying worker thread, I think that is what authors
>> would typically expect to happen, but obviously doing that would not be an
>> option here.  It is also not specified what needs to be output from the
>> node after terminate() has been called on it (the input unmodified?
>> silence? something else?)  Also, given the fact that you can already
>> disconnect the node from the graph, why do we need the terminate() method
>> in the first place?
>>
>
> I'm not sure why that's not an option, and the terminate() description
> DOES say the worker thread should be terminated.  (not the underlying
> system thread, but that's not intended.)  As Alex said, terminate() is
> there for uniformity with Workers, and for clarity in enforcing "no more
> onaudioprocess events, please."
>

That effect can be achieved by just disconnecting the node.


> b) The API gives you the illusion that multiple AudioWorkerNode's will run
>> on different DedicatedWorkerScopes.  That, in Web Workers world, would mean
>> different threads, but that will not be the case here.  I think that
>> discrepancy is problematic.
>>
>
> Why?
>

Honestly I can't quite imagine how authors would perceive this.  Here is an
example: do you think it's unreasonable for an author to expect that worker
nodes on the same audio context use the same global object?  The answer to
that question is observable to script.


> c) Using DedicatedWorkerGlobalScope is a bit weird in terms of APIs that
>> are exposed to workers.  For example, should these workers support
>> onlanguagechange?  What about IndexedDB on workers?  What about nested Web
>> Workers?
>>
>
> Yes, I would think they would support these.  Particularly nested Web
> Workers, e.g.
>

What is the use case in supporting these?


> d) At least on Gecko, Web Workers have specific implementation concerns in
>> terms of their message queue, event processing model and so on.  It might
>> be a lot of effort for us to properly implement the observable behavior of
>> these workers running inside our audio processing thread code (which has a
>> completely different event processing model, etc.)
>>
>
> That's the point of having this discussion, yes.
>
>
>> e) The topic of whether or not synchronous APIs must be allowed on
>> workers is being debated on public-script-coord, and it seems like there is
>> no consensus on that yet.  But I find the possibility of running
>> synchronous XHR on the audio processing thread unacceptable for example,
>> given its realtime requirements.
>>
>
> Of course.  That (using synchronous XHR) would be a dumb thing to do. So
> would "while (true) ;".
>

Well, labeling these concerns as dumb doesn't really address the issue.  I
agree with you completely that those are dumb, but the point is that we're
making them possible, so we should be thinking very hard about the
consequences of that.  There is always a chance of people doing these
things.


> I think tying this node to Web Workers opens a huge can of worms.  It will
>> also keep biting us in the future as more and more APIs are exposed to
>> workers.  I am wondering if we can get away with a completely different API
>> that only tries to facilitate the things that authors would typically need
>> to do during audio processing without attempting to make that a Web Worker?
>>
>
> Only by using some other language to represent an "audio shader", and
> having that language runtime run in the audio thread.  Anything else would
> add inherent latency, which would destroy the ability of this to
> self-describe Web Audio.
>

Is inventing a new language really our only alternative?  I don't think
so.  Another option is to run these workers under a restricted and new
global object that exposed built-in Ecmascript objects, and expose any
additional APIs that addresses a use case that makes sense to audio
processing.


> I think at the very least, if we really want to keep tying these concepts
>> to Web Workers, it might be worthwhile to bring that up on
>> public-script-coord, since it is at least bending the original use cases
>> that Web Workers were designed for.  :-)
>>
>
> Sure, I'm happy to ask public-script-coord for a review.
>
>
>> 3. What is the purpose of addParameter and removeParameter?  It seems to
>> me that if we defined a structured clone algorithm for AudioParam (which is
>> another detail missing from the spec that needs to be clarified anyway) and
>> make it transferable, then the author would be able to postMessage() an
>> AudioParam just as easily.  Is there a good reason to have a specialized
>> method for what is effectively posting an AudioParam to the worker?  (Note
>> that we'd probably need to add methods to AudioParam for extracting a-rate
>> and k-rate values at any given time in that case, so that the worker script
>> can get the right values when it needs them.)
>>
>
> No.  To have AudioParams be connectable (i.e. connect an Oscillator into
> an AudioParam) with zero latency, they must be evaluated in the audio
> thread, only in the current block, only when they are needed.  You can't
> extract ahead of time, or ask for them later; their value buffers need to
> be handed to the onaudioprocess when it fires.
>

That's exctly what I meant.  I was not suggesting adding methods for
arbitrarily peeking into the AudioParam values from the main thread.

(Note that my suggestion may mean exposing these objects through another
interface to the worker.)


> 4. More on the idea of transferring an AudioParam to a worker, that will
>> probably involve some kind of neutering the object.  It might make sense to
>> introduce a clone() method on AudioParam as a way for authors to be able to
>> keep a copy of the object around on the main thread.  That could of course
>> be a future enhancement idea.
>>
>
> In essence, the design of AudioParams here is much like AudioNodes
> themselves - one part sits in the main thread, one part sits in the audio
> thread.  The latter does the heavy lifting.
>

Well, my point was more towards the state that is stored inside the
AudioParam (namely the automation events.)  We need to specify what happens
if you hand off an AudioParam to a worker, and then add or remove an
automation event to/from it after that.


>
> 5. Still more on the idea of transferring an AudioParam, another thing
>> that we need to worry about is what happens if you try to transfer an
>> AudioParam that is currently being used somehow (either through being a
>> property on another AudioNode, or being used as an input to one.)
>>
>
> How would you make such a transfer?
>

Through addParameter?

Cheers,
-- 
Ehsan