Bug 79765 - [Chromium] Layout Test webaudio/audiobuffersource-channels.html is failing
Summary: [Chromium] Layout Test webaudio/audiobuffersource-channels.html is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wei James (wistoch)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-28 01:24 PST by Kenichi Ishibashi
Modified: 2012-03-02 01:32 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2012-02-28 02:03 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (6.67 KB, patch)
2012-02-29 22:23 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (6.55 KB, patch)
2012-03-01 16:57 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-02-28 01:24:09 PST
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.
Comment 2 Wei James (wistoch) 2012-02-28 01:29:26 PST
(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?
Comment 3 Kenichi Ishibashi 2012-02-28 01:37:08 PST
Committed suppression.
http://trac.webkit.org/changeset/109090
Comment 4 Kenichi Ishibashi 2012-02-28 01:42:06 PST
(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.
Comment 5 Wei James (wistoch) 2012-02-28 02:03:27 PST
Created attachment 129216 [details]
Patch
Comment 6 Kenichi Ishibashi 2012-02-28 02:06:22 PST
Kent-san, could you please rubber-stamp r+?
Comment 7 Kent Tamura 2012-02-28 02:24:37 PST
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?
Comment 8 Kenichi Ishibashi 2012-02-28 17:10:30 PST
Hi Wei,

Could you please respond Kent-san's comments? I'll roll this out otherwise.
Comment 9 Chris Rogers 2012-02-28 17:16:19 PST
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.
Comment 10 Wei James (wistoch) 2012-02-28 17:19:09 PST
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.
Comment 11 Wei James (wistoch) 2012-02-28 17:26:31 PST
(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
Comment 12 Wei James (wistoch) 2012-02-29 22:23:23 PST
Created attachment 129634 [details]
Patch
Comment 13 Chris Rogers 2012-03-01 11:58:02 PST
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 };
Comment 14 Wei James (wistoch) 2012-03-01 16:57:36 PST
Created attachment 129778 [details]
Patch
Comment 15 Wei James (wistoch) 2012-03-01 17:03:43 PST
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.
Comment 16 Chris Rogers 2012-03-01 17:28:29 PST
Comment on attachment 129778 [details]
Patch

Looks good.
Comment 17 WebKit Review Bot 2012-03-02 01:32:22 PST
Comment on attachment 129778 [details]
Patch

Clearing flags on attachment: 129778

Committed r109532: <http://trac.webkit.org/changeset/109532>
Comment 18 WebKit Review Bot 2012-03-02 01:32:28 PST
All reviewed patches have been landed.  Closing bug.