Intent to Implement and Ship: WebAudio: AudioScheduledSourceNode base class

[Raymond Toy]

Contact emails

rt...@chromium.org, hong...@chromium.org

Spec

https://webaudio.github.io/web-audio-api/

https://webaudio.github.io/web-audio-api/#idl-def-AudioBufferSourceNode

Summary

Add an AudioScheduledSourceNode class to capture the common features of the source nodes that can be scheduled: AudioBufferSourceNode, ConstantSourceNode, and OscillatorNode.  These are now subclasses of AudioScheduledSourceNode.

Motivation

The common methods (start, stop) and attributes (onended) are moved to AudioScheduledSourceNode to make it clearer that all source nodes share the same basic methods and attributes.

Interoperability and Compatibility Risk

This is a new addition to the WebAudio spec agreed upon by the working group.  We expect all browsers to eventually implement this.

There is some risk in this change because methods that used to exist for, say, an OscillatorNode are now on AudioScheduledSourceNode.  Uses of things like OscillatorNode.prototype.hasOwnProperty(“start”) will now fail.  An http-archive search for such usages show no occurrences at all (as of today).

Ongoing technical constraints

None

Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes.

OWP launch tracking bug

http://crbug.com/661207

Link to entry on the feature dashboard

https://www.chromestatus.com/features/5644431130624000

Requesting approval to ship?

Yes

[Chris Harrelson]

LGTM1

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Philip Jägenstedt]

Thanks for the httparchive research, hope this one works out without fallout.

Have you begun the implementation of this? Given that both AudioBufferSourceNode and its parent interface AudioScheduledSourceNode has start() methods, I wonder how it'll actually work. I don't believe there's any overload resolution mechanism that works across prototypes. Are you sure that this should work per WebIDL, and can you see if it'd actually work?

It could be that the spec has to change to move start() to AudioScheduledSourceNode only, but to throw exceptions depending on what the "context object" (this) is and how many arguments were passed. Would the change still be desirable if so?

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Raymond Toy]

On Thu, Nov 10, 2016 at 6:53 AM, Philip Jägenstedt <foo...@chromium.org> wrote:

Thanks for the httparchive research, hope this one works out without fallout.

Yeah, now that we know how these seemingly harmless changes can affect pages, we wanted to take some precautions. :-) 

Have you begun the implementation of this? Given that both AudioBufferSourceNode and its parent interface AudioScheduledSourceNode has start() methods, I wonder how it'll actually work. I don't believe there's any overload resolution mechanism that works across prototypes. Are you sure that this should work per WebIDL, and can you see if it'd actually work?

The current CL (not quite yet ready for review) is at  https://codereview.chromium.org/2471353004/.  It seems to work, but I need to do more testing, especially in light of your comments.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Philip Jägenstedt]

On Thu, Nov 10, 2016 at 8:44 PM Raymond Toy <rt...@chromium.org> wrote:

On Thu, Nov 10, 2016 at 6:53 AM, Philip Jägenstedt <foo...@chromium.org> wrote:

Thanks for the httparchive research, hope this one works out without fallout.

Yeah, now that we know how these seemingly harmless changes can affect pages, we wanted to take some precautions. :-) 

Have you begun the implementation of this? Given that both AudioBufferSourceNode and its parent interface AudioScheduledSourceNode has start() methods, I wonder how it'll actually work. I don't believe there's any overload resolution mechanism that works across prototypes. Are you sure that this should work per WebIDL, and can you see if it'd actually work?

The current CL (not quite yet ready for review) is at  https://codereview.chromium.org/2471353004/.  It seems to work, but I need to do more testing, especially in light of your comments.

What should myAudioBufferSourceNode.start() do? It currently doesn't throw TypeError, but I think it will with the changed spec, but perhaps not intentionally?

The weird thing is that AudioScheduledSourceNode.prototype.start.call(myAudioBufferSourceNode) would mean something different.

If you'd implement it as virtual+override in C++, then probably the only way to do it spec-side is to have only a start() method on the base clase and switch on the type of the context object in the prose, as there's no such thing as virtual.

Anyway, LGTM2 while working out those details in spec and implementation.

[Domenic Denicola]

From: Philip Jägenstedt [mailto:foo...@chromium.org]

> What should myAudioBufferSourceNode.start() do? It currently doesn't throw TypeError, but I think it will with the changed spec, but perhaps not intentionally?
>
> The weird thing is that AudioScheduledSourceNode.prototype.start.call(myAudioBufferSourceNode) would mean something different.
>
> If you'd implement it as virtual+override in C++, then probably the only way to do it spec-side is to have only a start() method on the base clase and switch on the type of the context object in the prose, as there's no such thing as virtual.

Is there a spec issue where this is being discussed?

In lieu of one: I think the proposed pattern in the CL is totally reasonable and idiomatic for normal JavaScript classes. The Web IDL proposed has the same semantics as this very normal JavaScript:

class Foo { x() { } }
class Bar extends Foo { x() { } }

i.e. method overriding in a derived class. There's no need to collocate to the derived class and do a switch on the type of the context object.

The implications for Bar.prototype.x.call(myFoo) are expected and normal; you are explicitly applying the base class method to the derived class, which is expected to apply the base class logic. That is not a problem.

[Raymond Toy]

On Fri, Nov 11, 2016 at 7:36 AM, Domenic Denicola <d...@domenic.me> wrote:

From: Philip Jägenstedt [mailto:foo...@chromium.org]

> What should myAudioBufferSourceNode.start() do? It currently doesn't throw TypeError, but I think it will with the changed spec, but perhaps not intentionally?
>
> The weird thing is that AudioScheduledSourceNode.prototype.start.call(myAudioBufferSourceNode) would mean something different.
>
> If you'd implement it as virtual+override in C++, then probably the only way to do it spec-side is to have only a start() method on the base clase and switch on the type of the context object in the prose, as there's no such thing as virtual.

Is there a spec issue where this is being discussed?

The initial discussion started here: https://github.com/WebAudio/web-audio-api/pull/902#r72907030, with the actual issue in https://github.com/WebAudio/web-audio-api/issues/960 

[Philip Jägenstedt]

I'm certainly not saying that this pattern is always suspect and wrong, just wondering if they were in fact hoping that the same method overload resolution would apply as when defining two methods of the same name on the same interface. That there's a case that's begun throwing that didn't before is what made me suspicious.

Maybe all arguments should simply be optional on AudioBufferSourceNode's start method?

[Raymond Toy]

On Fri, Nov 11, 2016 at 1:29 AM, Philip Jägenstedt <foo...@chromium.org> wrote:

On Thu, Nov 10, 2016 at 8:44 PM Raymond Toy <rt...@chromium.org> wrote:

On Thu, Nov 10, 2016 at 6:53 AM, Philip Jägenstedt <foo...@chromium.org> wrote:

Thanks for the httparchive research, hope this one works out without fallout.

Yeah, now that we know how these seemingly harmless changes can affect pages, we wanted to take some precautions. :-) 

Have you begun the implementation of this? Given that both AudioBufferSourceNode and its parent interface AudioScheduledSourceNode has start() methods, I wonder how it'll actually work. I don't believe there's any overload resolution mechanism that works across prototypes. Are you sure that this should work per WebIDL, and can you see if it'd actually work?

The current CL (not quite yet ready for review) is at  https://codereview.chromium.org/2471353004/.  It seems to work, but I need to do more testing, especially in light of your comments.

What should myAudioBufferSourceNode.start() do? It currently doesn't throw TypeError, but I think it will with the changed spec, but perhaps not intentionally?

This should start the node playing immediately. Internally, there isn't much change.  There was already an AudioScheduledSource class that contained the common start and stop methods used by all sources. 

[Philip Jägenstedt]

On Fri, Nov 11, 2016 at 4:59 PM Raymond Toy <rt...@chromium.org> wrote:

On Fri, Nov 11, 2016 at 1:29 AM, Philip Jägenstedt <foo...@chromium.org> wrote:

On Thu, Nov 10, 2016 at 8:44 PM Raymond Toy <rt...@chromium.org> wrote:

On Thu, Nov 10, 2016 at 6:53 AM, Philip Jägenstedt <foo...@chromium.org> wrote:

Thanks for the httparchive research, hope this one works out without fallout.

Yeah, now that we know how these seemingly harmless changes can affect pages, we wanted to take some precautions. :-) 

Have you begun the implementation of this? Given that both AudioBufferSourceNode and its parent interface AudioScheduledSourceNode has start() methods, I wonder how it'll actually work. I don't believe there's any overload resolution mechanism that works across prototypes. Are you sure that this should work per WebIDL, and can you see if it'd actually work?

The current CL (not quite yet ready for review) is at  https://codereview.chromium.org/2471353004/.  It seems to work, but I need to do more testing, especially in light of your comments.

What should myAudioBufferSourceNode.start() do? It currently doesn't throw TypeError, but I think it will with the changed spec, but perhaps not intentionally?

This should start the node playing immediately. Internally, there isn't much change.  There was already an AudioScheduledSource class that contained the common start and stop methods used by all sources.

Ah, so the problem is that the CL leaves all of the arguments as optional, but the spec has them as required. The spec needs to change, then.

[Raymond Toy]

AudioBufferSource.start has required arguments to make it distinctly different from AudioScheduledSourceNode.start, which has one optional arg.  I think the CL should match this.  Unless the spec is actually wrong here.

[Philip Jägenstedt]

I'm afraid the spec is wrong. If one drops "optional" from the first two arguments, you'll get this:

Uncaught TypeError: Failed to execute 'start' on 'AudioBufferSourceNode': 2 arguments required, but only 0 present.

[Raymond Toy]

Ah.  Right. I'll file an issue to fix this.

[Raymond Toy]

The spec issue is https://github.com/WebAudio/web-audio-api/issues/1070.  Hopefully someone will review the corresponding pull request soon.

[Rick Byers]

LGTM3 (no need to wait for the PR to land IMHO - it's clearly a bug)