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.
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28dbg%29/builds/8481/steps/webkit_tests/logs/stdio
(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?
Committed suppression. http://trac.webkit.org/changeset/109090
(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.
Created attachment 129216 [details] Patch
Kent-san, could you please rubber-stamp r+?
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?
Hi Wei, Could you please respond Kent-san's comments? I'll roll this out otherwise.
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 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.
(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
Created attachment 129634 [details] Patch
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 };
Created attachment 129778 [details] Patch
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 on attachment 129778 [details] Patch Looks good.
Comment on attachment 129778 [details] Patch Clearing flags on attachment: 129778 Committed r109532: <http://trac.webkit.org/changeset/109532>
All reviewed patches have been landed. Closing bug.