Skip to content

Specify rules to determine the output channel count for AudioNodes that have a tail time, when the input channel count changes#1787

Merged
hoch merged 9 commits intoWebAudio:masterfrom
padenot:channels-tail
Nov 16, 2018
Merged

Specify rules to determine the output channel count for AudioNodes that have a tail time, when the input channel count changes#1787
hoch merged 9 commits intoWebAudio:masterfrom
padenot:channels-tail

Conversation

@padenot
Copy link
Copy Markdown
Member

@padenot padenot commented Nov 8, 2018

This fixes #1719.


Preview | Diff

…at have a tail time, when the input channel count changes.

This fixes WebAudio#1719.
@padenot padenot self-assigned this Nov 8, 2018
@padenot padenot requested a review from hoch November 8, 2018 11:06
@padenot
Copy link
Copy Markdown
Member Author

padenot commented Nov 8, 2018

@karlt, can you have a look at this?

Comment thread index.bs Outdated
<h3 id="channels-tail-time">
Implication of tail-time on input and output channel count</h3>

When an {{AudioNode}} has a non-null <a>tail-time</a>, and an output channel
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is non-null a common spec language?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's regular English I think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So it could be zero, but must be defined. Right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"null" is an English word, but it is somewhat unusual.

Most uses of "tail-time" in the spec are for the "tail-time reference".
The exception is https://webaudio.github.io/web-audio-api/#AudioWorkletProcessor-methods which uses the expressions "a tail-time after their inputs are disconnected" and "tail-time interval".

I'd be inclined to avoid using "null" because that sounds like a JS value, which this is not.

Perhaps it would be fine to use "a tail-time reference" here, because that corresponds with "has any internal processing state which has not yet been emitted", but I guess it is more natural and consistent with the text below to use "a non-zero tail-time" or "a non-zero tail-time interval".

Comment thread index.bs Outdated
Comment thread index.bs Outdated
Comment thread index.bs
Comment thread index.bs Outdated

Note: Intuitively, this allows not losing stereo information as part of
processing: when multiple audio blocks of different channel count contribute to
an output block, then the output block's channel count is a superset of the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's block here? I believe it's a block of frames produced by the renderer, but It might not be clear for readers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, I'll make it clearer.

Comment thread index.bs
Comment thread index.bs Outdated
<h3 id="channels-tail-time">
Implication of tail-time on input and output channel count</h3>

When an {{AudioNode}} has a non-null <a>tail-time</a>, and an output channel
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"null" is an English word, but it is somewhat unusual.

Most uses of "tail-time" in the spec are for the "tail-time reference".
The exception is https://webaudio.github.io/web-audio-api/#AudioWorkletProcessor-methods which uses the expressions "a tail-time after their inputs are disconnected" and "tail-time interval".

I'd be inclined to avoid using "null" because that sounds like a JS value, which this is not.

Perhaps it would be fine to use "a tail-time reference" here, because that corresponds with "has any internal processing state which has not yet been emitted", but I guess it is more natural and consistent with the text below to use "a non-zero tail-time" or "a non-zero tail-time interval".

Comment thread index.bs Outdated
Comment thread index.bs Outdated

When an {{AudioNode}} has a non-null <a>tail-time</a>, and an output channel
count that depends on the input channels count, any change in input channel
MUST take the {{AudioNode}}'s <a>tail-time</a> into account
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about "the AudioNode's tail-time must be taken into account when the input channel count changes"?

The thing (subject of verb) that "must take the AudioNode's tail-time into account" is really the implementation, or implementation of the node, or perhaps loosely the node, not the change in input channel count. Using the passive voice avoids having to say.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, better, thanks.

Comment thread index.bs Outdated

- For a {{ConvolverNode}}, since the number of input channel determines the
<a href="#Convolution-channel-configurations">audio processing algorithm</a>
to use, it MUST immediately start outputing two channels.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An immediate increase in output channel count is the general case on increase in input channel count, and must also happen on BiquadFilterNode, IIRFilterNode, and WaveShaperNode because the input with greater channel count will affect the immediate output buffer.

I don't think it is necessary to special case ConvolverNode in normative text, but a note seems helpful to indicate that this affects ConvolverNode with only mono response.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've move or less reversed the prose, only mentioning ConvolverNode in the informative note.

Comment thread index.bs Outdated
Comment thread index.bs Outdated
Comment thread index.bs Outdated
karlt and others added 5 commits November 9, 2018 11:33
Co-Authored-By: padenot <paul@paul.cx>
Co-Authored-By: padenot <paul@paul.cx>
Co-Authored-By: padenot <paul@paul.cx>
Co-Authored-By: padenot <paul@paul.cx>
@padenot
Copy link
Copy Markdown
Member Author

padenot commented Nov 9, 2018

I'm trying this github suggestions things out, but had to made adjustments locally, so I'll have to push a merge commit, hopefully everything is still readable.

Comment thread index.bs
Copy link
Copy Markdown
Contributor

@karlt karlt left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@padenot
Copy link
Copy Markdown
Member Author

padenot commented Nov 16, 2018

@hoch, do you want to have a final look at this before merging?

Copy link
Copy Markdown
Member

@hoch hoch left a comment

Choose a reason for hiding this comment

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

LGTM

@hoch hoch merged commit ca7227b into WebAudio:master Nov 16, 2018
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.

channel count changes in filter nodes with tail time

3 participants