Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

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.

Description

Extending 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -11 lines) Patch
M LayoutTests/webaudio/dom-exceptions-expected.txt View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/webaudio/offlineaudiocontext-constructor.html View 1 2 3 4 5 6 1 chunk +10 lines, -0 lines 0 comments Download
M LayoutTests/webaudio/offlineaudiocontext-constructor-expected.txt View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
M Source/modules/webaudio/AudioContext.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -3 lines 0 comments Download
M Source/modules/webaudio/OfflineAudioContext.cpp View 1 2 3 4 5 7 1 chunk +3 lines, -2 lines 0 comments Download
M Source/platform/audio/HRTFPanner.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +14 lines, -4 lines 0 comments Download

Messages

Total messages: 44 (5 generated)
Inactive
Please provide a link to the spec in your CL description: http://webaudio.github.io/web-audio-api/#widl-AudioContext-createBuffer-AudioBuffer-unsigned-long-numberOfChannels-unsigned-long-length-float-sampleRate Can you add ...
6 years, 5 months ago (2014-07-15 15:45:29 UTC) #1
Inactive
https://codereview.chromium.org/375383002/diff/1/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/1/Source/platform/audio/HRTFPanner.cpp#newcode90 Source/platform/audio/HRTFPanner.cpp:90: else if (sampleRate < 88200.0) I am guessing you ...
6 years, 5 months ago (2014-07-15 15:49:40 UTC) #2
Raymond Toy
The title and description seems wrong. AudioBuffers already support sample rates down to 3 kHz ...
6 years, 5 months ago (2014-07-15 16:02:34 UTC) #3
KhNo
On 2014/07/15 15:45:29, Chris Dumez wrote: > Please provide a link to the spec in ...
6 years, 5 months ago (2014-07-15 16:55:08 UTC) #4
KhNo
On 2014/07/15 16:02:34, Raymond Toy wrote: > The title and description seems wrong. AudioBuffers already ...
6 years, 5 months ago (2014-07-15 17:21:10 UTC) #5
Raymond Toy
On 2014/07/15 17:21:10, KhNo wrote: > On 2014/07/15 16:02:34, Raymond Toy wrote: > > The ...
6 years, 5 months ago (2014-07-15 17:27:01 UTC) #6
KhNo
On 2014/07/15 17:27:01, Raymond Toy wrote: > On 2014/07/15 17:21:10, KhNo wrote: > > On ...
6 years, 5 months ago (2014-07-15 17:37:51 UTC) #7
Raymond Toy
On 2014/07/15 17:37:51, KhNo wrote: > On 2014/07/15 17:27:01, Raymond Toy wrote: > > On ...
6 years, 5 months ago (2014-07-15 18:04:23 UTC) #8
KhNo
PTAL. :)
6 years, 4 months ago (2014-07-27 14:15:32 UTC) #9
Raymond Toy
Looks good overall. One final nit: Change "Expending" to "Extending" in the description. https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/offlineaudiocontext-constructor.html File ...
6 years, 4 months ago (2014-07-30 16:49:38 UTC) #10
KhNo
PTAL, I have tested it manually, it doesn't have any problem since it is same ...
6 years, 4 months ago (2014-07-31 16:45:54 UTC) #11
Raymond Toy
On 2014/07/31 16:45:54, KhNo wrote: > PTAL, I have tested it manually, it doesn't have ...
6 years, 4 months ago (2014-07-31 17:16:24 UTC) #12
Raymond Toy
https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/offlineaudiocontext-constructor.html File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/80001/LayoutTests/webaudio/offlineaudiocontext-constructor.html#newcode22 LayoutTests/webaudio/offlineaudiocontext-constructor.html:22: shouldNotThrow("new OfflineAudioContext(2, 512, 96000)"); On 2014/07/31 16:45:53, KhNo wrote: ...
6 years, 4 months ago (2014-07-31 17:25:13 UTC) #13
KhNo
On 2014/07/31 17:16:24, Raymond Toy wrote: > On 2014/07/31 16:45:54, KhNo wrote: > > PTAL, ...
6 years, 3 months ago (2014-09-04 05:40:49 UTC) #15
Raymond Toy
https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/offlineaudiocontext-constructor.html File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/offlineaudiocontext-constructor.html#newcode13 LayoutTests/webaudio/offlineaudiocontext-constructor.html:13: // Make sure we don't throw exception supported ranges ...
6 years, 3 months ago (2014-09-04 17:27:20 UTC) #17
Raymond Toy
On 2014/09/04 05:40:49, KhNo wrote: > On 2014/07/31 17:16:24, Raymond Toy wrote: > > On ...
6 years, 3 months ago (2014-09-04 17:32:53 UTC) #18
KhNo
PTAL. https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/offlineaudiocontext-constructor.html File LayoutTests/webaudio/offlineaudiocontext-constructor.html (right): https://codereview.chromium.org/375383002/diff/140001/LayoutTests/webaudio/offlineaudiocontext-constructor.html#newcode13 LayoutTests/webaudio/offlineaudiocontext-constructor.html:13: // Make sure we don't throw exception supported ...
6 years, 3 months ago (2014-09-05 05:03:26 UTC) #19
Raymond Toy
https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio/OfflineAudioContext.cpp File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio/OfflineAudioContext.cpp#newcode71 Source/modules/webaudio/OfflineAudioContext.cpp:71: "sampleRate", sampleRate, 3000.f, ExceptionMessages::InclusiveBound, 192000.f, ExceptionMessages::InclusiveBound)); On 2014/09/05 05:03:25, ...
6 years, 3 months ago (2014-09-05 05:10:31 UTC) #21
Raymond Toy
Looks pretty good. Just a few more comments. https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/180001/Source/platform/audio/HRTFPanner.cpp#newcode84 Source/platform/audio/HRTFPanner.cpp:84: ASSERT(sampleRate ...
6 years, 3 months ago (2014-09-05 05:18:30 UTC) #22
Raymond Toy
If you are not going to add a layout test for this in this CL, ...
6 years, 3 months ago (2014-09-05 05:20:04 UTC) #23
KhNo
On 2014/09/05 05:20:04, Raymond Toy wrote: > If you are not going to add a ...
6 years, 3 months ago (2014-09-05 05:32:04 UTC) #24
KhNo
PTAL https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio/OfflineAudioContext.cpp File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/375383002/diff/140001/Source/modules/webaudio/OfflineAudioContext.cpp#newcode71 Source/modules/webaudio/OfflineAudioContext.cpp:71: "sampleRate", sampleRate, 3000.f, ExceptionMessages::InclusiveBound, 192000.f, ExceptionMessages::InclusiveBound)); On 2014/09/05 ...
6 years, 3 months ago (2014-09-05 05:32:39 UTC) #25
KhNo
On 2014/09/05 05:20:04, Raymond Toy wrote: > If you are not going to add a ...
6 years, 3 months ago (2014-09-05 07:53:02 UTC) #26
Raymond Toy
https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp#newcode87 Source/platform/audio/HRTFPanner.cpp:87: return fftSizes[0]; Why not just return 64 instead of ...
6 years, 3 months ago (2014-09-05 16:19:52 UTC) #27
KhNo
PTAL https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp#newcode87 Source/platform/audio/HRTFPanner.cpp:87: return fftSizes[0]; On 2014/09/05 16:19:52, Raymond Toy wrote: ...
6 years, 3 months ago (2014-09-05 20:18:14 UTC) #28
KhNo
https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp#newcode95 Source/platform/audio/HRTFPanner.cpp:95: return fftSizes[4]; On 2014/09/05 20:18:14, KhNo wrote: > On ...
6 years, 3 months ago (2014-09-05 20:22:52 UTC) #30
Raymond Toy
https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/220001/Source/platform/audio/HRTFPanner.cpp#newcode95 Source/platform/audio/HRTFPanner.cpp:95: return fftSizes[4]; On 2014/09/05 20:22:52, KhNo wrote: > On ...
6 years, 3 months ago (2014-09-05 20:28:04 UTC) #31
KhNo
PTAL
6 years, 3 months ago (2014-09-05 20:49:41 UTC) #32
Raymond Toy
https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/HRTFPanner.cpp#newcode82 Source/platform/audio/HRTFPanner.cpp:82: if (sampleRate < 5512.5) Actually, I was just looking. ...
6 years, 3 months ago (2014-09-05 21:01:43 UTC) #33
KhNo
https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/HRTFPanner.cpp File Source/platform/audio/HRTFPanner.cpp (right): https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/HRTFPanner.cpp#newcode82 Source/platform/audio/HRTFPanner.cpp:82: if (sampleRate < 5512.5) On 2014/09/05 21:01:43, Raymond Toy ...
6 years, 3 months ago (2014-09-05 21:29:17 UTC) #34
Raymond Toy
On 2014/09/05 21:29:17, KhNo wrote: > https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/HRTFPanner.cpp > File Source/platform/audio/HRTFPanner.cpp (right): > > https://codereview.chromium.org/375383002/diff/260001/Source/platform/audio/HRTFPanner.cpp#newcode82 > ...
6 years, 3 months ago (2014-09-05 22:17:47 UTC) #35
Raymond Toy
Please update the description to say 3 kHz instead of 8.(last line of description.) And ...
6 years, 3 months ago (2014-09-05 22:53:03 UTC) #36
KhNo
On 2014/09/05 22:17:47, Raymond Toy wrote: > On 2014/09/05 21:29:17, KhNo wrote: > > > ...
6 years, 3 months ago (2014-09-06 14:17:53 UTC) #37
Raymond Toy
LGTM with a few nits. But please test this with some demos with sample rates ...
6 years, 3 months ago (2014-09-08 20:01:49 UTC) #38
KhNo
Raymond, thanks for review. Your comments always make code is getting better. Sometime, I feel ...
6 years, 3 months ago (2014-09-10 12:59:06 UTC) #39
KhNo
I have tested many sample rates after creating some test html. audiobuffersourcenode -> panner -> ...
6 years, 3 months ago (2014-09-11 11:59:53 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/375383002/300001
6 years, 3 months ago (2014-09-11 12:01:11 UTC) #42
commit-bot: I haz the power
Committed patchset #12 (id:300001) as 181816
6 years, 3 months ago (2014-09-11 13:05:59 UTC) #43
Raymond Toy
6 years, 3 months ago (2014-09-11 16:41:21 UTC) #44
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

Powered by Google App Engine
This is Rietveld 408576698