Bug 79765 – [Chromium] Layout Test webaudio/audiobuffersource-channels.html is failing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

The following layout test is failing on Debug builds

webaudio/audiobuffersource-channels.html

http://trac.webkit.org/changeset/109076 introduced the test. It tries to set more than 6 channels, but AudioNodeOutput doesn't allow more than 6 channels(from AudioNodeOutput.cpp):

// This is considering that 5.1 (6 channels) is the largest we'll ever deal with.
// It can easily be increased to support more if the web audio specification is updated.
const unsigned MaxNumberOfChannels = 6;

Looks like the test should set less than 6 channels for now.

[Kenichi Ishibashi]

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28dbg%29/builds/8481/steps/webkit_tests/logs/stdio

[Wei James (wistoch)]

(In reply to comment #0)
> The following layout test is failing on Debug builds
> 
> webaudio/audiobuffersource-channels.html
> 
> http://trac.webkit.org/changeset/109076 introduced the test. It tries to set more than 6 channels, but AudioNodeOutput doesn't allow more than 6 channels(from AudioNodeOutput.cpp):
> 
> // This is considering that 5.1 (6 channels) is the largest we'll ever deal with.
> // It can easily be increased to support more if the web audio specification is updated.
> const unsigned MaxNumberOfChannels = 6;
> 
> Looks like the test should set less than 6 channels for now.

how about remove the assert in AudioNodeOutput to support more than 6 channels in AudioNodeOutput?

[Kenichi Ishibashi]

Committed suppression.
http://trac.webkit.org/changeset/109090

[Kenichi Ishibashi]

(In reply to comment #2)
> (In reply to comment #0)
> > The following layout test is failing on Debug builds
> > 
> > webaudio/audiobuffersource-channels.html
> > 
> > http://trac.webkit.org/changeset/109076 introduced the test. It tries to set more than 6 channels, but AudioNodeOutput doesn't allow more than 6 channels(from AudioNodeOutput.cpp):
> > 
> > // This is considering that 5.1 (6 channels) is the largest we'll ever deal with.
> > // It can easily be increased to support more if the web audio specification is updated.
> > const unsigned MaxNumberOfChannels = 6;
> > 
> > Looks like the test should set less than 6 channels for now.
> 
> how about remove the assert in AudioNodeOutput to support more than 6 channels in AudioNodeOutput?

The comment sounds reasonable to me, but I'm not familiar with webaudio stuff. I think you can ask the author to remove it. For now, I'd like to suggest updating the test to avoid crashes.

[Wei James (wistoch)]

Created attachment 129216 [details]
Patch

[Kenichi Ishibashi]

Kent-san, could you please rubber-stamp r+?

[Kent Tamura]

Comment on attachment 129216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129216&action=review

I'm not sure this change is correct or not.
We had better roll 109076 out.

> Source/WebCore/ChangeLog:7
> +

Please write a reason why the removing 6 channel limitation is right.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

ChangeLog should not have this OOPS line.

> Source/WebCore/webaudio/AudioNodeOutput.cpp:-40
> -// This is considering that 5.1 (6 channels) is the largest we'll ever deal with.
> -// It can easily be increased to support more if the web audio specification is updated.
> -const unsigned MaxNumberOfChannels = 6;

Isn't this specified in the standard?

[Kenichi Ishibashi]

Hi Wei,

Could you please respond Kent-san's comments? I'll roll this out otherwise.

[Chris Rogers]

Let's just keep the test disabled for now and not roll out the change.  I think James can fix this:


James, in looking further at this, the short answer is that we need to add *some* kind of maximum number of channels and keep the ASSERT just for basic sanity checking, security, etc.

I notice that we have three places where checks exist (or used to exist):
AudioBuffer (in AudioBuffer::create())
AudioBufferSourceNode  <--- check recently removed but should be added back
AudioNodeOutput <-- where current ASSERT problem is happening

Notice this check in AudioBuffer::create():

PassRefPtr<AudioBuffer> AudioBuffer::create(unsigned numberOfChannels, size_t numberOfFrames, float sampleRate)
{
    if (sampleRate < 22050 || sampleRate > 96000 || numberOfChannels > 10 || !numberOfFrames)
        return 0;

    return adoptRef(new AudioBuffer(numberOfChannels, numberOfFrames, sampleRate));
}

We need to protect against wild values of unreasonably large number of channels (perhaps 10 is too small a value, and use 32?).  But in any case, the value we choose (10 or higher) should be defined in a common place for all these checks, perhaps as a static method of AudioContext called maxNumberOfChannels()?, so that we can also check against this same value in AudioBufferSourceNode::setBuffer() and also switch the ASSERT in AudioNodeOutput.cpp to compare against this value instead of the file-local constant MaxNumberOfChannels as we currently have.

[Wei James (wistoch)]

Comment on attachment 129216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129216&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> Please write a reason why the removing 6 channel limitation is right.

Currently web audio only support limited channels, some place only support mono/stereo channel and some place only support less than 6 channels. we are working on removing the limitation. Several patches have been landed for it. 

So at last we will have no limitation for the channel number.

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> ChangeLog should not have this OOPS line.

got it. will fix it. thanks

>> Source/WebCore/webaudio/AudioNodeOutput.cpp:-40
>> -const unsigned MaxNumberOfChannels = 6;
> 
> Isn't this specified in the standard?

it is a draft and currently only support 6 channels at most. we are working to let it support more.

[Wei James (wistoch)]

(In reply to comment #9)
> Let's just keep the test disabled for now and not roll out the change.  I think James can fix this:
> 
> 
> James, in looking further at this, the short answer is that we need to add *some* kind of maximum number of channels and keep the ASSERT just for basic sanity checking, security, etc.
> 
> I notice that we have three places where checks exist (or used to exist):
> AudioBuffer (in AudioBuffer::create())
> AudioBufferSourceNode  <--- check recently removed but should be added back
> AudioNodeOutput <-- where current ASSERT problem is happening
> 
> Notice this check in AudioBuffer::create():
> 
> PassRefPtr<AudioBuffer> AudioBuffer::create(unsigned numberOfChannels, size_t numberOfFrames, float sampleRate)
> {
>     if (sampleRate < 22050 || sampleRate > 96000 || numberOfChannels > 10 || !numberOfFrames)
>         return 0;
> 
>     return adoptRef(new AudioBuffer(numberOfChannels, numberOfFrames, sampleRate));
> }
> 
> We need to protect against wild values of unreasonably large number of channels (perhaps 10 is too small a value, and use 32?).  But in any case, the value we choose (10 or higher) should be defined in a common place for all these checks, perhaps as a static method of AudioContext called maxNumberOfChannels()?, so that we can also check against this same value in AudioBufferSourceNode::setBuffer() and also switch the ASSERT in AudioNodeOutput.cpp to compare against this value instead of the file-local constant MaxNumberOfChannels as we currently have.


ok. I will work on it in this way. thanks

[Wei James (wistoch)]

Created attachment 129634 [details]
Patch

[Chris Rogers]

Comment on attachment 129634 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129634&action=review

Thanks James, this looks almost finished:

> Source/WebCore/webaudio/AudioContext.h:69
> +// This is considering 32 is large enough for multiple channels audio. 

I'd also add that this limit is somewhat arbitrary and could be increased if necessary.

> Source/WebCore/webaudio/AudioContext.h:70
> +const unsigned MaxNumberOfChannels = 32;

We can't have a "const unsigned" in a header file like this for namespace/linkage reasons.  These must always go in .cpp files
Instead, I'd recommend making this a private enum, something like:
enum { MaxNumberOfChannels = 32 };

[Wei James (wistoch)]

Created attachment 129778 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 129634 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129634&action=review

>> Source/WebCore/webaudio/AudioContext.h:69
>> +// This is considering 32 is large enough for multiple channels audio. 
> 
> I'd also add that this limit is somewhat arbitrary and could be increased if necessary.

thanks. added.

>> Source/WebCore/webaudio/AudioContext.h:70
>> +const unsigned MaxNumberOfChannels = 32;
> 
> We can't have a "const unsigned" in a header file like this for namespace/linkage reasons.  These must always go in .cpp files
> Instead, I'd recommend making this a private enum, something like:
> enum { MaxNumberOfChannels = 32 };

thanks. fixed.

[Chris Rogers]

Comment on attachment 129778 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 129778 [details]
Patch

Clearing flags on attachment: 129778

Committed r109532: <http://trac.webkit.org/changeset/109532>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.