Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[audioworklet] AudioWorkletProcessor ondata should just be a callback rather than an EventHandler #1194

Closed
bfgeek opened this issue Apr 5, 2017 · 56 comments
Assignees
Labels
Needs Discussion The issue needs more discussion before it can be fixed.
Milestone

Comments

@bfgeek
Copy link

bfgeek commented Apr 5, 2017

I believe the intention here was to specifically deliver any ondata's sent from the main thread directly before the process method is called for that node?

If so AudioWorkletProcessor shouldn't have ondata being a EventHandler just a callback similar to process.

@hoch
Copy link
Member

hoch commented Apr 5, 2017

Yeah, these were loose ends that we'll have to figure out. I assume its interface should be loosely defined by a prose, not in a form of IDL?

How about sendData()? Perhaps we can start based on postMessage() and subtract things that we don't need?

@annevk
Copy link

annevk commented Apr 6, 2017

@domenic should probably be involved here to some extent if you're going to redesign postMessage(). What exactly are the goals of this new method?

@hoch
Copy link
Member

hoch commented Apr 6, 2017

@domenic @annevk

There two classes: AudioWorkletNode (main thread) and AudioWorkletProcessor (worklet thread). When user creates a AWN object, an instance of AWP is also created in the worklet global scope. They are tightly coupled from the construction and communicate each other using sendData() and ondata.

If we can send serialized data between them, that's good enough for our purpose. So making ondata as a UA-invoked callback makes sense because we don't need the whole nine yard of Event class. Also the destination of sendData() is defined at the construction of AWN/AWP pair, and it cannot be changed.

Please let me know if you need more clarification!

@annevk
Copy link

annevk commented Apr 6, 2017

I see, the goal is not exposing EventTarget and friends in worklets. Okay.

  1. Let's not call the callbacks something that starts with "on". Within the platform all of those are event handlers. We should stay true to that.
  2. Are these worklets always same-origin? Or do we need a security story?
  3. If they are same-origin and we don't need a security story this should indeed be pretty easy.

@hoch
Copy link
Member

hoch commented Apr 6, 2017

By 2, do you mean by "same-origin" that the worklet script from a different origin cannot be used? Hmm. That actually sounds bad for our use cases. Can you explain what kind of 'security story' we need?

@annevk
Copy link

annevk commented Apr 6, 2017

@hoch I guess it depends on what the origin in charge is of that worklet script. I noticed in the worklet specification that the worklets are loaded with CORS, so I guess they end up being CORS-same-origin in which case we probably don't need anything special. Is that correct?

@hoch
Copy link
Member

hoch commented Apr 6, 2017

That's correct and I missed that. Just to be sure, I am including our spec editors here: @rtoy and @padenot. I believe @bfgeek also has some thoughts on this.

@annevk
Copy link

annevk commented Apr 10, 2017

Note also that StructuredDeserialize can fail. You need to consider that and either use a different error callback or some other way to signal failure on the receiver end. See also whatwg/html#936.

@annevk
Copy link

annevk commented Apr 10, 2017

cc @bakulf

@hoch
Copy link
Member

hoch commented Apr 10, 2017

Okay, here's my conclusion from this thread:

  1. Change EventHandler ondata to processordata of void callback. This callback won't be a part of WebIDL. Its behavior will be described in a prose just like what we do with AudioWorkletProcessor's process().
  2. For V1, the script form the different origin is not allowed.
  3. For the failure of StructuredDeserialize, I think we should channel the exception to the main scope. The same thing also should happen when process() call fails due to the exception.

@domenic
Copy link
Contributor

domenic commented Apr 10, 2017

For the failure of StructuredDeserialize, I think we should channel the exception to the main scope. The same thing also should happen when process() call fails due to the exception.

It's possible it makes sense to do this for web audio; I am not sure. But I just wanted to note that for HTML and postMessage(), we will not tell the originator about deserialization failures, but instead we will tell the target that they were supposed to be getting a message, but did not. (Via a new "messageerror" event.)

@hoch
Copy link
Member

hoch commented Apr 10, 2017

@domenic So you mean we want to signal the error to AudioWorkletGlobalScope, not the main scope. I believe that's logically correct, but how can developers see/monitor the error message in that case?

@domenic
Copy link
Contributor

domenic commented Apr 10, 2017

Using self.addEventListener("messageerror", e => ...) for example

(Or using the developer tools, which should show all such errors.)

@padenot
Copy link
Member

padenot commented Apr 11, 2017

For V1, the script form the different origin is not allowed.

Why would that be ?

@annevk
Copy link

annevk commented Apr 11, 2017

As long as there is CORS, which there is per the worklet loading pipeline, it should be fine.

@hoch
Copy link
Member

hoch commented Apr 11, 2017

In that case, we can simply refer what the worklet does. We don't want to repeat how CORS works on the worklet in the WebAudio spec.

@hoch
Copy link
Member

hoch commented Apr 11, 2017

PTAL @rtoy, @padenot and @joeberkovitz:

First, we need to take ondata handler out of the IDL.

interface AudioWorkletNode : AudioNode {
  readonly attribute AudioParamMap parameters;
  void sendData(any data);
}

interface AudioWorkletProcessor {
  readonly attribute AudioContextInfo contextInfo;
  void sendData(any data);
}

Secondly, we need a prose that describes how processordata or nodedata works.

  • sendData(any data) method uses StructuredSerialize(data) before sending the data.
  • AudioWorkletNode has processordata(serializedData) callback to handle the data from the associated AudioWorkletProcessor.
  • AudioWorkletProcessor has nodedata(serializedData) callback to handle the data from the associated AudioWorkletNode.
  • These callbacks invoke StructuredDeserialize(serializedData) before the data can be used. The failure in deserialization process throws a messageerror exception to the scope.

I guess this also covers: #1193.

@hoch
Copy link
Member

hoch commented Apr 11, 2017

self.addEventListener("messageerror", e => ...)

@domenic @bfgeek WorkletGlobalScope does not inherit EventTarget, thus we don't have such functionality. Perhaps we should consider adding something like this to the global scope?

@domenic
Copy link
Contributor

domenic commented Apr 11, 2017

I see, that is a bit of a blocker. I am curious what @bfgeek thinks then. It seems better to have such events than to have each type of worklet have its own bespoke way of receiving errors from the main thread.

@annevk
Copy link

annevk commented Apr 12, 2017

Another question, @bfgeek suggested worklets should not support shared memory, but @flagxor said on blink-dev audio worklets should. So which is it?

@flagxor
Copy link

flagxor commented Apr 12, 2017

For audio worklets in particular, access to shared memory seems key to being able to allow low latency audio from game engines, audio codecs, or speech synthesizers (just spoke to folks that do that one today).
Currently engines like Unity use different audio on the web than all other platforms, because engines like Wwise can't be supported with WebAudio's event loop driven style.

A typical pattern would likely be to use a ShardArrayBuffer + audio worket + a web worker running wasm code to mirror something like what I understand AudioWorkers to have originally looked like. Where JS running on an audio thread would have danger of not being able to keep up with hard deadlines, Wasm code can avoid GC pauses, and presumably deliver reliable audio rates.
The first non-main thread thing we ended up needing for NaCl was audio, so I rather expect to need to agitate for something to fill the API gap (and am hoping audio worklets will fill that need).

I believe blocking operations like Atomics.wait can be excluded similar to on the main ui thread, as typically these kinds of applications will just want to keep a lock free queue full.

I understand you guys want to avoid a pattern where audio data is posted, With SABs, there would be a single post, and then audio data is read directly from the buffer, so should be fast assuming the sender can keep up.

My general sense is that as more of the Web API surface is hardened to handle shared memory, more surfaces will make sense in general to support SharedArrayBuffer. But audio in particular is an area where Unity and others have raised concern the current solution isn't performant (as with current WebAudio you simply can't both generate your own audio and know you won't skip).

By the way, I had spoke to @rtoy a bit back about this and had the impression you guys were approaching v1 completeness, so I certainly didn't mean to derail that. Rather, I'm suggesting this is likely to be something folks will want to try this year, so worth not planning out in some general way.

@annevk
Copy link

annevk commented Apr 12, 2017

I'd prefer we write it down now as we think it should behave going forward. In particular I'd rather not revise the HTML Standard several times on this. (The HTML Standard is in "charge" of making sure Agent / Agent Cluster actually have grounding in browsers.)

(Aside: please don't use guys as a gender-neutral term; folks works if I can make a suggestion.)

@padenot
Copy link
Member

padenot commented Apr 12, 2017

Audio Worklets will have to support shared memory at some point to be competitive, that's for sure, for doing something like:

  • Asset sharing (i.e. only having a single copy of the data in multiple locations, probably accessed in a read-only fashion)
  • Lock-free ring buffers (writing on the main thread or a worker thread, read in the worklet thread)

However, simply having transferables (for now), until a couple vendors ship an implementation, while making sure there is nothing blocking worklets to use shared memory in the future (since it's opt-in) seems like a reasonable path to take.

Transferables, while generating some GC pressure, avoid copies (doing what is essentially a pointer swap), and the event loop is used as a synchronization mechanism, this can go a long way.

@annevk
Copy link

annevk commented Apr 12, 2017

Shared memory is actually not as much opt-in specification-wise. If your variant of postMessage() is equally generic and we make worklets part of the agent cluster of their owner (always a Window/document as I understand it) you'd just have it.

@padenot
Copy link
Member

padenot commented Apr 12, 2017

I've been told that API need to opt-in to be able to receive a SharedArrayBuffer, which is the reason why the Web Audio API don't use them, because we haven't prioritized speccing/implementing it.

I can't really find an authoritative source at the moment, but this line has been authored by the person who did SharedArrayBuffer.

If this is not the case, then we don't have much to do.

@domenic
Copy link
Contributor

domenic commented Apr 13, 2017

That makes sense to me. It's a pretty different programming model than workers, but it seems internally consistent, and the motivation of trying to make it per-node and avoid global state in every way is a good one.

@bfgeek
Copy link
Author

bfgeek commented Apr 13, 2017

@annevk re: SABs. This discussion was clarifying, initially I was only primarily thinking about the Houdini specs, and Audio wasn't top of mind.

So I think it makes sense to allow SABs for worklets initially, (this means that worklets are in the same group(?) as window and workers?). Allowing them for all worklets should be fine as we won't be providing any thread-to-thread communciation channel for the css-{layout,paint,animation}-apis. I.e. there isn't any "sendData" analog in those specs, everything is controlled by the engine.

Punting on transferrables still seems reasonable still i think in this specific case?

@hoch One thing that we should be clear about in the spec is not allowing futexes on the audio thread, I'm assuming this would be "bad thing"? :P.

@bfgeek
Copy link
Author

bfgeek commented Apr 13, 2017

Finally, after thinking about this a little, I'm personally 60/40 on if this should be callback or an eventhandler.

I think a simple callback is slightly nicer than having an event attached to an object? I.e.
ondata(event) { this.data = event.data; } vs. ondata(data) { this.data = data; } especially if we don't think that any of the other fields will be used on the event.

My personal default model for worklets has been use callbacks by default, but really curious what others think there.

Also as a historical node I think this feature was initially called postMessage but morphed to sendData as didn't support transferrables, and had instance-to-instance semantics.

@domenic
Copy link
Contributor

domenic commented Apr 13, 2017

Note that if you're talking about a real event handler, the syntax ondata(event) { ... } doesn't work, as that does a DefineProperty, not a Set, so it doesn't trigger the setter. Instead you need to do this.ondata = ... in the constructor.

@bfgeek
Copy link
Author

bfgeek commented Apr 13, 2017

Ah thanks.

@annevk
Copy link

annevk commented Apr 13, 2017

Couple notes:

  1. postMessage() isn't necessarily global. With MessageChannel it's very much locally-bound. I'm personally okay with not reusing it or events though, but it might be a tad of a learning curve if developers have to learn all kinds of new APIs just for worklets. It's not immediately clear to me it's that much more expensive to support events. It will also be more maintenance for everyone involved. Each novel variant of serializing and deserializing adds cost.
  2. I'm okay with not doing transferables, I was just noting it seems that @padenot is very much expecting them to be there.
  3. SAB: worklets would be their own Agent and in the same Agent Cluster as their parent document/Window (and its associated Windows) and any associated dedicated workers. The other thing here is that we'd make [[CanBlock]] false for the Worklet Agent per comments from @flagxor above.

@domenic
Copy link
Contributor

domenic commented Apr 13, 2017

Random idea, haven't thought it through: could each worklet class get a MessagePort instance? One on the "inside" (via the base class presumably), and one on the outside (via the worklet class)?

@flagxor
Copy link

flagxor commented Apr 13, 2017

Thanks all for talking thru SAB use case + impact.

FWIW, coming to this particular topic without all the context, here's one more thing to consider with Events vs simple callbacks. I've heard concern expressed that for audio worklets, in practice, anything that could cause a garbage collect has the potential to pop the audio stream (as collects could delay timely audio delivery). I assume more extensive measures would be required to avoid generating garbage for each Event?

With general JS running in the worklet, there'd be other things that might trigger GC, but Wasm at least has the potential to avoid it if care is applied, assuming the worklet plumbing / event loop itself doesn't generate garbage.

(Aside: please don't use guys as a gender-neutral term; folks works if I can make a suggestion.)

Yes indeed, quite so.

@joeberkovitz joeberkovitz added the Needs Discussion The issue needs more discussion before it can be fixed. label Apr 13, 2017
@hoch
Copy link
Member

hoch commented Apr 13, 2017

@bfgeek Yes, @domenic pointed out a while ago that we'd have to use callback not the event handler. I couldn't find that comment now, but I forgot to fix our IDL to reflect the discussion.

@annevk Since AudioWorklet is the first to implement the instance-to-instance messaging mechanism, I understand that you want to explore every option possible. However, the constraints we have is very different from other Worker or Worklet variants. The audio rendering thread will run JS VM so the GC triggered by user-supplied code will stop the entire audio rendering operation, which will eventually cause the glitch. (At least this is unavoidable in Chrome) If exposing Event/EventTarget/postMessage increases the chance of GC in that thread, then it already defeats the purpose of the AudioWorklet project. It needs to provide the simplest way of sending message, and that's why @bfgeek and I came up with sendData().

@smaug----
Copy link

GC may be triggered anyhow, whether or not Events are used.

If you really want to optimize out possible GCs, the passed data must not be any new object, but some mutating array containing raw data or something, something which would change any time new data is received.
If that is not done, I don't quite see the argument to not use Events.

(In Gecko the wrappers for Events are allocated from nursery heap to try to avoid major GCs. Minor GC should be fast.)

@annevk
Copy link

annevk commented Apr 13, 2017

Since AudioWorklet is the first to implement the instance-to-instance messaging mechanism

Just to reiterate, MessageChannel already does this.

And yeah, if you really want to avoid GC, you should have just a SharedArrayBuffer object and some kind of signal for when it's updated (per @wanderview).

@joeberkovitz joeberkovitz added this to the Web Audio V1 milestone Apr 13, 2017
@hoch
Copy link
Member

hoch commented Apr 13, 2017

Perhaps I have not been clear about this, but the primary purpose of sendData() and ondata is not about sharing data between two threads. It is about being able to send a light-weight "control message" to the processor.

Having two different layers for the different rate of signal (a-rate and k-rate) has been a successful pattern for computer music programming. (e.g. SuperCollider or Max/MSP) From my perspective, sendData() is designed to handle the k-rate data. The control (k) data is commonly very small and ephemeral; it is used and gets thrown away. For example,

// From the main global scope to the associated processor.
myNode.sendData({ volume: 0.6, type: 'noise' });

Supposedly you can send whatever you want (and it'll be serialized at the receiving end), but sending thousands of audio samples over the thread is definitely not the use case here. If it were our intention to build this API, we would have chosen SAB or Transferable for sure.

It might makes sense to have the more generalized messaging mechanism across the web platform, but I would really like to argue that the light-weight messaging is super important for audio.

@padenot @rtoy @joeberkovitz WDYT?

@smaug----
Copy link

smaug---- commented Apr 13, 2017

myNode.sendData({ volume: 0.6, type: 'noise' }); already creates garbage, not much different to postMessage(). Are you saying the object created by sendData is somehow ok, but the Event object created by postMessage isn't?

Sure, the data with postMessage would be event.data

@domenic
Copy link
Contributor

domenic commented Apr 13, 2017

Yes, if the argument is about lightweightness or GC, I don't see any reason to avoid postMessage/MessageChannels.

If you need to avoid GCs, then as @annevk said, you should be using shared memory. If you just want to have small lightweight messages, postMessage/MessageChannels are fine for that purpose; the additional Event object is not a significant overhead.

@hoch
Copy link
Member

hoch commented Apr 13, 2017

I would like to take stab at using "the implicit MessageChannel" for AudioWorkletNode and AudioWorkletProcessor. @domenic suggested the worker interface is already using this pattern, and I believe this could work for the AudioWorklet case.

// From the main scope.
myNode.port.postMessage(...);

// In AudioWorkletGlobalScope
class extends AudioWorkletProcessor {
  constructor() {
    this.port.onmessage = () => { ... };
  }
  ...
}

@domenic also suggested to expose port as a member of the class instead of extends MessagePort to processor/node. I think it has to be this way, because we don't want start()/stop() methods to leak to the node or the processor. We expect developers to use start()/stop() method for their own custom node.

Two issues on my mind:

  1. We cannot define onmessage handler as a part of the class definition. this.onmessage must be defined in the constructor. Not ideal, but it's not the end of the world. It also seems like there is no workaround for this.
  2. postMessage() allows Transferable by definition. Not sure what other WG members think about this.

@rtoy
Copy link
Member

rtoy commented Apr 13, 2017 via email

@padenot
Copy link
Member

padenot commented Apr 14, 2017

tl;dr, we should use MessagePort and postMessage.

Long version:

Big GC pauses are to be avoided, so being able to use Transferables or shared memory is a must. For control messages, the controls objects in @hoch examples should be short lived or very long lived (i.e. reused across callback), and the GC pauses should be minimal (considering the techniques implemented in modern browsers).

In any case, minimizing the GC churn is a worthwhile goal (and not only for audio, webgl, webvr, and the new vulkan-like API that's shaping up have those constraints, although it's a bit less critical for anything but audio, considering the latency we're dealing with, here).

If we really want to pick something that has good performance characteristics for our use case, we should be doing some hands-on analysis to quantify the impact of various APIs that require creating js objects.

For example, we should measure, for a single IPC transmission of an empty object:

  • the number of allocations and deallocations
  • the number of GC-managed objects created on the worklet thread
  • the time time it takes to GC those objects
  • the time it takes to receive and send and object from the worklet thread (effectively the transmission overhead), taking into account, for example, the serialization/deserialization algorithm

For all those absolute metrics, we should compute their standard deviation (because we're interested in the worst case, the Web Audio API being a real-time system). Then we can compare APIs styles (normal MessagePort + postMessage, a custom sendData, etc.) against each other, and decide.

However, my guess is that any communication API that exists today on the web platform allocates memory and causes GC churn, and we need to pick an API allows minimizing this.

Going with our custom API might make some of this easier, and might even allow GC-less processing on the worklet side, but is probably more complicated, and it's unclear that it would make things faster in the long run.

Indeed, allowing SharedArrayBuffer, and communicating via the usual lock-free mechanisms is an option that solves all this, but is more involved for authors. However, we're dealing with a spectrum of authors, with different needs and knowledge. Being able to use a slower, easier method of communication (postMessage with transferables) is not in opposition with doing things the "right" way (SharedArrayBuffer + lock free algorithm, where you'd use postMessage once to set things up and then build a custom and fast communication side-channel).

In conclusion, I like @hoch's and @domenic view, summarized in #1194 (comment), for the following reasons:

  • it solves the problem
  • it reuses an existing and well-known web platforms construct, authors are familiar with it, implementers can probably (I haven't checked) reuse code
  • it can be used in an easy-but-less-performant style (sending js objects, causing some GC churn that we haven't quantified)
  • it can be used in an harder-but-more-performant style (using SharedArrayBuffer and atomics, probably causing little to no gc churn)
  • can function without SharedArrayBuffer if not available in the implementation, taking into account the various timeline we're working with here, shipping AudioWorkler, shipping SharedArrayBuffer and also the fact that some people will disable SharedArrayBuffer for privacy reasons.

@padenot
Copy link
Member

padenot commented Apr 14, 2017

And since WebAudio supports sample rates of 96 kHz (Chrome supports 384 kHz), any pause that is a significant fraction of 1.33 ms (128/96000) (0.33 ms for Chrome) will cause a glitch.

It is technically true that on some OSes you can open an audio stream with a buffer size of 128 frames (or less, but Web Audio API has a fixed block size of 128 frames), at a very high sample-rate, which would mean having a callback each 0.33ms in the worst case (this is not specific to Chrome, it mostly depends on the capability of the sound card and the OS and the system configuration). Depending on the OS, you then have a buffer queue, or buffer ping-ponging.

However, in practice, the measured loopback latency of a modern browser is around 30ms on OSX (this is from a paper that is probably going to be published at the next Web Audio Conference, it's not public yet. This figure is on OSX, measures the input->output latency, and is true for Firefox and Chrome). This is because browsers are using conservative buffer sizes to allow for higher callback load (i.e., being able to do more expensive processing without glitching) and to work around device-specific bugs, and also because modern browsers are using a multi-process and multi-thread architecture, bouncing the audio between multiple threads and processes, that might or might not be high priority/real-time thread.

This allows for some slack, but I clearly agree we should design this API in a way that we does not prevent optimizing things in the future.

@annevk
Copy link

annevk commented Apr 14, 2017

So if we go down that road we need to:

  1. Change DOM to expose Event and EventTarget in worklets.
  2. Change HTML to expose MessageEvent, MessagePort, and MessageChannel in worklets.

And presumably only audio worklets. I can help with the specification-side of that, but to land changes in those standards we'll also need to have web-platform-tests that I'd prefer someone else to take on.

@domenic
Copy link
Contributor

domenic commented Apr 14, 2017

Technically no need for MessageChannel.

@hoch
Copy link
Member

hoch commented Apr 14, 2017

@bfgeek @nhiroki It seems like we have several things to consider in terms of the Worklet infra. (comment, comment) WDYT?

@joeberkovitz
Copy link
Contributor

I've been following this silently so far and I'm all for investigating the use of MessagePort. I just want to throw out an opinion that in terms of frequency, this kind of messaging will typically occur only when the application wants to change an attribute or invoke an operation on a node. It doesn't happen on every render quantum. I am not sure if the concerns about GC are taking this usage profile into account, but maybe it's a legit concern even if the messaging is not so frequent.

@padenot
Copy link
Member

padenot commented Apr 20, 2017

If something is possible, it will be abused.

In any case, we have been chatting about this in w3c/css-houdini-drafts#380, and it validates the plan laid out in #1194 (comment).

@joeberkovitz
Copy link
Contributor

From WG call today: @hoch is going to pursue the MessagePort/postMessage approach since it leverages existing spec work. If there are performance problems from GC, they can emerge and be measured.

@hoch hoch changed the title AudioWorkletProcessor ondata should just be a callback rather than an EventHandler [audioworklet] AudioWorkletProcessor ondata should just be a callback rather than an EventHandler Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion The issue needs more discussion before it can be fixed.
Projects
None yet
Development

No branches or pull requests

9 participants