Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
375383002:
Extending supported sample rate from 3.0KHz to 192.0KHz for OfflineAudioContext. (Closed)
|
Created:
6 years, 5 months ago by KhNo Modified:
6 years, 3 months ago Reviewers:
Raymond Toy CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionExtending supported sample rate from 3.0KHz to 192.0KHz for OfflineAudioContext.
Currently, AudioBuffer is supported 3.0KHz to 192.0 KHz.
However, offlineAudioContext is not supported lower sample rate than 44.0KHz.
http://webaudio.github.io/web-audio-api/#the-audiobuffersourcenode-interface
“Describes the sample-rate of the linear PCM audio data
in the buffer in sample-frames per second.
An implementation must support sample-rates
in at least the range 22050 to 96000”
Changed supported sampleRate range 3KHz to 192KHz and fftsize for convolution.
BUG=394009
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181816
Patch Set 1 #
Total comments: 1
Patch Set 2 : simplify code as review. #Patch Set 3 : Expend supported sampleRate 8.0 ~ 96 KHz. #Patch Set 4 : Update layoutTest #
Total comments: 18
Patch Set 5 : Extend range same as AudioBuffers, fix as review. #Patch Set 6 : Fix as review #
Total comments: 20
Patch Set 7 : Change some review point and logic to find fftsize. #
Total comments: 4
Patch Set 8 : change type to .f #Patch Set 9 : Fix as review direct return fftsize #
Total comments: 6
Patch Set 10 : Remove fftSizes array. #
Total comments: 2
Patch Set 11 : Change fftSizeForSampleRate logic. #
Total comments: 6
Patch Set 12 : update names and comment. #
Messages
Total messages: 44 (5 generated)
Please provide a link to the spec in your CL description: http://webaudio.github.io/web-audio-api/#widl-AudioContext-createBuffer-Audio... Can you add a layout test that used to fail and now passes with your patch?
https://codereview.chromium.org/375383002/diff/1/Source/platform/audio/HRTFPa... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/1/Source/platform/audio/HRTFPa... Source/platform/audio/HRTFPanner.cpp:90: else if (sampleRate < 88200.0) I am guessing you want '>', not '>'. We could simplify like this: size_t fftSize = 512; if (sampleRate < 44100.f) fftSize = 256; else if (sampleRate > 88200.f) fftSize = 1024;
The title and description seems wrong. AudioBuffers already support sample rates down to 3 kHz and up to 192 kHz. See crbug.com/344375. This CL seems to be allowing the HRTF panner to support a wider range of sample rates. However, I don't think just adjusting the allowed range and the fft size is enough. This might be ok for a temporary stopgap, but since the HRTF panner tables contain sampled impulse responses, I think we should resample the responses to the correct sample rate.
On 2014/07/15 15:45:29, Chris Dumez wrote: > Please provide a link to the spec in your CL description: > http://webaudio.github.io/web-audio-api/#widl-AudioContext-createBuffer-Audio... > > Can you add a layout test that used to fail and now passes with your patch? Thanks for your reviewing. I have added the link of spec. This patch was not finalized yet, as your comment, I have to create test cases and verify for it. I will request to you if there is updating of the patch set. :)
On 2014/07/15 16:02:34, Raymond Toy wrote: > The title and description seems wrong. AudioBuffers already support sample > rates down to 3 kHz and up to 192 kHz. > > See crbug.com/344375. > > This CL seems to be allowing the HRTF panner to support a wider range of sample > rates. However, I don't think just adjusting the allowed range and the fft size > is enough. This might be ok for a temporary stopgap, but since the HRTF panner > tables contain sampled impulse responses, I think we should resample the > responses to the correct sample rate. Hi, Raymond. That was deficient description since I had not finalized the patch set (create layout tests and verification). I should have leave WIP. I have replaced description, currently offlineAudioContext is not allowed using smaller sample rate than 44.1KHz. So this patch set make it possible. I also checked impulse responses assets before creating this patch set. The audioBus was resampling assets if it has different sampleRate with original 44.1KHz (assets sampled) So, if we give correct fftsize with considering twice for convolution. It is fine. I will create layout tests for this patch set.
On 2014/07/15 17:21:10, KhNo wrote: > On 2014/07/15 16:02:34, Raymond Toy wrote: > > The title and description seems wrong. AudioBuffers already support sample > > rates down to 3 kHz and up to 192 kHz. > > > > See crbug.com/344375. > > > > This CL seems to be allowing the HRTF panner to support a wider range of > sample > > rates. However, I don't think just adjusting the allowed range and the fft > size > > is enough. This might be ok for a temporary stopgap, but since the HRTF > panner > > tables contain sampled impulse responses, I think we should resample the > > responses to the correct sample rate. > > Hi, Raymond. > That was deficient description since I had not finalized the patch set (create > layout tests and verification). > I should have leave WIP. > > I have replaced description, currently offlineAudioContext is not allowed using > smaller sample rate than 44.1KHz. > So this patch set make it possible. > > I also checked impulse responses assets before creating this patch set. > The audioBus was resampling assets if it has different sampleRate with original > 44.1KHz (assets sampled) > > So, if we give correct fftsize with considering twice for convolution. It is > fine. > I will create layout tests for this patch set. Sorry. You're right and I'm wrong. The responses are already sample-rate converted. When you're ready, please update the description. And can you also enable support for up to 192 kHz? Windows supports that so we should allow OfflineContexts to support that as well.
On 2014/07/15 17:27:01, Raymond Toy wrote: > On 2014/07/15 17:21:10, KhNo wrote: > > On 2014/07/15 16:02:34, Raymond Toy wrote: > > > The title and description seems wrong. AudioBuffers already support sample > > > rates down to 3 kHz and up to 192 kHz. > > > > > > See crbug.com/344375. > > > > > > This CL seems to be allowing the HRTF panner to support a wider range of > > sample > > > rates. However, I don't think just adjusting the allowed range and the fft > > size > > > is enough. This might be ok for a temporary stopgap, but since the HRTF > > panner > > > tables contain sampled impulse responses, I think we should resample the > > > responses to the correct sample rate. > > > > Hi, Raymond. > > That was deficient description since I had not finalized the patch set (create > > layout tests and verification). > > I should have leave WIP. > > > > I have replaced description, currently offlineAudioContext is not allowed > using > > smaller sample rate than 44.1KHz. > > So this patch set make it possible. > > > > I also checked impulse responses assets before creating this patch set. > > The audioBus was resampling assets if it has different sampleRate with > original > > 44.1KHz (assets sampled) > > > > So, if we give correct fftsize with considering twice for convolution. It is > > fine. > > I will create layout tests for this patch set. > > Sorry. You're right and I'm wrong. The responses are already sample-rate > converted. > > When you're ready, please update the description. > > And can you also enable support for up to 192 kHz? Windows supports that so we > should allow OfflineContexts to support that as well. Yes, I will try it. Two questions, only for windows? or All? Don't we need to consider changing the specification? First implementation, then changing the spec. :)
On 2014/07/15 17:37:51, KhNo wrote: > On 2014/07/15 17:27:01, Raymond Toy wrote: > > On 2014/07/15 17:21:10, KhNo wrote: > > > On 2014/07/15 16:02:34, Raymond Toy wrote: > > > > The title and description seems wrong. AudioBuffers already support > sample > > > > rates down to 3 kHz and up to 192 kHz. > > > > > > > > See crbug.com/344375. > > > > > > > > This CL seems to be allowing the HRTF panner to support a wider range of > > > sample > > > > rates. However, I don't think just adjusting the allowed range and the > fft > > > size > > > > is enough. This might be ok for a temporary stopgap, but since the HRTF > > > panner > > > > tables contain sampled impulse responses, I think we should resample the > > > > responses to the correct sample rate. > > > > > > Hi, Raymond. > > > That was deficient description since I had not finalized the patch set > (create > > > layout tests and verification). > > > I should have leave WIP. > > > > > > I have replaced description, currently offlineAudioContext is not allowed > > using > > > smaller sample rate than 44.1KHz. > > > So this patch set make it possible. > > > > > > I also checked impulse responses assets before creating this patch set. > > > The audioBus was resampling assets if it has different sampleRate with > > original > > > 44.1KHz (assets sampled) > > > > > > So, if we give correct fftsize with considering twice for convolution. It is > > > fine. > > > I will create layout tests for this patch set. > > > > Sorry. You're right and I'm wrong. The responses are already sample-rate > > converted. > > > > When you're ready, please update the description. > > > > And can you also enable support for up to 192 kHz? Windows supports that so we > > should allow OfflineContexts to support that as well. > > Yes, I will try it. Two questions, only for windows? or All? > Don't we need to consider changing the specification? > First implementation, then changing the spec. :) Might as well 192 kHz for all platforms. The spec doesn't need changing because it seems it doesn't specify any range of valid rates. But also see https://github.com/WebAudio/web-audio-api/issues/314
PTAL. :)
Looks good overall. One final nit: Change "Expending" to "Extending" in the description. https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... LayoutTests/webaudio/offlineaudiocontext-constructor.html:15: shouldThrow("new OfflineAudioContext(2, 512, 98000)"); Aren't these two tests already covered in dom-exceptions? Unless you specifically want to test 4000 and 98000, I would just use the ones from dom-exceptions. https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... LayoutTests/webaudio/offlineaudiocontext-constructor.html:22: shouldNotThrow("new OfflineAudioContext(2, 512, 96000)"); Might as well add a test for 48000, which is another very common sample rate. https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioContext.cpp:83: return sampleRate >= 8000 && sampleRate <= 96000; AudioBuffers support sample rates from 3kHz to 192 kHz. I think offline contexts should support the same range. https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/OfflineAudioContext.cpp:70: exceptionState.throwDOMException(SyntaxError, "sample rate (" + String::number(sampleRate) + ") must be in the range 8000-96000 Hz."); Probably a good time to replace this with ExceptionMessages::indexOutsideRange as done in lines 57-65. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:48: const float coefficientSampleRate = 11025.f; Where does 11025 come from? I thought the HRTF database was sampled at 44100. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:88: size_t fftSize = minimumFFTSize; This initial value seems useless since you give fftSize a new value in the next line. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:92: return fftSize; If we expand the sample rate to 192 kHz, will this still work?
PTAL, I have tested it manually, it doesn't have any problem since it is same as AudioBuffer already supported range. However, I would like to add additional layout test for offlineAudioContext - panner - HRTF for next CL if I find the idea. Can I prepare it separately with this CL since I'm looking for idea. :) Thanks for always your review. https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... LayoutTests/webaudio/offlineaudiocontext-constructor.html:15: shouldThrow("new OfflineAudioContext(2, 512, 98000)"); On 2014/07/30 16:49:38, Raymond Toy wrote: > Aren't these two tests already covered in dom-exceptions? Unless you > specifically want to test 4000 and 98000, I would just use the ones from > dom-exceptions. Yes, it can be replaced. I think it doesn't need to test duplicately. https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... LayoutTests/webaudio/offlineaudiocontext-constructor.html:22: shouldNotThrow("new OfflineAudioContext(2, 512, 96000)"); On 2014/07/30 16:49:38, Raymond Toy wrote: > Might as well add a test for 48000, which is another very common sample rate. I agree with you, if there are famous values should be added. https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioContext.cpp:83: return sampleRate >= 8000 && sampleRate <= 96000; On 2014/07/30 16:49:38, Raymond Toy wrote: > AudioBuffers support sample rates from 3kHz to 192 kHz. I think offline contexts > should support the same range. I think there is no reason we should hesitate to extend range. It should have same range with AudioBuffers. https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/OfflineAudioContext.cpp:70: exceptionState.throwDOMException(SyntaxError, "sample rate (" + String::number(sampleRate) + ") must be in the range 8000-96000 Hz."); On 2014/07/30 16:49:38, Raymond Toy wrote: > Probably a good time to replace this with ExceptionMessages::indexOutsideRange > as done in lines 57-65. Thanks for good recommend. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:48: const float coefficientSampleRate = 11025.f; On 2014/07/30 16:49:38, Raymond Toy wrote: > Where does 11025 come from? I thought the HRTF database was sampled at 44100. This value is just determined to calculate fftsize actually not related with HRTF database's samplerate. It means minimum unit to divide. floor(sampleRate / coefficientSampleRate) * minimumFFTSize. Actually, there might be better word and name. Please recommend if you have. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:88: size_t fftSize = minimumFFTSize; On 2014/07/30 16:49:38, Raymond Toy wrote: > This initial value seems useless since you give fftSize a new value in the next > line. Done. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:92: return fftSize; On 2014/07/30 16:49:38, Raymond Toy wrote: > If we expand the sample rate to 192 kHz, will this still work? Yes, it can be. kMaxFFTPow2Size is 15 for Android, for others are 24.
On 2014/07/31 16:45:54, KhNo wrote: > PTAL, I have tested it manually, it doesn't have any problem since it is same as > AudioBuffer already supported range. Sorry, I don't quite follow you here. What did you test manually? And what do you mean it has the same AudioBuffer supported range? AudioBuffers support 3-192 kHz currently. > However, I would like to add additional layout test for offlineAudioContext - > panner - HRTF for next CL if I find the idea. Yes we are missing a test for HRTF so a layout test for that would be welcome. > > Can I prepare it separately with this CL since I'm looking for idea. :) > > Thanks for always your review. > > https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... > File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): > > https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... > LayoutTests/webaudio/offlineaudiocontext-constructor.html:15: shouldThrow("new > OfflineAudioContext(2, 512, 98000)"); > On 2014/07/30 16:49:38, Raymond Toy wrote: > > Aren't these two tests already covered in dom-exceptions? Unless you > > specifically want to test 4000 and 98000, I would just use the ones from > > dom-exceptions. > > Yes, it can be replaced. I think it doesn't need to test duplicately. > > https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... > LayoutTests/webaudio/offlineaudiocontext-constructor.html:22: > shouldNotThrow("new OfflineAudioContext(2, 512, 96000)"); > On 2014/07/30 16:49:38, Raymond Toy wrote: > > Might as well add a test for 48000, which is another very common sample rate. > > I agree with you, if there are famous values should be added. > > https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... > File Source/modules/webaudio/AudioContext.cpp (right): > > https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... > Source/modules/webaudio/AudioContext.cpp:83: return sampleRate >= 8000 && > sampleRate <= 96000; > On 2014/07/30 16:49:38, Raymond Toy wrote: > > AudioBuffers support sample rates from 3kHz to 192 kHz. I think offline > contexts > > should support the same range. > > I think there is no reason we should hesitate to extend range. It should have > same range with AudioBuffers. > > https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... > Source/modules/webaudio/OfflineAudioContext.cpp:70: > exceptionState.throwDOMException(SyntaxError, "sample rate (" + > String::number(sampleRate) + ") must be in the range 8000-96000 Hz."); > On 2014/07/30 16:49:38, Raymond Toy wrote: > > Probably a good time to replace this with ExceptionMessages::indexOutsideRange > > as done in lines 57-65. > > Thanks for good recommend. > > https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... > File Source/platform/audio/HRTFPanner.cpp (right): > > https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... > Source/platform/audio/HRTFPanner.cpp:48: const float coefficientSampleRate = > 11025.f; > On 2014/07/30 16:49:38, Raymond Toy wrote: > > Where does 11025 come from? I thought the HRTF database was sampled at 44100. > > This value is just determined to calculate fftsize actually not related with > HRTF database's samplerate. > It means minimum unit to divide. > floor(sampleRate / coefficientSampleRate) * minimumFFTSize. > > Actually, there might be better word and name. Please recommend if you have. > > https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... > Source/platform/audio/HRTFPanner.cpp:88: size_t fftSize = minimumFFTSize; > On 2014/07/30 16:49:38, Raymond Toy wrote: > > This initial value seems useless since you give fftSize a new value in the > next > > line. > > Done. > > https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... > Source/platform/audio/HRTFPanner.cpp:92: return fftSize; > On 2014/07/30 16:49:38, Raymond Toy wrote: > > If we expand the sample rate to 192 kHz, will this still work? > > Yes, it can be. kMaxFFTPow2Size is 15 for Android, for others are 24.
https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/off... LayoutTests/webaudio/offlineaudiocontext-constructor.html:22: shouldNotThrow("new OfflineAudioContext(2, 512, 96000)"); On 2014/07/31 16:45:53, KhNo wrote: > On 2014/07/30 16:49:38, Raymond Toy wrote: > > Might as well add a test for 48000, which is another very common sample rate. > > I agree with you, if there are famous values should be added. Just 48000 is enough for me. https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/modules/webaudio/... Source/modules/webaudio/AudioContext.cpp:83: return sampleRate >= 8000 && sampleRate <= 96000; On 2014/07/31 16:45:53, KhNo wrote: > On 2014/07/30 16:49:38, Raymond Toy wrote: > > AudioBuffers support sample rates from 3kHz to 192 kHz. I think offline > contexts > > should support the same range. > > I think there is no reason we should hesitate to extend range. It should have > same range with AudioBuffers. Great. Then you can use AudioBuffer::minSampleRate and maxSampleRate to get the limits. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:48: const float coefficientSampleRate = 11025.f; On 2014/07/31 16:45:54, KhNo wrote: > On 2014/07/30 16:49:38, Raymond Toy wrote: > > Where does 11025 come from? I thought the HRTF database was sampled at 44100. > > This value is just determined to calculate fftsize actually not related with > HRTF database's samplerate. > It means minimum unit to divide. > floor(sampleRate / coefficientSampleRate) * minimumFFTSize. > > Actually, there might be better word and name. Please recommend if you have. Might be better to split this into two parts. One constant to give the sample rate of the data in the database (to make it explicit). Then another constant to produce the correct scaling. https://codereview.chromium.org/375383002/diff/80001/Source/platform/audio/HR... Source/platform/audio/HRTFPanner.cpp:92: return fftSize; On 2014/07/31 16:45:54, KhNo wrote: > On 2014/07/30 16:49:38, Raymond Toy wrote: > > If we expand the sample rate to 192 kHz, will this still work? > > Yes, it can be. kMaxFFTPow2Size is 15 for Android, for others are 24. One other thing. Don't we need to make fftSize a power of two here? The sampleRate could be anything in range.
keonho07.kim@samsung.com changed reviewers: - ch.dumez@samsung.com
On 2014/07/31 17:16:24, Raymond Toy wrote: > On 2014/07/31 16:45:54, KhNo wrote: > > PTAL, I have tested it manually, it doesn't have any problem since it is same > as > > AudioBuffer already supported range. > > Sorry, I don't quite follow you here. What did you test manually? And what do > you mean it has the same AudioBuffer supported range? AudioBuffers support 3-192 > kHz currently. > As you know, currently AudioBuffer is already supporting the range of sampleRate from 3 to 192 Khz. But, offlineAudioContext was not considered even if it is using also same AudioBuffer logic. The isSamppleRateRangeGood() that is used for only offlineAudioContext restricts the range of sample rate differently with AudioContext. In addition, separately, FFTSize for HRTFPanner needs to fix the range and calculataon for correct size. The sample rate was extended from (44100 ~ 96000) to (3000 ~ 192000). However, FFTSize for each sampleRate was not considered. Modification in Source/platform/audio/HRTFPanner.cpp will give correct fftsize depend on AudioBuffer's sampleRate. I have manually created test application audiocontext/offline-audiocontext create audio buffer with samplerate 3 and 192. This buffer is passed to panner node, then goes out destination node. However, It was difficult to make it layout test.
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/of... File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/of... LayoutTests/webaudio/offlineaudiocontext-constructor.html:13: // Make sure we don't throw exception supported ranges of sample rate with min/max. Fix up phrasing: // Make sure we don't throw exceptions for supported ranges of sample rates for an OfflineAudioContext. https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/of... LayoutTests/webaudio/offlineaudiocontext-constructor.html:16: // Make sure we don't throw exception supported ranges of sample rate. Probably don't need this if you change the comment on line 13. https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/AudioBuffer.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/AudioBuffer.cpp:47: return 3000.f; No .f required: http://www.chromium.org/blink/coding-style#TOC-Floating-point-literals https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/AudioBuffer.cpp:53: return 192000.f; Same as line 47. https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.cpp:71: "sampleRate", sampleRate, 3000.f, ExceptionMessages::InclusiveBound, 192000.f, ExceptionMessages::InclusiveBound)); Is the .f required here? I don't think it's important if we use an integer or float value in the message. https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:48: // Support sample rate from 3 kHz to 192 kHz. This comment seems unrelated to the following code. Update it to match the code. You probably want to explain why 32 and 4096 are appropriate limits on the fft size. https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:51: const float coefficientSampleRate = 2756.25f; No trialing "f" required according to blink style. You probably explained this to me, but where does the 2765.25 come from? You'll probably need to add a comment to say where this magic number comes from. https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:87: ASSERT(sampleRate >= 3000.f && sampleRate <= 192000.f); No ".f" allowed per blink style guide. https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:95: return scale * minfftSize; Don't you need to make sure the returned FFT size is a power of two?
On 2014/09/04 05:40:49, KhNo wrote: > On 2014/07/31 17:16:24, Raymond Toy wrote: > > On 2014/07/31 16:45:54, KhNo wrote: > > > PTAL, I have tested it manually, it doesn't have any problem since it is > same > > as > > > AudioBuffer already supported range. > > > > Sorry, I don't quite follow you here. What did you test manually? And what do > > you mean it has the same AudioBuffer supported range? AudioBuffers support > 3-192 > > kHz currently. > > > > As you know, currently AudioBuffer is already supporting the range of sampleRate > from 3 to 192 Khz. > But, offlineAudioContext was not considered even if it is using also same > AudioBuffer logic. > The isSamppleRateRangeGood() that is used for only offlineAudioContext restricts > the range of sample rate differently with AudioContext. > > In addition, separately, FFTSize for HRTFPanner needs to fix the range and > calculataon for correct size. > The sample rate was extended from (44100 ~ 96000) to (3000 ~ 192000). > However, FFTSize for each sampleRate was not considered. > Modification in Source/platform/audio/HRTFPanner.cpp will give correct fftsize > depend on AudioBuffer's sampleRate. > > I have manually created test application audiocontext/offline-audiocontext > create audio buffer with samplerate 3 and 192. > This buffer is passed to panner node, then goes out destination node. However, > It was difficult to make it layout test. I think one way to do this is the same way we do a layout test for the codecs. We have a reference result file saved. The layout test saves the generated audio data and run-webkit-tests will compare the actual data to the expected data. However, this method requires lots of manual comparisons and updates if there are slight changes to the code or compiler options. If possible, can we have a reference file and compare that against the actual generated data and use some kind of SNR and absolute difference metric? That makes it a bit more robust against small numerical changes. Also we should probably have one test for 44100 for sure, and then another test for whatever sample rate you think would make a good test.
PTAL. https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/of... File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/of... LayoutTests/webaudio/offlineaudiocontext-constructor.html:13: // Make sure we don't throw exception supported ranges of sample rate with min/max. On 2014/09/04 17:27:20, Raymond Toy wrote: > Fix up phrasing: > > // Make sure we don't throw exceptions for supported ranges of sample rates for > an OfflineAudioContext. Done. https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/of... LayoutTests/webaudio/offlineaudiocontext-constructor.html:16: // Make sure we don't throw exception supported ranges of sample rate. On 2014/09/04 17:27:20, Raymond Toy wrote: > Probably don't need this if you change the comment on line 13. Done. https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/AudioBuffer.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/AudioBuffer.cpp:47: return 3000.f; On 2014/09/04 17:27:20, Raymond Toy wrote: > No .f required: > http://www.chromium.org/blink/coding-style#TOC-Floating-point-literals Done. https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/AudioBuffer.cpp:53: return 192000.f; On 2014/09/04 17:27:20, Raymond Toy wrote: > Same as line 47. Done. https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.cpp:71: "sampleRate", sampleRate, 3000.f, ExceptionMessages::InclusiveBound, 192000.f, ExceptionMessages::InclusiveBound)); On 2014/09/04 17:27:20, Raymond Toy wrote: > Is the .f required here? I don't think it's important if we use an integer or > float value in the message. There is compile error. ../../third_party/WebKit/Source/bindings/core/v8/ExceptionMessages.h:105:19: note: candidate template ignored: deduced conflicting types for parameter 'NumberType' ('float' vs. 'int') I have added type casting to (float). https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:48: // Support sample rate from 3 kHz to 192 kHz. On 2014/09/04 17:27:20, Raymond Toy wrote: > This comment seems unrelated to the following code. Update it to match the code. > You probably want to explain why 32 and 4096 are appropriate limits on the fft > size. I changed logic to find correct fft size. Do not need more this magic values. https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:51: const float coefficientSampleRate = 2756.25f; On 2014/09/04 17:27:20, Raymond Toy wrote: > No trialing "f" required according to blink style. > > You probably explained this to me, but where does the 2765.25 come from? You'll > probably need to add a comment to say where this magic number comes from. Done. https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:87: ASSERT(sampleRate >= 3000.f && sampleRate <= 192000.f); On 2014/09/04 17:27:20, Raymond Toy wrote: > No ".f" allowed per blink style guide. Done. https://codereview.chromium.org/375383002/diff/140001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:95: return scale * minfftSize; On 2014/09/04 17:27:20, Raymond Toy wrote: > Don't you need to make sure the returned FFT size is a power of two? Yes, it is required. I think I missed it during update patch set. I changed the logic to find correct fft size from array that is guaranteed power of two. Thanks for detailed checking.
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.cpp:71: "sampleRate", sampleRate, 3000.f, ExceptionMessages::InclusiveBound, 192000.f, ExceptionMessages::InclusiveBound)); On 2014/09/05 05:03:25, KhNo wrote: > On 2014/09/04 17:27:20, Raymond Toy wrote: > > Is the .f required here? I don't think it's important if we use an integer or > > float value in the message. > > There is compile error. > ../../third_party/WebKit/Source/bindings/core/v8/ExceptionMessages.h:105:19: > note: candidate template ignored: deduced conflicting types for parameter > 'NumberType' ('float' vs. 'int') > > I have added type casting to (float). Oh, sorry about that! I think you should either use static_cast<float>(3000) or 3000.0f.
Looks pretty good. Just a few more comments. https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:84: ASSERT(sampleRate >= 3000 && sampleRate <= 192000); I think you should use AudioBuffer::minAllowedSampleRate() instead of 3000. https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:101: return fftSizes[index]; Since this is so simple, I think you should just delete the fftSizes array and instead of setting the index, just return the actual FFT size.
If you are not going to add a layout test for this in this CL, please file a bug that we need tests for the HRTF panner. Thanks!
On 2014/09/05 05:20:04, Raymond Toy wrote: > If you are not going to add a layout test for this in this CL, please file a bug > that we need tests for the HRTF panner. Thanks! Ok. I think that is little heavy layout test to implement. I will file a bug then implement it. Thanks for reviewing.
PTAL https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.cpp:71: "sampleRate", sampleRate, 3000.f, ExceptionMessages::InclusiveBound, 192000.f, ExceptionMessages::InclusiveBound)); On 2014/09/05 05:10:31, Raymond Toy wrote: > On 2014/09/05 05:03:25, KhNo wrote: > > On 2014/09/04 17:27:20, Raymond Toy wrote: > > > Is the .f required here? I don't think it's important if we use an integer > or > > > float value in the message. > > > > There is compile error. > > ../../third_party/WebKit/Source/bindings/core/v8/ExceptionMessages.h:105:19: > > note: candidate template ignored: deduced conflicting types for parameter > > 'NumberType' ('float' vs. 'int') > > > > I have added type casting to (float). > > Oh, sorry about that! I think you should either use static_cast<float>(3000) or > 3000.0f. No problem. https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:84: ASSERT(sampleRate >= 3000 && sampleRate <= 192000); On 2014/09/05 05:18:30, Raymond Toy wrote: > I think you should use AudioBuffer::minAllowedSampleRate() instead of 3000. Yes, I agree with that. However, current implementation can not call method module method.(AudioBuffer.cpp is in module/) from plotform/ to module/. So, now I'm preparing the next patch set of it. I will clean up all sample rate numeric to use get one method after remove dependancy libmodule and libplatform. https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:101: return fftSizes[index]; On 2014/09/05 05:18:30, Raymond Toy wrote: > Since this is so simple, I think you should just delete the fftSizes array and > instead of setting the index, just return the actual FFT size. Done.
On 2014/09/05 05:20:04, Raymond Toy wrote: > If you are not going to add a layout test for this in this CL, please file a bug > that we need tests for the HRTF panner. Thanks! https://code.google.com/p/chromium/issues/detail?id=411191 I filed a bug, I will keep find way to create layout tests as your tip.
https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:87: return fftSizes[0]; Why not just return 64 instead of looking it up in fftSizes[0]? Do the same for all of the other branches. https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:95: return fftSizes[4]; There's a difference here. In the original the FFT size is 512 if the sample rate is < 88200. You have 1024 here. I'm thinking maybe the boundaries should either be the average of the sample rates you have here, or maybe the geometric mean (sqrt(a*b)) of the sample rates. Thus we use a size of 512 for rates between (22050+44100)/2 and (44100+88200)/2. (33075 to 66150) Or do you think that a rate of 44101 really needs 1024 instead of 512? If you do use the average, be sure to use (22050+44100)/2 instead of 33075. It makes it easier to see where you got these magic numbers.
PTAL https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:87: return fftSizes[0]; On 2014/09/05 16:19:52, Raymond Toy wrote: > Why not just return 64 instead of looking it up in fftSizes[0]? Do the same for > all of the other branches. Sorry, I misunderstood your previous comment. I fixed https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:95: return fftSizes[4]; On 2014/09/05 16:19:52, Raymond Toy wrote: > There's a difference here. In the original the FFT size is 512 if the sample > rate is < 88200. You have 1024 here. > // So for sample rates around 44.1KHz an FFT size of 512 is good. The fftsize was 512 for 44100 kHz as above comment. The fftsize was 1024 for 88200 kHz since condition (sampleRate(88200) < 88200) is false, original code also had 1024 for 88200 kHz. > I'm thinking maybe the boundaries should either be the average of the sample > rates you have here, or maybe the geometric mean (sqrt(a*b)) of the sample > rates. Thus we use a size of 512 for rates between (22050+44100)/2 and > (44100+88200)/2. (33075 to 66150) > > Or do you think that a rate of 44101 really needs 1024 instead of 512? > Yes, it must have 1024 for 44101. If sampleRate is bigger than 44100, it needs 1024. fftsize must be a multiple of 2 since fftframe is supporting log2(fftsize) context size. So, unfortunately, I think it can not be used your way although 44101 just add 1 from 44100 for convolution. Do you think I'm still missing something or your comment. > If you do use the average, be sure to use (22050+44100)/2 instead of 33075. It > makes it easier to see where you got these magic numbers.
Patchset #10 (id:240001) has been deleted
https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:95: return fftSizes[4]; On 2014/09/05 20:18:14, KhNo wrote: > On 2014/09/05 16:19:52, Raymond Toy wrote: > > There's a difference here. In the original the FFT size is 512 if the sample > > rate is < 88200. You have 1024 here. > > > > // So for sample rates around 44.1KHz an FFT size of 512 is good. > The fftsize was 512 for 44100 kHz as above comment. > The fftsize was 1024 for 88200 kHz since condition (sampleRate(88200) < 88200) > is false, original code also had 1024 for 88200 kHz. > Please ignore above my comment, there is mistakes, you right. > > I'm thinking maybe the boundaries should either be the average of the sample > > rates you have here, or maybe the geometric mean (sqrt(a*b)) of the sample > > rates. Thus we use a size of 512 for rates between (22050+44100)/2 and > > (44100+88200)/2. (33075 to 66150) > > > > Or do you think that a rate of 44101 really needs 1024 instead of 512? > > > > Yes, it must have 1024 for 44101. If sampleRate is bigger than 44100, it needs > 1024. > fftsize must be a multiple of 2 since fftframe is supporting log2(fftsize) > context size. > > So, unfortunately, I think it can not be used your way although 44101 just add 1 > from 44100 for convolution. > > Do you think I'm still missing something or your comment. > > > > If you do use the average, be sure to use (22050+44100)/2 instead of 33075. It > > makes it easier to see where you got these magic numbers. >
https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:95: return fftSizes[4]; On 2014/09/05 20:22:52, KhNo wrote: > On 2014/09/05 20:18:14, KhNo wrote: > > On 2014/09/05 16:19:52, Raymond Toy wrote: > > > There's a difference here. In the original the FFT size is 512 if the sample > > > rate is < 88200. You have 1024 here. > > > > > > > // So for sample rates around 44.1KHz an FFT size of 512 is good. > > The fftsize was 512 for 44100 kHz as above comment. > > The fftsize was 1024 for 88200 kHz since condition (sampleRate(88200) < 88200) > > is false, original code also had 1024 for 88200 kHz. > > > > Please ignore above my comment, there is mistakes, you right. > > > > I'm thinking maybe the boundaries should either be the average of the sample > > > rates you have here, or maybe the geometric mean (sqrt(a*b)) of the sample > > > rates. Thus we use a size of 512 for rates between (22050+44100)/2 and > > > (44100+88200)/2. (33075 to 66150) > > > > > > Or do you think that a rate of 44101 really needs 1024 instead of 512? > > > > > > > Yes, it must have 1024 for 44101. If sampleRate is bigger than 44100, it needs > > 1024. > > fftsize must be a multiple of 2 since fftframe is supporting log2(fftsize) > > context size. > > > > So, unfortunately, I think it can not be used your way although 44101 just add > 1 > > from 44100 for convolution. > > > > Do you think I'm still missing something or your comment. > > > > > > > If you do use the average, be sure to use (22050+44100)/2 instead of 33075. > It > > > makes it easier to see where you got these magic numbers. > > > Ok. I'm not saying my proposal is the best way, but it seems to give something closer to the original. If you want to keep the original behavior with an FFT size of 512 for 22050 < rate < 88200, that would be ok with me too. I just don't have a good feel right now for what the FFT size should be for each sample rate.
PTAL
https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:82: if (sampleRate < 5512.5) Actually, I was just looking. I think we can do better than this. In HRTFElevation.cpp, we sample rate convert the responses to the new frequency. According to the comment in line 77, the response are 512 samples at 44100, but it seems we only use the first 256. When we resample the response, we will get a new length (as done in AudioBus::createBySampleRateConverting). We can use this length to set the fft size by rounding the length up to the nearest power of 2 and then multiply by 2. So for 44100, the length is 256, so the FFT size is 512. For 88200, the length would be 512, so the FFT size is 1024. Finally, for 44101, the length is truncate(256.0058) = 256, so the FFT size is 512. What do you think?
https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:82: if (sampleRate < 5512.5) On 2014/09/05 21:01:43, Raymond Toy wrote: > Actually, I was just looking. I think we can do better than this. > > In HRTFElevation.cpp, we sample rate convert the responses to the new frequency. > According to the comment in line 77, the response are 512 samples at 44100, but > it seems we only use the first 256. When we resample the response, we will get a > new length (as done in AudioBus::createBySampleRateConverting). We can use this > length to set the fft size by rounding the length up to the nearest power of 2 > and then multiply by 2. > > So for 44100, the length is 256, so the FFT size is 512. For 88200, the length > would be 512, so the FFT size is 1024. Finally, for 44101, the length is > truncate(256.0058) = 256, so the FFT size is 512. > > What do you think? int impulseLength = 256; float impulseSampleRate = 44100; double ratio = sampleRate / impulseSampleRate; return floor(impulseLength * ratio) * 2; Is it similar with your proposal? Am I right? It is good idea for explain where magic number is from. One question, fftSize only can be a 2^n. Do you have idea how we have to change the last line? return floor(impulseLength * ratio) * 2;
On 2014/09/05 21:29:17, KhNo wrote: > https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... > File Source/platform/audio/HRTFPanner.cpp (right): > > https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... > Source/platform/audio/HRTFPanner.cpp:82: if (sampleRate < 5512.5) > On 2014/09/05 21:01:43, Raymond Toy wrote: > > Actually, I was just looking. I think we can do better than this. > > > > In HRTFElevation.cpp, we sample rate convert the responses to the new > frequency. > > According to the comment in line 77, the response are 512 samples at 44100, > but > > it seems we only use the first 256. When we resample the response, we will get > a > > new length (as done in AudioBus::createBySampleRateConverting). We can use > this > > length to set the fft size by rounding the length up to the nearest power of 2 > > and then multiply by 2. > > > > So for 44100, the length is 256, so the FFT size is 512. For 88200, the length > > would be 512, so the FFT size is 1024. Finally, for 44101, the length is > > truncate(256.0058) = 256, so the FFT size is 512. > > > > What do you think? > > int impulseLength = 256; > float impulseSampleRate = 44100; > double ratio = sampleRate / impulseSampleRate; > > return floor(impulseLength * ratio) * 2; > > Is it similar with your proposal? Am I right? > It is good idea for explain where magic number is from. > > One question, fftSize only can be a 2^n. > Do you have idea how we have to change the last line? > return floor(impulseLength * ratio) * 2; You could do something like double resampledLength = impulseLength * ratio; return 2 * (1 << static_cast<unsigned>(log2(resampledLength))) But you have to be careful about rounding. Or just use a set of if's like you had before: // The next two lines are how AudioBus::createBySampleRateConverting // computes the resampled length. We might want to refactor that and add a // function to compute length so AudioBus and HRTFPanner can use it. double ratio = 44100 / sampleRate; int resampledLength = impulseLength / ratio; if (resampledLength <= 32) // I think the smallest resampledLength is 17 return 64; if (resampledLength <= 64) return 128; ... if (resampledLength <= 1024) return 2048; return 4096;
Please update the description to say 3 kHz instead of 8.(last line of description.) And please test this on some demos where we have set the sampling rate to something very low and very high. There shouldn't be any unexpected changes in sound quality. (It seems none of my machines can go below 44.1 but can go as high as 96 or 192.)
On 2014/09/05 22:17:47, Raymond Toy wrote: > On 2014/09/05 21:29:17, KhNo wrote: > > > https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... > > File Source/platform/audio/HRTFPanner.cpp (right): > > > > > https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/H... > > Source/platform/audio/HRTFPanner.cpp:82: if (sampleRate < 5512.5) > > On 2014/09/05 21:01:43, Raymond Toy wrote: > > > Actually, I was just looking. I think we can do better than this. > > > > > > In HRTFElevation.cpp, we sample rate convert the responses to the new > > frequency. > > > According to the comment in line 77, the response are 512 samples at 44100, > > but > > > it seems we only use the first 256. When we resample the response, we will > get > > a > > > new length (as done in AudioBus::createBySampleRateConverting). We can use > > this > > > length to set the fft size by rounding the length up to the nearest power of > 2 > > > and then multiply by 2. > > > > > > So for 44100, the length is 256, so the FFT size is 512. For 88200, the > length > > > would be 512, so the FFT size is 1024. Finally, for 44101, the length is > > > truncate(256.0058) = 256, so the FFT size is 512. > > > > > > What do you think? > > > > int impulseLength = 256; > > float impulseSampleRate = 44100; > > double ratio = sampleRate / impulseSampleRate; > > > > return floor(impulseLength * ratio) * 2; > > > > Is it similar with your proposal? Am I right? > > It is good idea for explain where magic number is from. > > > > One question, fftSize only can be a 2^n. > > Do you have idea how we have to change the last line? > > return floor(impulseLength * ratio) * 2; > > You could do something like > > double resampledLength = impulseLength * ratio; > return 2 * (1 << static_cast<unsigned>(log2(resampledLength))) > > But you have to be careful about rounding. Or just use a set of if's like you > had before: > > // The next two lines are how AudioBus::createBySampleRateConverting > // computes the resampled length. We might want to refactor that and add a > // function to compute length so AudioBus and HRTFPanner can use it. > double ratio = 44100 / sampleRate; > int resampledLength = impulseLength / ratio; > > > if (resampledLength <= 32) // I think the smallest resampledLength is 17 > return 64; > if (resampledLength <= 64) > return 128; > ... > if (resampledLength <= 1024) > return 2048; > return 4096; Thanks for guide. It was that I would like to do. :) I have double checked boundary values specially. fftSizeForSampleRate(3000.000000) = 32 fftSizeForSampleRate(8000.000000) = 64 fftSizeForSampleRate(11025.000000) = 128 fftSizeForSampleRate(22050.000000) = 256 fftSizeForSampleRate(44100.000000) = 512 fftSizeForSampleRate(48000.000000) = 512 fftSizeForSampleRate(88200.000000) = 1024 fftSizeForSampleRate(96000.000000) = 1024 fftSizeForSampleRate(176400.000000) = 2048 fftSizeForSampleRate(192000.000000) = 2048
LGTM with a few nits. But please test this with some demos with sample rates that differ from 44100. Good choices: 48000, 96000, 192000, if possible. Ideally try something less than 44100, but it seems none of my machines can do that (easily). https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:79: // So for sample rates around 44.1KHz an FFT size of 512 is good. We double the FFT-size only for sample rates at least double this. Nit: Remove the sentence beginning with "We double...". Probably replace that sentence with something like: For different sample rates, the truncated response is resampled. The resampled length is used to compute the FFT size by choosing a power of two that is greater than or equal the resampled length. This power of two is doubled to get the actual FFT size. https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:82: int impulseLength = 256; Nit: I think I'd call this truncatedImpulseLength, to kind of match the comments in line 78. https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:83: double ratio = sampleRate / 44100; Nit: Rename ratio to sampleRateRatio (to kind of match the names in AudioBus.cpp)
Raymond, thanks for review. Your comments always make code is getting better. Sometime, I feel why I couldn't do that before reviewing. :) After testing about sample rates less than 44.1kHz. It will go to merge. https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:79: // So for sample rates around 44.1KHz an FFT size of 512 is good. We double the FFT-size only for sample rates at least double this. On 2014/09/08 20:01:49, Raymond Toy wrote: > Nit: Remove the sentence beginning with "We double...". > > Probably replace that sentence with something like: > > For different sample rates, the truncated response is resampled. The resampled > length is used to compute the FFT size by choosing a power of two that is > greater than or equal the resampled length. This power of two is doubled to get > the actual FFT size. Done. https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:82: int impulseLength = 256; On 2014/09/08 20:01:49, Raymond Toy wrote: > Nit: I think I'd call this truncatedImpulseLength, to kind of match the comments > in line 78. Done. https://codereview.chromium.org/375383002/diff/280001/Source/platform/audio/H... Source/platform/audio/HRTFPanner.cpp:83: double ratio = sampleRate / 44100; On 2014/09/08 20:01:49, Raymond Toy wrote: > Nit: Rename ratio to sampleRateRatio (to kind of match the names in > AudioBus.cpp) Done.
I have tested many sample rates after creating some test html. audiobuffersourcenode -> panner -> hardware destination(pulse audio) In my machine, I can test from 4khz to 192khz. They are playing successfully. 3khz was not supported. I read crbug.com/344375. I couldn't find where 3khz is from. I think pulse audio sink usually support from 4khz. If you don't mind to change to 4khz, I will change it to 4khz.
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/375383002/300001
Message was sent while issue was closed.
Committed patchset #12 (id:300001) as 181816
Message was sent while issue was closed.
On 2014/09/11 11:59:53, KhNo wrote: > I have tested many sample rates after creating some test html. > audiobuffersourcenode -> panner -> hardware destination(pulse audio) > > In my machine, I can test from 4khz to 192khz. > They are playing successfully. > > 3khz was not supported. I read crbug.com/344375. > I couldn't find where 3khz is from. I think pulse audio sink usually support > from 4khz. > If you don't mind to change to 4khz, I will change it to 4khz. Thanks for doing these tests! I think 3kHz should be ok even if we can't test it. This makes us consistent with crbug.com/344375 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
