Bug 173022 - [Web Audio] createScriptProcessor throws IndexSizeError for valid arguments
Summary: [Web Audio] createScriptProcessor throws IndexSizeError for valid arguments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-06 11:37 PDT by Benjamin King
Modified: 2017-06-07 21:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.27 KB, patch)
2017-06-07 17:59 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (10.34 KB, patch)
2017-06-07 19:59 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (10.40 KB, patch)
2017-06-07 20:25 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin King 2017-06-06 11:37:19 PDT
Safari Version 11.0 (13604.1.21.7) on macOS 10.13 Beta (17A264c)

Go to http://test.webrtc.org and hit start, you will get this error

Microphone test [ FAILED ] Failed to get access to local media due to error: IndexSizeError"

{"ts":1496772961420,"name":"getusermedia","id":3,"args":{"status":"fail","error":{"code":1,"name":"IndexSizeError","message":"The index is not in the allowed range.","line":10,"column":15801,"sourceURL":"https://test.webrtc.org/main.js"}}},

Full report:

https://test.webrtc.org/report/AMIfv97ilE0n4dR2Lt5LLqYfS-vjdKKe31HoB4tWaw40JI3gOIq4TtS9NO3-IFfCPR3OjoE9FA8VqKXeiwqIjRZoxTfDEGkzIywyN2WzhR8kA2XHEKIljthyyqmuGf7UctRFGypFdoksXWHLR7UJ4hTDBcKo7a4pCQ
Comment 1 Benjamin King 2017-06-07 11:48:22 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)
Comment 2 Jon Lee 2017-06-07 17:03:33 PDT
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.
Comment 3 Jer Noble 2017-06-07 17:59:39 PDT
Created attachment 312261 [details]
Patch
Comment 4 Sam Weinig 2017-06-07 18:15:19 PDT
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;
Comment 5 Jer Noble 2017-06-07 19:56:58 PDT
(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.
Comment 6 Jer Noble 2017-06-07 19:59:45 PDT
Created attachment 312268 [details]
Patch
Comment 7 Sam Weinig 2017-06-07 20:10:30 PDT
> > > 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.
Comment 8 Sam Weinig 2017-06-07 20:17:44 PDT
(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.
Comment 9 Sam Weinig 2017-06-07 20:20:57 PDT
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;
}
Comment 10 Jer Noble 2017-06-07 20:25:42 PDT
Created attachment 312269 [details]
Patch
Comment 11 Jer Noble 2017-06-07 20:30:02 PDT
(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
Comment 12 Jer Noble 2017-06-07 20:30:45 PDT
(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. :)
Comment 13 Sam Weinig 2017-06-07 20:38:51 PDT
(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.
Comment 14 Eric Carlson 2017-06-07 21:25:09 PDT
(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 15 WebKit Commit Bot 2017-06-07 21:31:35 PDT
Comment on attachment 312269 [details]
Patch

Clearing flags on attachment: 312269

Committed r217919: <http://trac.webkit.org/changeset/217919>
Comment 16 WebKit Commit Bot 2017-06-07 21:31:37 PDT
All reviewed patches have been landed.  Closing bug.