Bug 173022 – [Web Audio] createScriptProcessor throws IndexSizeError for valid arguments

• Component: Media
• Status: RESOLVED FIXED

[Benjamin King]

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

[Benjamin King]

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)

[Jon Lee]

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.

[Jer Noble]

Created attachment 312261 [details]
Patch

[Sam Weinig]

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;

[Jer Noble]

(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.

[Jer Noble]

Created attachment 312268 [details]
Patch

[Sam Weinig]

> > > 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.

[Sam Weinig]

(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.

[Sam Weinig]

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;
}

[Jer Noble]

Created attachment 312269 [details]
Patch

[Jer Noble]

(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

[Jer Noble]

(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. :)

[Sam Weinig]

(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.

[Eric Carlson]

(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.

[WebKit Commit Bot]

Comment on attachment 312269 [details]
Patch

Clearing flags on attachment: 312269

Committed r217919: <http://trac.webkit.org/changeset/217919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.