fix: enforce spec limits for ConvolverNode.buffer and allow 32-channel buffers - #1258
Open
giaBaoJS wants to merge 1 commit into
Open
fix: enforce spec limits for ConvolverNode.buffer and allow 32-channel buffers#1258giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
…l buffers The ConvolverNode buffer setter accepted any impulse response, so a buffer with an unsupported channel count or a sample rate different from the context's reached the engine instead of throwing NotSupportedError. The existing channel-count check only ran for buffers passed through the constructor options. AudioBuffer rejected 32 channels while its own error message and MAX_CHANNEL_COUNT both treat 32 as the inclusive upper bound, and the spec requires implementations to support at least 32 channels. WPT the-convolvernode-interface goes from 170/253 to 203/255 passing assertions; convolver-channels.html now passes all 32 of its assertions and ctor-convolver.html passes the illegal-sample-rate case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No linked issue: found by reading source against the Web Audio API specification.
Setting
ConvolverNode.bufferto an impulse response with an unsupportedchannel count, or with a sample rate different from the context's, now throws
NotSupportedErrorinstead of silently configuring the convolver. This is thebehaviour the spec requires and the behaviour browsers already have, but code
that relied on the previous permissive setter will now see an exception.
Introduced changes
ConvolverNode.buffersetter now runs the spec's setter steps. Web Audio API§1.17.2: "If the buffer number of channels is not 1, 2, 4, or if the
sample-rate of the buffer is not the same as the sample-rate of its associated
BaseAudioContext, a NotSupportedError MUST be thrown." Previously the only
channel-count check lived in
ConvolverOptionsValidator, so it ran for abuffer passed through the constructor options and never for a later
assignment. The sample-rate condition was not checked anywhere, and the native
ConvolverNodeHostObject::setBufferhas no guard of its own, so a 31-channelimpulse response was accepted and allocated one convolver per channel.
AudioBuffer/createBuffernow accept 32 channels. The bound was writtennumberOfChannels >= 32while the error message on the next line reports therange as
[1, 32]andMAX_CHANNEL_COUNTis inclusive everywhere else in theengine. §1.1.2 requires an implementation to support at least 32 channels.
Same one-line fix in the three web-core copies.
tests/convolver-buffer.test.tscovering both.Measurements
WPT smoke profile, before and after on the same build:
wpt-compare.mjsreports no regressions in any of the other 27 sections.convolver-channels.htmlwas aborting mid-task: it callscontext.createBuffer(count, 1, sampleRate)for count 1..32 outside theshould()wrapper, so the 32-channel call threw and killed the task at count31. With both fixes it now passes all 32 of its assertions.
ctor-convolver.html's "illegal sample rate buffer throws NotSupportedError"also passes now; its three remaining failures are unrelated (missing
DynamicsCompressorNode, theclamped-maxdefault forchannelCountMode, andchannelCountoption validation).No render-path code changed. The new checks run in a JS property setter at
graph-configuration time, and no C++ file is touched.
Splitting this, if you prefer
The two fixes are separable and I am happy to split them into two PRs on
request. The
AudioBufferbound is a strictly more permissive bug fix with nobehaviour change for anything that worked before; the
ConvolverNodesetter isthe actual behaviour change. They are bundled here only because
convolver-channels.htmlneeds both to stop aborting, so neither one alonemakes that file pass.
Checklist
On the three blank boxes: there is no issue to link, as noted at the top. The
WPT coverage summary is regenerated by maintainers with
yarn wpt:report:docs,which needs a second run against the published release in
apps/common-app,and the interface support table itself does not change here. The Android
old-arch spec file is unaffected, since no native or spec surface changed.