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

Add AudioWorklet section #869

Merged
merged 14 commits into from Aug 19, 2016
Merged

Add AudioWorklet section #869

merged 14 commits into from Aug 19, 2016

Conversation

hoch
Copy link
Member

@hoch hoch commented Jun 29, 2016

PTAL.

Preview: http://hoch.github.io/web-audio-api/#AudioWorklet

I did not remove the AudioWorker section yet per @rtoy's suggestion. Once we settle on this change, I will submit another PR to remove the section.

The <dfn>AudioWorklet</dfn> interface
</h2>
<p>
The AudioWorklet object allows importing user-supplied Javascript code
Copy link
Member

@rtoy rtoy Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the style is to use either <code>AudioWorklet</code> or <a><code>AudioWorklet</code></a> if you want a link.

Copy link
Member Author

@hoch hoch Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph is about AudioWorklet. Does it still need to be linked to itself?

Copy link
Member Author

@hoch hoch Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix the rest of <a> to <code>.

Copy link
Member Author

@hoch hoch Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Represents the default value of the parameter. If this value is
out of the range of float data type or the range defined by
<code>minValue</code> and <code>maxValue</code>, an
NotSupportedError exception MUST be thrown.
Copy link
Member

@padenot padenot Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markup on the exception name.

@hoch
Copy link
Member Author

@hoch hoch commented Jul 27, 2016

@domenic Can I ask you the TAG review on this new section? You can preview the change at my forked repo.

http://hoch.github.io/web-audio-api/#AudioWorklet

@domenic
Copy link
Contributor

@domenic domenic commented Jul 27, 2016

I'm happy to review, and love worklets, although I am no longer on the TAG :).

@hoch
Copy link
Member Author

@hoch hoch commented Jul 27, 2016

A fresh pair of eyes wouldn't hurt! Please take a look if you're willing. :) I just pinged @slightlyoff for the TAG review.

@domenic
Copy link
Contributor

@domenic domenic commented Jul 27, 2016

Review notes:

  • Worklet isn't linking to its definition at https://drafts.css-houdini.org/worklets/
  • The importing stuff seems to be duplicating the work in the worklet spec?
  • Transferable is no longer defined. Just use object
  • The syntax Float32Array[][] is invalid Web IDL. Not sure what it was supposed to be. sequence<sequence<Float32Array>>??
  • readonly attribute static is incorrect. I think you meant static readonly attribute.
  • An attribute should not return a sequence<>, since that will cause it to return a new sequence every time. I think you want [SameObject] FrozenArray? Except maybe this shouldn't be in the class at all, and is meant to be taken care of by a subclass.
  • Maybe the examples should be at the top of the section, as an intro? Or maybe that doesn't fit with the rest of the spec.
  • In general I don't understand why AudioWorkletNode constructs two other objects. What does it do with them? Where are they stored?
  • "via the algorithm defined by the [!Worker] specification." seems broken. You want to reference https://html.spec.whatwg.org/multipage/#dom-messageport-postmessage which has seen some fairly recent important normative changes.
  • In the AudioWorkletNode constructor definition, step 2.1 talks about subclasses; I don't understand. What subclass?
  • "This method gets executed isochronously by the audio rendering thread. " is not a method definition. What does the method actually do? I think it's supposed to implemented by the subclass, in which case it should not be in the IDL at all.
  • Can "registerAudioWorkletProcessor" be renamed "registerProcessor"? What other types of processors would an AudioWorklet be registering?
    In general, studying https://drafts.css-houdini.org/css-paint-api-1/#paint-worklet might be helpful. In particular I can't find any concept analogous to that spec's "paint image definition", which seems kind of important. The whole storage setup seems entirely glossed over, with a simple sentence "Registers a definition of a class that is derived from AudioWorkletProcessor" for the method. You need something more like https://drafts.css-houdini.org/css-paint-api-1/#dom-paintworkletglobalscope-registerpaint.

@hoch
Copy link
Member Author

@hoch hoch commented Jul 28, 2016

Thanks @domenic! I will work on them.

<section>
<h2 id="AudioWorkletNodeOptions">
<dfn>AudioWorkletNodeOptions</dfn>
</h2>
Copy link
Contributor

@joeberkovitz joeberkovitz Jul 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this somehow derive from the AudioNodeOptions dictionary in #863?

@hoch
Copy link
Member Author

@hoch hoch commented Aug 5, 2016

@domenic Here's my response for your review.

Maybe the examples should be at the top of the section, as an intro? Or maybe that doesn't fit with the rest of the spec.

Although this needs to be discussed with WG, I actually like the idea.

In the AudioWorkletNode constructor definition, step 2.1 talks about subclasses; I don't understand. What subclass? In general I don't understand why AudioWorkletNode constructs two other objects. What does it do with them? Where are they stored?

It's a subclass derived from AudioWorkletProcessor. This concept of 'two-objects-in-different-threads' is quite new to the world, even for the Worklet. However, we need two objects that represents the 'main thread interface' and the 'audio thread interface' respectively. It might be nice to have your perspective on this because the discussion has been going on only inside of WG.

I think it's supposed to implemented by the subclass, in which case it should not be in the IDL at all.

Hmm. This is surprising because this process method is the core of AudioWorkletProcessor. As you pointed this must be implemented by the subclass. If you don't, you get the silence out of this node. (or should we throw? this needs to be defined too.) How can we capture the interface of this method if we cannot put it on the IDL?

@domenic
Copy link
Contributor

@domenic domenic commented Aug 9, 2016

It's a subclass derived from AudioWorkletProcessor. This concept of 'two-objects-in-different-threads' is quite new to the world, even for the Worklet. However, we need two objects that represents the 'main thread interface' and the 'audio thread interface' respectively. It might be nice to have your perspective on this because the discussion has been going on only inside of WG.

Two objects in different threads seem fine. The problem is that the spec as written creates the objects and then does nothing with them. This means that they can be garbage collected immediately. You need to specify where to store them and what holds references to them and uses them later.

Hmm. This is surprising because this process method is the core of AudioWorkletProcessor. As you pointed this must be implemented by the subclass. If you don't, you get the silence out of this node. (or should we throw? this needs to be defined too.) How can we capture the interface of this method if we cannot put it on the IDL?

The IDL is just instructions for how bindings generators in implementations should perform conversions and create APIs. It doesn't have anything to do with the web developer-facing interface and requirements for using the API. That is better documented in, well, documentation, not in normative interface definition language.

Since the implementation should not have a process method, it must not appear in the IDL.

@hoch hoch merged commit 5cadcb2 into WebAudio:gh-pages Aug 19, 2016
1 check passed
@hoch
Copy link
Member Author

@hoch hoch commented Aug 19, 2016

Merged this PR after the LGTM from @joeberkovitz and @rtoy. As @padenot suggested, we decided to seek TAG review after adding the section into the spec because the PR is too bloated with comments.

@slightlyoff
Copy link

@slightlyoff slightlyoff commented Sep 9, 2016

Apologies for the slow response.

Reading through the comments here and the draft spec text, a few things in addition to @domenic's great comments:

  • The definition of import in the [Worklet Spec] is:
interface Worklet {
    [NewObject] Promise<void> import(USVString moduleURL);
};

whereas the interface in WebAudio uses DOMString:

interface AudioWorklet : Worklet {
    Promise<void> import (DOMString moduleUrl);
};
  • Perhaps I'm being daft, but the number of potential AudioWorkletGlobalScope instances that can be running at the same time doesn't seem to be spelled out anywhere. The example code at the front of the section makes me think that it could be any of:
    • One per AudioWorkletNode
    • One per "Main global scope"
    • One per AudioWorkletProcessor subclass type ...although I assume it's the first?
  • What's the lifetime policy? When can these worklets be shut down and/or restarted?
  • What happens if an exception is thrown inside the worklet while processing audio? Anything? Nothing?
  • Like @domenic, I think it'd be great if the example code were pulled out into a separate Use Cases or Explainer document.
  • ScriptAudioProcessorNode is still defined, but deprecated. This seems tenuous. What is usage currently like? Can it be removed from the spec?

@rtoy
Copy link
Member

@rtoy rtoy commented Sep 9, 2016

On Thu, Sep 8, 2016 at 6:07 PM, Alex Russell notifications@github.com
wrote:

Apologies for the slow response.

Reading through the comments here and the draft spec text, a few things in
addition to @domenic https://github.com/domenic's great comments:

  • The definition of import in the [Worklet Spec] is:

interface Worklet {
[NewObject] Promise import(USVString moduleURL);
};

whereas the interface in WebAudio uses DOMString:

interface AudioWorklet : Worklet {
Promise import (DOMString moduleUrl);
};

  • Perhaps I'm being daft, but the number of potential
    AudioWorkletGlobalScope instances that can be running at the same time
    doesn't seem to be spelled out anywhere. The example code at the front of
    the section makes me think that it could be any of:
    • One per AudioWorkletNode
    • One per "Main global scope"
    • One per AudioWorkletProcessor subclass type ...although I assume
      it's the first?
  • What's the lifetime policy? When can these worklets be shut down
    and/or restarted?

​We are trying to figure out the lifetime. Ideally it would behave like a
native node, but we're not sure exactly how to specify that for the worklet.

  • What happens if an exception is thrown inside the worklet while
    processing audio? Anything? Nothing?
  • Like @domenic https://github.com/domenic, I think it'd be great if
    the example code were pulled out into a separate Use Cases or Explainer
    document.
  • ScriptAudioProcessorNode is still defined, but deprecated. This
    seems tenuous. What is usage currently like? Can it be removed from the
    spec?

​My take on this is that we need to keep ScriptAudioProcessorNode around
until all relevant browsers have at least an intent to remove it.
Otherwise, it seems you'd be in a strange state where it's still in
browsers but you can't find out where it's documented.

https://www.chromestatus.com/metrics/feature/timeline/popularity/646 says
about .003% of pages. To put that in perspective, 0.4% of pages use
AudioContext (
https://www.chromestatus.com/metrics/feature/timeline/popularity/652).
However, I think that is not truly representative. I suspect that many of
the uses of ScriptProcessorNode are required because there is no other way
to do the processing with existing nodes.

For chrome, I'm expecting that once we get a good part of AudioWorklet
implemented, we'll start pushing deprecation messages for
ScriptProcessorNode and then try to remove it a few months after
AudioWorklet is officially shipped. Those are the tentative plans. Things
may change.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#869 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAofPPnFdFrx1Eknebr0l2l6hX1TIrp6ks5qoLFjgaJpZM4JBSo7
.

Ray

@padenot
Copy link
Member

@padenot padenot commented Sep 9, 2016

Also there is to be a very solid polyfill for it, so people can drop a
file in and have their page (re-)working in seconds, and then maybe fix
their applications to use an AudioWorklet.

Because of the way the Web Audio API works, quite lot of applications
can only use ScriptProcessors to achieve their goal.

This is going to be complicated, and will require coordinated
communication to avoid breaking everything. I'm hopeful, though.

On Fri, Sep 9, 2016, at 04:26 AM, rtoy wrote:

On Thu, Sep 8, 2016 at 6:07 PM, Alex Russell
notifications@github.com
wrote:

Apologies for the slow response.

Reading through the comments here and the draft spec text, a few
things in
addition to @domenic https://github.com/domenic's great comments:

  • The definition of import in the [Worklet Spec] is:

interface Worklet {
[NewObject] Promise import(USVString moduleURL);
};

whereas the interface in WebAudio uses DOMString:

interface AudioWorklet : Worklet {
Promise import (DOMString moduleUrl);
};

  • Perhaps I'm being daft, but the number of potential
    AudioWorkletGlobalScope instances that can be running at the
    same time
    doesn't seem to be spelled out anywhere. The example code at the
    front of
    the section makes me think that it could be any of:

    • One per AudioWorkletNode
    • One per "Main global scope"
    • One per AudioWorkletProcessor subclass type ...although I
      assume
      it's the first?
  • What's the lifetime policy? When can these worklets be shut
    down
    and/or restarted?

    We are trying to figure out the lifetime. Ideally it would
    behave like a
    native node, but we're not sure exactly how to specify that for the
    worklet.

    • What happens if an exception is thrown inside the worklet
      while
      processing audio? Anything? Nothing?
  • Like @domenic https://github.com/domenic, I think it'd be
    great if
    the example code were pulled out into a separate Use Cases or
    Explainer
    document.

  • ScriptAudioProcessorNode is still defined, but deprecated.
    This
    seems tenuous. What is usage currently like? Can it be removed
    from the
    spec?

    My take on this is that we need to keep
    ScriptAudioProcessorNode around
    until all relevant browsers have at least an intent to remove it.
    Otherwise, it seems you'd be in a strange state where it's still in
    browsers but you can't find out where it's documented.

https://www.chromestatus.com/metrics/feature/timeline/popularity/646
says
about .003% of pages. To put that in perspective, 0.4% of pages use
AudioContext (
https://www.chromestatus.com/metrics/feature/timeline/popularity/652).
However, I think that is not truly representative. I suspect that
many of
the uses of ScriptProcessorNode are required because there is no
other way
to do the processing with existing nodes.

For chrome, I'm expecting that once we get a good part of
AudioWorklet
implemented, we'll start pushing deprecation messages for
ScriptProcessorNode and then try to remove it a few months after
AudioWorklet is officially shipped. Those are the tentative
plans. Things
may change.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#869 (comment)
,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAofPPnFdFrx1Eknebr0l2l6hX1TIrp6ks5qoLFjgaJpZM4JBSo7
.

--
Ray

— You are receiving this because you were mentioned. Reply to this
email directly, view it on GitHub[1], or mute the thread[2].

Links:

  1. #869 (comment)
  2. https://github.com/notifications/unsubscribe-auth/AAQglaZC9c80yEv3pW3Df6urPTv8dHGqks5qoMPhgaJpZM4JBSo7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants