ConvolverNode: disallow state modification, describe algorithm, clarify language

[olivierthereaux]

Originally reported on W3C Bugzilla ISSUE-17359 Tue, 05 Jun 2012 11:48:20 GMT
Reported by Philip Jägenstedt
Assigned to

Audio-ISSUE-72 (ConvolverNodeState): ConvolverNode state modification [Web Audio API]

http://www.w3.org/2011/audio/track/issues/72

Raised by: Philip Jägenstedt
On product: Web Audio API

When buffer and normalize are modified, when does it take effect? If modifications are not applied atomically, it's possible to get spikes (or dropouts) depending on the order of setting and the previous state.

Related: https://www.w3.org/2011/audio/track/issues/28

Further, setting normalize to true is defined using the phrasing "when the buffer atttribute is set", which implies that the order of setting the attributes matters. However, no such order-dependence exists when setting normalize to false.

If an order-dependence is intended, it ought to be changed to either commit atomically after the script thread has finished, or we should have a setter taking both a buffer and a normalize flag.

[olivierthereaux]

Original comment by Chris Rogers on W3C Bugzilla. Fri, 08 Jun 2012 23:06:38 GMT

Much more detail added here:
https://dvcs.w3.org/hg/audio/rev/4df179094971

[olivierthereaux]

Original comment by Marcus Geelnard (Opera) on W3C Bugzilla. Tue, 12 Jun 2012 09:13:59 GMT

The change mostly covers the questions asked. Feedback on the new changes:

• The algorithm defined in the C++ (?) function calculateNormalizationScale would be much better defined in pseudo code, and could probably be more compact. The code also seems to depend on internal data structures specific to a particular implementation.
• The text "A mono, stereo, or 4-channel AudioBuffer containing the (possibly multi-channel) impulse response" is confusing. What does "possibly multi-channel" mean in this context? Can a mono AudioBuffer be multi-channel?
• Editorial: "Normative requirements for multi-channel convolution matrixing are described here". Please don't use "here"-links.
• It is unspecified what should happen if you first set the buffer attribute to an AudioBuffer "buf", and later make changes to your locally referenced "buf" (or, for that matter, make modifications directly to the array returned by buffer.getChannelData(k)).

[olivierthereaux]

Original comment by Marcus Geelnard (Opera) on W3C Bugzilla. Tue, 12 Jun 2012 09:19:09 GMT

...additionally, I'd suggest that the buffer + normalize attributes are replaced by a single setImpulseResponse(AudioBuffer buffer, boolean normalize) method. That would make the interface much clearer, and avoid possible problems with modifying the AudioBuffer after setting it.

[olivierthereaux]

Original comment by Philip Jägenstedt on W3C Bugzilla. Tue, 12 Jun 2012 09:41:54 GMT

Indeed, transferring ownership of the buffer and neutering it would avoid any problems with later modification.

[joeberkovitz]

See #288 for potentially related questions.

[joeberkovitz]

Since #288 is resolved in terms of disallowing ABSN buffer setting dynamically, propose the same approach here: do not allow buffer or normalize flags to be set more than once (and perhaps require them both to be set in a single method)

[padenot]

I think this makes sense.

Some of Marcus' concerns have been fixed with the neutering changes. We need to address the rest of the comments, that are still valid:

• Rewrite the algorithm in js, without relying on WebKit's data structures
• Clarify the prose: "possibly multi-channel" is redundant
• Inline the requirements for the multi-channel convolution matrixing. This is clearly normative text, and belongs to the same document.
• Add the requirement that the buffer and the normalization can only be set once

Do we still want to add a method to do the setting of the buffer and the normalization in one go, or do we think that this ship has sailed?

[joeberkovitz]

I'd guess that the ship has sailed on the current API, that new method was probably just some wishful thinking on my part :-)

About the other comments: I will broaden the title of this issue.

[notthetup]

One fallout of restricting the allocation of the buffer property more than once, would be the inability of changing the IR based on some user input like 'head motion'. Currently this kinda works, but it glitches when a new ReverbConvolver is created (when the buffer property is set).

The work around suggested by someone during the WebAudio Conference was to crossfade between two ConvolverNodes. Which kinda works for now.

The (long winded) question here would be, in case of the inability to set buffer more than once, such a use-case would need to re-create multiple ConvolverNodes per second. Would that be a performance issue?

[joeberkovitz]

Marking as needing WG review on the next call because this set of decisions is turning out to be non-obvious.

There may be yet other solutions worth considering, like allowing buffer/normalize changes but requiring atomic application to the node (this was proposed by Marcus but did not get discussed later in the thread).

Any glitching in the current Convolver implementations that is the result of spec problems (as opposed to bugs) please mention in this thread.

[rtoy]

On Mon, May 4, 2015 at 9:57 PM, Chinmay Pendharkar <notifications@github.com

wrote:

One fallout of restricting the allocation of the buffer property more
than once, would be the inability of changing the IR based on some user
input like 'head motion'. Currently this kinda works, but there is an issue
with glitch when a new ReverbConvolver is created.

​This glitching is why AudioBufferSourceNode's don't allow you to set the
buffer more than once. As you say, glitching also happens when you change
the buffer of a ConvolverNode.

I think it makes sense to disallow setting the buffer for a ConvolverNode
too.​

The work around suggested by someone during the WebAudio conference was to
crossfade between two ConvolverNodes. Which kinda works for now
http://chinpen.net/auralizr/.

The (long winded) question here would be, in case of the inability to set
buffer more than once, such a use-case would need to re-create multiple
ConvolverNodes per second. Would that be a performance issue?

​I guess there could be a performance issue, depending on how long your
impulse responses are and how fast you fade out the old one.​

​Presumably this could be done fairly quickly at which point the old
convolver node can be disconnected from the graph. You may have to wait
for GC to collect that node, however.​

—
Reply to this email directly or view it on GitHub
#122 (comment)
.

Ray

[joeberkovitz]

Resolution:

1. Allow buffer property of ConvolverNode to be set at will, with guidance in the spec clarifying that glitching may occur.
2. Specify that normalize only takes effect when buffer is set, so normalize must be set prior to the buffer being set.

[rtoy]

I don't think anything needs to be done. The spec already says for normalize:

Changes to this value do not take effect until the next time the buffer attribute is set.

I think this pretty much implies you must set normalize before setting the buffer, otherwise the convolver node won't normalize the buffer.

[joeberkovitz]

Closing as per @rtoy's suggestion