Move utility functions for sample rate to AudioUtilities.

Move utility functions for sample rate to AudioUtilities.

isSampleRateRangeGood() -> isValidAudioBufferSampleRate()
minAllowedSampleRate() -> minAudioBufferSampleRate()
maxAllowedSampleRate() -> maxAudioBufferSampleRate()

These functions have been moved to AudioUtilities.
Some const values were used in some files sporadically instead of them.
Make these functions using every places.

BUG=413244
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181947

[KhNo, 2014-09-11 14:17:06]

Patchset #1 (id:1) has been deleted

[KhNo, 2014-09-11 14:27:38]

keonho07.kim@samsung.com changed reviewers:
+ dalecurtis@chromium.org, rtoy@chromium.org

[KhNo, 2014-09-11 14:27:38]

PTAL.

[Raymond Toy, 2014-09-11 16:38:45]

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.cpp (right):

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.cpp:71: // Currently, we truncate the
impulse responses to half this size,
Need more context here because "this size" has no meaning anymore.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.h (right):

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:45: // Check the given sample rate is
supported by platform.
Be more descriptive. Something like

// Check that |sampleRate| is a valid rate for AudioBuffers.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:46: PLATFORM_EXPORT bool
isSampleRateRangeGood(float sampleRate);
Maybe a better name would be isValidAudioBufferSampleRate?

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:48: // Caculates correct fftsize with the
given sample rate.
Fix typo: Caculates -> Calculates.

And make it clear this is for the HRTF Panner, not just any old fft size.

And maybe rename it to hrtfFFTSizeForSampleRate to make it clearer.

Actually, I think it might be better to leave fftSizeForSampleRate in HRTFPanner
instead of moving it here.  It's really specific to the panner.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:51: // Renturn max/min sample rate that
platform supported.
Fix typo: Renturn -> Return.

And as for isSampleRateRangeGood, state this is for AudioBuffers

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:53: PLATFORM_EXPORT float
maxAllowedSampleRate();
Maybe rename these to minAudioBufferSampleRate and maxAudioBufferSampleRate?

[KhNo, 2014-09-12 14:51:18]

Patchset #2 (id:40001) has been deleted

[KhNo, 2014-09-12 14:57:42]

Patchset #2 (id:60001) has been deleted

[KhNo, 2014-09-12 15:01:05]

PTAL, Raymond.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.cpp (right):

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.cpp:71: // Currently, we truncate the
impulse responses to half this size,
On 2014/09/11 16:38:45, Raymond Toy wrote:
> Need more context here because "this size" has no meaning anymore.

Done.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.h (right):

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:45: // Check the given sample rate is
supported by platform.
On 2014/09/11 16:38:45, Raymond Toy wrote:
> Be more descriptive. Something like
> 
> // Check that |sampleRate| is a valid rate for AudioBuffers.

Done.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:46: PLATFORM_EXPORT bool
isSampleRateRangeGood(float sampleRate);
On 2014/09/11 16:38:45, Raymond Toy wrote:
> Maybe a better name would be isValidAudioBufferSampleRate?

Yes, it is for AudioBuffer.
I have done.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:48: // Caculates correct fftsize with the
given sample rate.
On 2014/09/11 16:38:45, Raymond Toy wrote:
> Fix typo: Caculates -> Calculates.
> 
> And make it clear this is for the HRTF Panner, not just any old fft size.
> 
> And maybe rename it to hrtfFFTSizeForSampleRate to make it clearer.
> 
> Actually, I think it might be better to leave fftSizeForSampleRate in
HRTFPanner
> instead of moving it here.  It's really specific to the panner.

I think you right. There is better position. I reverted it.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:51: // Renturn max/min sample rate that
platform supported.
On 2014/09/11 16:38:45, Raymond Toy wrote:
> Fix typo: Renturn -> Return.
> 
> And as for isSampleRateRangeGood, state this is for AudioBuffers

Done.

https://codereview.chromium.org/565643003/diff/20001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:53: PLATFORM_EXPORT float
maxAllowedSampleRate();
On 2014/09/11 16:38:45, Raymond Toy wrote:
> Maybe rename these to minAudioBufferSampleRate and maxAudioBufferSampleRate?

Done.

[Raymond Toy, 2014-09-12 15:56:55]

LGTM, with a few nits.

Thanks for consolidating these into one place.

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.cpp (right):

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.cpp:77: // Windows can support up to this
rate.
Nit: Not your fault, bu can you update this comment to say:

// Windows can support audio sampling rates this high, so allow AudioBuffer
rates this high as well.

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.h (right):

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:48: // Return max/min sample rate that
platform supported.
Nit: Fix comment:

// Return max/min sample rate supported by AudioBuffers.

[KhNo, 2014-09-12 16:17:12]

Thanks for review.

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.cpp (right):

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.cpp:77: // Windows can support up to this
rate.
On 2014/09/12 15:56:55, Raymond Toy wrote:
> Nit: Not your fault, bu can you update this comment to say:
> 
> // Windows can support audio sampling rates this high, so allow AudioBuffer
> rates this high as well.

Done.

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
File Source/platform/audio/AudioUtilities.h (right):

https://codereview.chromium.org/565643003/diff/80001/Source/platform/audio/Au...
Source/platform/audio/AudioUtilities.h:48: // Return max/min sample rate that
platform supported.
On 2014/09/12 15:56:55, Raymond Toy wrote:
> Nit: Fix comment:
> 
> // Return max/min sample rate supported by AudioBuffers.

Done.

[KhNo, 2014-09-12 16:17:18]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-09-12 16:18:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565643003/100001

[KhNo, 2014-09-12 17:41:04]

The CQ bit was unchecked by keonho07.kim@samsung.com

[KhNo, 2014-09-12 17:53:29]

Raymond, I have add modification in MediaElementSourceNode file.
Could you review it as well? I think it should be included here.

[Raymond Toy, 2014-09-12 18:24:06]

On 2014/09/12 17:53:29, KhNo wrote:
> Raymond, I have add modification in MediaElementSourceNode file.
> Could you review it as well? I think it should be included here.

Looks ok, but I'm not sure how this is going to work since you've already sent
this CL to the CQ. It's probably not a good idea to modify the CL while the CQ
is working on it.

So, wait until the CQ is done and let's see what was actually committed. If the
CQ fails, we can consider this again.

[DaleCurtis, 2014-09-12 18:25:48]

(uploading a new patch set cancels commit of the previous one, though it will
still run the bots)

[DaleCurtis, 2014-09-12 18:25:56]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@chromium.org

[KhNo, 2014-09-13 01:09:17]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-09-13 01:09:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565643003/120001

[commit-bot: I haz the power, 2014-09-13 02:15:51]

Committed patchset #4 (id:120001) as 181947