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] Revise AWN/AWP instantiation algorithm: structured serialization and abstract operation #1193

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

Comments

@annevk
Copy link

annevk commented Apr 5, 2017

In whatwg/html@97d644c @domenic changed the way cloning works to be serializer-based instead. The Audio API needs to be updated to account for that.

@hoch
Copy link
Member

hoch commented Apr 5, 2017

@annevk We don't support Transfer in AudioWorklet's comm channel anyway, so changing the term from 'structured cloning' to 'serializing'. (and perhaps with a link to the reference) Does it makes sense?

@annevk
Copy link
Author

annevk commented Apr 6, 2017

Yeah, I think you want to invoke StructuredSerialize and StructuredDeserialize, referenced and with their proper names, but you also need to define the processing model of some of this in more detail.

E.g., step 4 of "The instantiation of AudioWorkletNode and AudioWorkletProcessor" currently says structured cloning is involved, but I can't find where that actually happens. (That algorithm also weirdly seems to cross threads without any kind of message passing or acknowledment that it's doing so. Is there an issue on that?)

@domenic
Copy link
Contributor

domenic commented Apr 6, 2017

I think ideally this would be an opportunity to make the actual usage more rigorous in "The instantiation of AudioWorkletNode and AudioWorkletProcessor". In particular, you'd have steps like:

  • Let serializedOptions be StructuredSerialize(options).
  • Let optionsInWorklet be StructuredDeserialize(serializedOptions, the AudioWorkletGlobalScope's Realm).
  • Now you can use optionsInWorklet in the Construct() step.

I'm not sure exactly how to phrase the prose references that currently say "structured clone". Maybe something like replacing

This may be any value or JavaScript object handled by the structured clone algorithm.

by

This value may be any serializable value

@domenic
Copy link
Contributor

domenic commented Apr 6, 2017

Oh, I see I left my tab open for too long and @annevk said what I was going to say, basically.

@hoch
Copy link
Member

hoch commented Apr 6, 2017

Thanks @annevk, @domenic - I will try to capture this change in our text.

@hoch
Copy link
Member

hoch commented Apr 6, 2017

One more question:

That algorithm also weirdly seems to cross threads without any kind of message passing or acknowledment that it's doing so. Is there an issue on that?

@annevk How do you describe cross-thread operation in terms of EcmaScript? The algorithm is largely based on the PaintWorklet, except that it has several cross-thread tasks. Any example for this pattern?

@annevk
Copy link
Author

annevk commented Apr 7, 2017

Well, each thread has an event loop. So what you end up doing is queuing tasks back and forth (message passing through tasks).

@padenot
Copy link
Member

padenot commented Apr 7, 2017

We have several examples of cross-thread messaging in the Web Audio API. When talking to the rendering thread, use a control message. We don't have a traditional event loop, but we have our own mechanism that is compatible with our processing based on rendering quantum. When talking to the main thread, queue a task on the main thread as usual, quoting the current spec:

Communication from the control thread to the rendering thread is done using control message
passing. Communication in the other direction is done using regular event loop tasks.

@annevk
Copy link
Author

annevk commented Apr 7, 2017

The way worklets are defined they have an event loop, so you'll have to reconcile that somehow I suppose.

@padenot
Copy link
Member

padenot commented Apr 7, 2017

Yes, this is not hard to do.

@hoch
Copy link
Member

hoch commented Apr 7, 2017

I was hoping to find some examples that describe cross-thread operation in terms of ES, but I guess a well-written prose is good enough for this case.

@annevk
Copy link
Author

annevk commented Apr 7, 2017

@hoch in ECMAScript you'd have to use a worker and postMessage(). In a specification that translates to serializing objects and queuing tasks to deserialize them.

@hoch
Copy link
Member

hoch commented Apr 7, 2017

@annevk Thanks for the clarification. We don't use either worker of postMessage, so I will try to capture the cross-thread task in a prose, as @padenot suggested above.

@annevk
Copy link
Author

annevk commented Apr 7, 2017

@hoch for the record, you can't describe anything in terms of ECMAScript in the specification. You'll always need to use some abstract concepts. There will be way too many gotchas otherwise.

@hoch
Copy link
Member

hoch commented Apr 7, 2017

@annevk I used abstract methods like Get, Construct and others in the algorithm because I thought this is the norm. Isn't this the ECMAScript?

@hoch
Copy link
Member

hoch commented Apr 7, 2017

Nvm. The document was huge, so I missed the description:

"These operations are not a part of the ECMAScript language; they are defined here to solely to aid the specification of the semantics of the ECMAScript language. Other, more specialized abstract operations are defined throughout this specification."

So I should refer them the abstract operation of ECMAScript.

Thanks @annevk!

@annevk
Copy link
Author

annevk commented Apr 7, 2017

Some of that is needed for worklets I believe, but Map() looks wrong. I'm not familiar enough with the setup to recommend something unfortunately.

@hoch
Copy link
Member

hoch commented Apr 13, 2017

The summary for AudioWG teleconference:

  1. Edit the instantiation of AWN/AWP section by replacing StructuredClone with the new StructuredSerialize and StructuredDeserialize. @domenic suggested the example steps for this.
  2. Review the abstract operation of ECMA scripts in the instantiation algorithm.
  3. Clarify how the cross-thread operation works in the instantiation algorithm.

@hoch hoch changed the title Refactor structured cloning Revise AWN/AWP instantiation algorithm: structured serialization and abstract operation Apr 13, 2017
@joeberkovitz joeberkovitz added this to the Web Audio V1 milestone Apr 13, 2017
@hoch
Copy link
Member

hoch commented May 18, 2017

@annevk I looked up the web worker's spec to see how the cross-thread operation is done during the instantiation. I could not find a clear example of such thing. Could you point me to any precedence of cross-thread tasks in the spec of worker variants?

@domenic
Copy link
Contributor

domenic commented May 18, 2017

It's a little indirect because it goes through a couple algorithms, but if you start in https://html.spec.whatwg.org/#dom-worker and follow the "run a worker" algorithm you'll see the first step of that starts doing stuff in another thread.

@hoch
Copy link
Member

hoch commented May 23, 2017

Thanks @domenic. However, the problem is that how the cross-thread interaction should be described like this:

  1. Queue a task t1 on thread A. Block thread A. (because the object construction should be atomic.)
  2. When the event loop on thread B runs t1, run the task. Then unblock thread A.
  3. On thread A, continue the rest of operation.

I couldn't find any appearance of this pattern in Web API. (but my knowledge on Web API is pretty limited) If blocking/unblocking a thread is not allowed, that means AudioWorkletNode can get away with the corresponding AudioWorkletProcessor being absent and WG needs a discussion for it.

The worker spec was certainly helpful, but it was mostly about 'doing something in a separate thread' rather than the interaction between two threads which is required for the instantiation of AudioWorkletNode and AudioWorkletProcessor.

@annevk
Copy link
Author

annevk commented May 24, 2017

@hoch you're not allowed to block the main thread. You can block other threads with caution. HTML uses "pause" for this (although it uses that for historical "main thread" usage which we really don't want to expand); I agree we need more threading infrastructure to define all these things better, ordinary prose is your best bet for now.

I'm not sure why construction needs to be atomic as all instructions to the object will come from subsequent tasks, no? So unless you somehow communicate using shared memory there's no real need for that.

@hoch
Copy link
Member

hoch commented May 24, 2017

@annevk I see. I knew it'd be controversial but was worth to try.

I'm not sure why construction needs to be atomic as all instructions to the object will come from subsequent tasks, no? So unless you somehow communicate using shared memory there's no real need for that.

td; dr: Non-blocking instantiation makes the timing of the processed audio stream from AWP ambiguous.

Long version: AWN(AudioWorkletNode) lives on the main scope/thread as a JS reference for the node and AWP(AudioWorkletProcessor) lives on the worklet scope/rendering thread to process audio stream.

If user creates an AWN and the main thread is not blocked, the creation of AWP will be async + parallel. That means we don't know when the AWP is going to be ready to process the audio stream. If everything works out, the closest timing of AWP to be up and ready would be the next render quantum. However, when AWP initialization takes a bit of time then we cannot guarantee when we will hear the sound. We already have the similar issue in PannerNode and I think this blocking node construction is the right thing to do for AudioNode. Because the timing matters.

I'll try the "loose prose" approach first.

@annevk
Copy link
Author

annevk commented May 24, 2017

So it's only synchronized upon creation and possibly parallel afterwards? I wonder if @padenot discussed this with the Servo folks at Mozilla.

@hoch
Copy link
Member

hoch commented May 24, 2017

Yes, that's correct. At least that's how it works in Chrome. The main thread initializes AudioNode and the inner object that processes audio (equivalent of AudioWorkletProcessor), and once they are ready the inner object gets pulled by the rendering thread.

@hoch hoch changed the title Revise AWN/AWP instantiation algorithm: structured serialization and abstract operation [audioworklet] Revise AWN/AWP instantiation algorithm: structured serialization and abstract operation Jun 2, 2017
@padenot
Copy link
Member

padenot commented Jun 8, 2017

It's been like that forever in the Web Audio API. You can't have guarantees about AudioNode creation, but you can schedule them properly afterwards. Were it be designed today, we would maybe make it different (create AudioNode async by returning promises?), but it's unclear that it would be better.

Indeed, you can create your AudioWorkletNode, and you can immediately start posting messages to it. Those would be appropriately sent to the AudioWorkletProcessor, that will process them when it's ready to do so, and can reply back to inform the main thread to perform synchronization via message passing.

There is no need to block any thread or to be atomic in the object creation.

@hoch, does that answer your concern? Maybe I'm missing something?

@hoch
Copy link
Member

hoch commented Jun 8, 2017

that will process them when it's ready to do so

It means the timing of an AWP being ready will be "unknown". We saw the similar problem with PannerNode in Chrome and the issue is observable; the audio processing module takes too long to be initialized so PannerNode outputs silent.

Also, what if the construction of AWP somehow fails? Somehow an AWN must render itself inactive because of the lack of a valid AWP on the other side. You can do whatever (e.g. connect/disconnect) but it won't do anything audible. Is that what we want?

can reply back to inform the main thread to perform synchronization via message passing.

Yes, this is easy enough to do, but the timing of synchronization is still undefined and this makes AudioWorkletNode different from the other nodes except for the PannerNode. (Chrome does the construction in the main thread, atomically.)

There is no need to block any thread or to be atomic in the object creation.

I think my wording was a bit extreme with "blocking the main thread", however I think the object creation should be atomic. Also the blocking cost, or the cost of making the process atomic, is supposedly minuscule (10~100µs), so I think I just have to find a better wording for the spec.

@hoch
Copy link
Member

hoch commented Jun 8, 2017

We can make AWN output silence when it's in a weird state:

  1. The construction of AWP fails.
  2. The AWP process() method throws the exception.
  3. The construction of AWP is not finished, but the AWN is connected to the graph.

Then asynchronously notify the user with:

  1. AudioWorkletNode.onstatechange (AudioNode is already EventTarget, so this makes sense.)
  2. AudioWorkletNode.port.onmessage

@padenot WDYT?

@padenot
Copy link
Member

padenot commented Jun 9, 2017

  1. This we can synchronously throw an exception, abort the construction (i.e. return null so it can't be connected to anything)
  2. This we should make the Worklet invalid and make it output silence as you note. The other option would be to make it pass-through, but I don't like that, it makes it harder to debug, in a way.
  3. is normal, it should not be an error. You create it, you connect it, etc., but it's not ready so it outputs silence like the other nodes.

For the notification, onstatechange or onerror would work. Semantically speaking, I don't think we should overload onmessage on the port, which is used for normal (author-initiated) messages.

@hoch
Copy link
Member

hoch commented Jun 9, 2017

  1. This we can synchronously throw an exception, abort the construction (i.e. return null so it can't be connected to anything)

Hmm. I thought you wanted to make the instantiation process non-atomic, asynchronous. Then how can we throw an exception synchronously from the async process?

  1. This we should make the Worklet invalid and make it output silence as you note. The other option would be to make it pass-through, but I don't like that, it makes it harder to debug, in a way.

Agreed. I like it to be silent for the same reason.

  1. isn't really controversial. :)

For the notification, onstatechange or onerror would work. Semantically speaking, I don't think we should overload onmessage on the port, which is used for normal (author-initiated) messages.

Exactly. Having onstatechange also means we need state read-only attribute in the node. Plus, onerror can be used for two purposes: 1) when the AWP construction fails and 2) when the execution of process() throws and it becomes silent.

@padenot
Copy link
Member

padenot commented Jun 12, 2017

Hmm. I thought you wanted to make the instantiation process non-atomic, asynchronous. Then how can we throw an exception synchronously from the async process?

Oops sorry I misread AWP for AWN, you're right that it would put the node in error.

hoch added a commit to hoch/web-audio-api that referenced this issue Jun 14, 2017
 - : introduce asynchronous construction process.
hoch added a commit to hoch/web-audio-api that referenced this issue Jun 14, 2017
- Introduce structured serialization.
- Remove redundant/incorrect abstract operation.
- Introduce asynchronous construction process.
hoch added a commit to hoch/web-audio-api that referenced this issue Jun 14, 2017
 - Introduce structured serialization.
 - Remove redundant/incorrect abstract operation.
 - Introduce async node construction.
@hoch
Copy link
Member

hoch commented Jun 15, 2017

Here's the work-in-progress PR, so we can discuss in F2F: Preview

@hoch hoch added the Needs Discussion The issue needs more discussion before it can be fixed. label Jun 16, 2017
@joeberkovitz
Copy link
Contributor

From F2F: We'll adopt @hoch's state enumeration proposal. We need language explaining that the running and stopped states are distinguished only by the value of the AWN's active source flag.

We also need to revise the description of how the active source flag is propagated from the rendering thread to the main thread to explicitly state that a message is queued. There are two copies of this flag: one on the AWP (to gate whether process() gets called again) and another on the AWN (to drive the internal reference that affects GC).

hoch added a commit to hoch/web-audio-api that referenced this issue Jun 21, 2017
 - Introduce structured serialization.
 - Remove redundant/incorrect abstract operation.
 - Introduce async node construction.
@hoch
Copy link
Member

hoch commented Jun 23, 2017

The instantiation of AWN/AWP requires some thread-specific information for each object. For this purpose, we need some synchronization steps when addModule() call resolves. So I filed an issue for the worklet spec: w3c/css-houdini-drafts#418.

hoch added a commit to hoch/web-audio-api that referenced this issue Jun 30, 2017
 - Introduce structured serialization.
 - Remove redundant/incorrect abstract operation.
 - Introduce async node construction.
@joeberkovitz
Copy link
Contributor

Fixed by #1265

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

5 participants