Summary: | [Web Audio] createScriptProcessor throws IndexSizeError for valid arguments | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin King <king7532> | ||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, jer.noble, jonlee, sam, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Mac | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Benjamin King
2017-06-06 11:37:19 PDT
The IndexSizeError is thrown by: audioContext.createScriptProcessor(this.bufferSize,this.inputChannelCount,this.outputChannelCount) this.bufferSize = 0 this.inputChannelCount = 6 this.outputChannelCount = 2 Steps to reproduce: 1. https://test.webrtc.org 2. Click start 3. Click on Audio Capture under Microphone to see the error message Reproducible using Safari Technology Preview Release 32 (Safari 11.0, WebKit 12604.1.23.0.4) We're not picking a good default buffer size. If I breakpoint in the script and override this.bufferSize to 256, the microphone test passes. Created attachment 312261 [details]
Patch
Comment on attachment 312261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312261&action=review Other than the compile error, looks good to me. > Source/WebCore/ChangeLog:11 > + The Web Audio spec defines a default behavior when clients pass in a value of 0 > + for bufferSize to the createScriptProcessor() method. Can you link to the relevant spec here? Is it time for me to do the BaseAudioContext split? > Source/WebCore/Modules/webaudio/AudioContext.cpp:511 > + case 256: > + case 512: > + case 1024: > + case 2048: > + case 4096: > + case 8192: > + case 16384: I'm really curious if if there any compilers that would generate this code for: auto v = std::log2(bufferSize); return v >= 8 && v <= 14; > Source/WebCore/Modules/webaudio/AudioContext.cpp:539 > + return node; This will need to be WTFMove(node) to get it to work with the ExceptionOr; (In reply to Sam Weinig from comment #4) > Comment on attachment 312261 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=312261&action=review > > Other than the compile error, looks good to me. > > > Source/WebCore/ChangeLog:11 > > + The Web Audio spec defines a default behavior when clients pass in a value of 0 > > + for bufferSize to the createScriptProcessor() method. > > Can you link to the relevant spec here? Sure. > Is it time for me to do the BaseAudioContext split? No time like the present! (So, yes?) > > Source/WebCore/Modules/webaudio/AudioContext.cpp:511 > > + case 256: > > + case 512: > > + case 1024: > > + case 2048: > > + case 4096: > > + case 8192: > > + case 16384: > > I'm really curious if if there any compilers that would generate this code > for: > auto v = std::log2(bufferSize); > return v >= 8 && v <= 14; Probably not; the spec wants to reject, e.g., 257 through 511, and I don't believe that line above has the same effect. > > Source/WebCore/Modules/webaudio/AudioContext.cpp:539 > > + return node; > > This will need to be WTFMove(node) to get it to work with the ExceptionOr; Ok. Created attachment 312268 [details]
Patch
> > > Source/WebCore/Modules/webaudio/AudioContext.cpp:511
> > > + case 256:
> > > + case 512:
> > > + case 1024:
> > > + case 2048:
> > > + case 4096:
> > > + case 8192:
> > > + case 16384:
> >
> > I'm really curious if if there any compilers that would generate this code
> > for:
> > auto v = std::log2(bufferSize);
> > return v >= 8 && v <= 14;
>
> Probably not; the spec wants to reject, e.g., 257 through 511, and I don't
> believe that line above has the same effect.
It doesn't, I was being dumb.
(In reply to Sam Weinig from comment #7) > > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:511 > > > > + case 256: > > > > + case 512: > > > > + case 1024: > > > > + case 2048: > > > > + case 4096: > > > > + case 8192: > > > > + case 16384: > > > > > > I'm really curious if if there any compilers that would generate this code > > > for: > > > auto v = std::log2(bufferSize); > > > return v >= 8 && v <= 14; > > > > Probably not; the spec wants to reject, e.g., 257 through 511, and I don't > > believe that line above has the same effect. > > It doesn't, I was being dumb. These two (using the lovely, hasOneBitSet aka isPowerOf2 helper), on the other hand, would work: inline bool hasOneBitSet(T value) { return !((value - 1) & value) && value; } static bool usingLog(unsigned input) { if (!hasOneBitSet(input)) return false; auto v = std::log2(input); return v >= 8 && v <= 14; } static bool manualLog(unsigned input) { if (!hasOneBitSet(input)) return false; int v = 0; while (input >>= 1) ++v; return v >= 8 && v <= 14; } But neither are as straightforward (or probably fast) as the switch statement. I just enjoy silly power of two games. Ok, last one I promise, this ones my favorite though: static bool log2builting(unsigned input) { if (!hasOneBitSet(input)) return false; int v = __builtin_ctz(input); return v >= 8 && v <= 14; } Created attachment 312269 [details]
Patch
(In reply to Sam Weinig from comment #9) > int v = __builtin_ctz(input); Oh, ffs. [1] [1] https://en.wikipedia.org/wiki/Find_first_set (In reply to Jer Noble from comment #11) > (In reply to Sam Weinig from comment #9) > > int v = __builtin_ctz(input); > > Oh, ffs. [1] > > [1] https://en.wikipedia.org/wiki/Find_first_set And yes, that last one _is_ the best one. :) (In reply to Jer Noble from comment #12) > (In reply to Jer Noble from comment #11) > > (In reply to Sam Weinig from comment #9) > > > int v = __builtin_ctz(input); > > > > Oh, ffs. [1] > > > > [1] https://en.wikipedia.org/wiki/Find_first_set > > And yes, that last one _is_ the best one. :) I found better (well, it lets the compiler to a bit better)! static bool log2builting(unsigned input) { if (__builtin_popcount(value) != 1) return false; int v = __builtin_ctz(input); return v >= 8 && v <= 14; } I am dead inside. (In reply to Sam Weinig from comment #13) > (In reply to Jer Noble from comment #12) > > (In reply to Jer Noble from comment #11) > > > (In reply to Sam Weinig from comment #9) > > > > int v = __builtin_ctz(input); > > > > > > Oh, ffs. [1] > > > > > > [1] https://en.wikipedia.org/wiki/Find_first_set > > > > And yes, that last one _is_ the best one. :) > > I found better (well, it lets the compiler to a bit better)! > > static bool log2builting(unsigned input) > { > if (__builtin_popcount(value) != 1) > return false; > int v = __builtin_ctz(input); > return v >= 8 && v <= 14; > } > > I am dead inside. Sir, you are nerd inside. Comment on attachment 312269 [details] Patch Clearing flags on attachment: 312269 Committed r217919: <http://trac.webkit.org/changeset/217919> All reviewed patches have been landed. Closing bug. |