WebAudio / web-audio-api

The Web Audio API v1.0, developed by the W3C Audio WG
https://webaudio.github.io/web-audio-api/
Other
1.05k stars 166 forks source link

Property bag for channelCount #697

Closed rtoy closed 8 years ago

rtoy commented 8 years ago

Should all constructors allow channelCount, channelCountMode, and channelIntepretation properties in the constructors?

Probably. Presumably they're ignored (like AudioBufferSource) or throw if you set them to invalid values.

696

hoch commented 8 years ago

I prefer to be able to set them in the property bag. Currently the code quickly becomes messy with lots of repetition.

For throwing exceptions, I also prefer to 'fail early with a specific error message'.

hoch commented 8 years ago

Well, to get the ball rolling -

dictionary AudioNodeOptions {
    numberOfInputs = 1;
    numberOfOutputs = 1;
    channelCount = 2;
    channelCountMode = "max";
    channelInterpretation = "speakers";
};

[Constructor(AudioContext audioContext, optional AudioNodeOptions audioNodeOptions)]
partial interface AudioNode {
}

An example would be:

var context = new AudioContext();
var gain = new GainNode(context, {
    channelCountMode: 'explicit',
    channelInterpretation: 'discrete'
});

After some thoughts, it actually brings up more questions.

  1. Unlike the factory methods (i.e. context.createFoo()), the constructor method without '...Node' in the name seems to too short for some nodes. For example, new Gain() or new GainNode(), which one de we prefer?
  2. The destination node automatically is instantiated at the moment of AudioContext construction, so we might want to include these options into the AudioContext's constructor option.
billhofmann commented 8 years ago

I think new GainNode() is more "fluent".

rtoy commented 8 years ago

I don't think AudioNodeOptions should have numberOfInputs and numberOfOutputs. For most nodes these aren't changeable. Only splitter/merger and script processor have such options, right?

Also, is GainNode too generic? Is AudioGainNode too verbose?

hoch commented 8 years ago

@bill-hofmann Yes. That's what I am thinking too. However, we changed the factory method from 'createGainNode()' to 'createGain()' so just wanted to ask what we want.

@rtoy I just put them there for the formality, but we still need to decide how a node should behave when invalid properties are given. Also AudioGainNode() is something I would like to avoid...

rtoy commented 8 years ago

What do you mean by invalid properties? Shouldn't unknown properties be ignored and invalid values for a property signal the same errors (or not) as setting the attributes would?

hoch commented 8 years ago

By 'an invalid property', I meant something like passing a number for a string-type attribute. Do we silently ignore this error without throwing anything? Hmm, personally I don't think that's the right thing to do, but I would like to have opinions from TAG.

padenot commented 8 years ago

This is something that would be taken care of by WebIDL.

mdjp commented 8 years ago

F2F: Yes we will include these properties inthe property bag

rtoy commented 8 years ago

I think the AudioNodeOptions dictionary should be separated into two parts:

dictionary AudioNodeIOCountOptions {
    unsigned numberOfInputs;
    unsigned numberOfOutputs;
}

and

dictionary AudioNodeChannelOptions {
    unsigned channelCount = 2;
    channelCountMode = "max";
    channelInterpretation = "speakers";
}

Why? By combining them into one, it looks like you can change numberOfInputs and numberOfOutputs for any node. But for most nodes you can't. I think ChannelMerger, ChannelSplitter, and ScriptProcessor are the only nodes that let you change either of these.

hoch commented 8 years ago

Note that it conflicts with what I have in here.

I can see why it is necessary, but not sure about having two different dictionaries for such small task.

rtoy commented 8 years ago

If you don't separate them, you get to explain in almost every constructor why you can have {numberOfInputs: 5} and not have 5 inputs to that node.

Also the constructor option for PannerNode probably shouldn't derive from AudioNodeChannelOptions since you can't change any of them.

And it's a bit confusing for ChannelMerger. You can set the numberOfInputs, but numberOfOutputs has no effect. Perhaps ChannelMergerOptions shouldn't derive from AudioNodeIOCountOption. Similarly for ChannelSplitterNode.

rtoy commented 8 years ago

After thinking this over, I think there should not be an AudioIOCountOptions dictionary. For every node, the constructor options dictionary should inherit from AudioNodeChannelOptions to allow the constructor to set the appropriately. Of course, the normal rules for the node should apply. Thus, you can set channelCount: 7 in the dictionary, but this would throw an error when constructing a PannerNode since that's illegal.

For ChannelMergerNode and ChannelSplitterNode, the constructor dictionaries would look something like

dictionary ChannelMergerOptions : AudioNodeChannelOptions {
    unsigned numberOfOutputs;
}