Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750543003:
Support AudioContextOptions latencyHint as double. (Closed)
|
Created:
3 years, 9 months ago by Andrew MacPherson Modified:
3 years, 8 months ago CC:
blink-reviews, chromium-reviews, darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, feature-media-reviews_chromium.org, haraken, jam, jochen+watch_chromium.org, kinuko+watch, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, Peter Beverloo, posciak+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport AudioContextOptions latencyHint as double.
Follow-up to https://codereview.chromium.org/2501863003
This adds support for the latencyHint value to be passed as a double in
order to request an exact latency amount.
BUG=564276
R=rtoy@chromium.org
Review-Url: https://codereview.chromium.org/2750543003
Cr-Commit-Position: refs/heads/master@{#464988}
Committed: https://chromium.googlesource.com/chromium/src/+/67c41de111691e8cfb173c6cb1f31ee1d24b5dfe
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fixes based on reviewer comments. #
Total comments: 12
Patch Set 3 : More updates based on reviewer feedback. #
Total comments: 1
Patch Set 4 : Round latencyHint bufsize to multiple of hw size. #Patch Set 5 : Fix audiocontextoptions LayoutTest. #
Total comments: 6
Patch Set 6 : More updates based on reviewer comments. #
Total comments: 10
Patch Set 7 : Fix buffer size calculations based on feedback. #
Total comments: 5
Patch Set 8 : Undo "power of 2" change, fix method params. #Patch Set 9 : No max value clamping in AudioLatency. #
Total comments: 2
Patch Set 10 : Update TODO with userid and crbug. #
Total comments: 18
Patch Set 11 : Fixes based on reviewer comments. #Patch Set 12 : Add comments on latencyHint as double value. #Patch Set 13 : Rebase after Blink rename. #Patch Set 14 : Update baseLatency to equal HW latency. #
Total comments: 8
Patch Set 15 : Refactor audiocontextoptions LayoutTest. #Messages
Total messages: 71 (16 generated)
andrew.macpherson@soundtrap.com changed reviewers: + olka@chromium.org
Hi rtoy@ and olka@, This is the initial CL for the latencyHint "exact" type, I'm putting it up for discussion as some parts aren't yet clear to me. I've added a couple of comments in the code, two other points are: - I'm clamping the "exact" values to be between "interactive" and "playback". Right now interactive implies the hardware buffer size so I don't think allowing a value lower than that currently makes sense, and we need some sort of upper bound at the very least because values greater than AudioDestination::kFIFOSize will crash. I'm not sure if this is the right thing to be doing though so comments/thoughts welcome here. - Related to that point what I think we'd really like is to decouple "interactive" from the minimum/hardware buffer size, so that for example we can leave default buffer size values as they are now for most users but allow people to request lower than "interactive" buffer sizes via this explicit latencyHint value when very low-latency audio is required. I suspect this is too big a change for this particular CL but just mentioning it as it's the direction we're most interested in going here, if what I'm thinking makes sense. One other small one: - audio_renderer_mixer_manager.cc:171: May need to fix this so that latency_map_ is updated correctly for the "exact" type. Let me know what you think and thanks again! https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_mixer_manager.h:123: } else if (a.latency != b.latency) In audio_renderer_mixer_manager_unittest.cc it's expected that changing the 'latency' value will use a new mixer, even if the underlying buffer size is the same. Should this still be the case, or should this just be a check for GetBufferDuration() in all cases now without looking at the specific 'latency' value? https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:85: m_callbackBufferSize = m_webAudioDevice->framesPerBuffer(); This CL changed m_callbackBufferSize to hardwareBufferSize(), which breaks latencyHint: https://codereview.chromium.org/2740103005 I've reverted this here but we'll need to figure out why it was needed, and also why the unit tests missed pointing out the breakage.
hongchan@chromium.org changed reviewers: + hongchan@chromium.org
https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:85: m_callbackBufferSize = m_webAudioDevice->framesPerBuffer(); On 2017/03/14 12:03:03, Andrew MacPherson wrote: > This CL changed m_callbackBufferSize to hardwareBufferSize(), which breaks > latencyHint: https://codereview.chromium.org/2740103005 I've reverted this here > but we'll need to figure out why it was needed, and also why the unit tests > missed pointing out the breakage. If you rebase the patch, you'll see this is fixed. However, we're still missing the test coverage as you mentioned.
Thanks for working on this! https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_mixer_manager.cc:71: hardware_params.frames_per_buffer()); Blink has a clampTo() function that's easier to read than this. Don't know if there's a Chromium equivalent. Also, you do very similar clamping in several places. Perhaps refactoring this clamping into its own function would be good. https://codereview.chromium.org/2750543003/diff/1/content/shell/renderer/layo... File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/shell/renderer/layo... content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:201: const double hw_sample_rate = 44100; Where do these numbers come from? And why these particular values? https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right): https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:62: Should("double-constructor baseLatency ok", context.baseLatency).beEqualTo(0.02); Doesn't this really depend on the actual sampleRate of the audio context? I run the layout tests in a browser all the time, and the context sample rate changes quite a bit when I'm experimenting. Usually 44.1 or 48 kHz, but I sometimes use 88.2, 96, or even 192. https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:66: Should("context = new AudioContext({'latencyHint': 5})", function () { Nit: I think it would be slightly better to compute this instead of a fixed 5. Use something like 2*playbackLatency or something definitely bigger than playbackLatency.
On 2017/03/14 12:03:03, Andrew MacPherson wrote: > Hi rtoy@ and olka@, > > This is the initial CL for the latencyHint "exact" type, I'm putting it up for > discussion as some parts aren't yet clear to me. I've added a couple of comments > in the code, two other points are: > > - I'm clamping the "exact" values to be between "interactive" and "playback". > Right now interactive implies the hardware buffer size so I don't think allowing > a value lower than that currently makes sense, and we need some sort of upper > bound at the very least because values greater than AudioDestination::kFIFOSize > will crash. I'm not sure if this is the right thing to be doing though so > comments/thoughts welcome here. > > - Related to that point what I think we'd really like is to decouple > "interactive" from the minimum/hardware buffer size, so that for example we can > leave default buffer size values as they are now for most users but allow people > to request lower than "interactive" buffer sizes via this explicit latencyHint > value when very low-latency audio is required. I suspect this is too big a > change for this particular CL but just mentioning it as it's the direction we're > most interested in going here, if what I'm thinking makes sense. > As the spec currently stands, we're free to do whatever we want that makes sense for the latency value. So, I'm ok if we allow latency values that are smaller than "interactive". Of course, this has a lower bound of 128 frames, which is the minimum size for WebAudio. On mac, it would be nice to be able to have 128 frames again instead of the current 256, if the user wanted it. On Android, the interactive latency is really larger (2048 and up), but I know some devices like a Nexus 4 and 9 can do 256 frames without any real trouble. (We'll try lowering the limits on Android in a different CL, which won't affect this CL.) And I agree that we need an upper bound. A limit of "playback" seems good, but maybe we can allow a larger value if the user really wants it?
https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_mixer_manager.cc:68: media::AudioLatency::GetHighLatencyBufferSize( HighLatencyBufferSize is 20 ms. Is it the the maximum value WebAudio want to have? rtoy@: latencies more than 10 ms will expose https://bugs.chromium.org/p/chromium/issues/detail?id=701000 - something to be aware of. I marked it as blocking https://bugs.chromium.org/p/chromium/issues/detail?id=564276 (which does not have an owner?) https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_mixer_manager.h:123: } else if (a.latency != b.latency) On 2017/03/14 12:03:03, Andrew MacPherson wrote: > In audio_renderer_mixer_manager_unittest.cc it's expected that changing the > 'latency' value will use a new mixer, even if the underlying buffer size is the > same. Should this still be the case, or should this just be a check for > GetBufferDuration() in all cases now without looking at the specific 'latency' > value? Let's not touch mixers at all. WebAudio is not supposed to go through them as of now - the experiment for that is closed. If later it's re-opened, we'll hit a DCHECK and fix the code. https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/rend... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/rend... content/renderer/media/renderer_webaudiodevice_impl.cc:136: hardware_params.frames_per_buffer()); Looks like we need something like media::AudioLatency::GetExactBufferSize()?
On 2017/03/14 15:26:49, Raymond Toy wrote: > On 2017/03/14 12:03:03, Andrew MacPherson wrote: > > Hi rtoy@ and olka@, > > > > This is the initial CL for the latencyHint "exact" type, I'm putting it up for > > discussion as some parts aren't yet clear to me. I've added a couple of > comments > > in the code, two other points are: > > > > - I'm clamping the "exact" values to be between "interactive" and "playback". > > Right now interactive implies the hardware buffer size so I don't think > allowing > > a value lower than that currently makes sense, and we need some sort of upper > > bound at the very least because values greater than > AudioDestination::kFIFOSize > > will crash. I'm not sure if this is the right thing to be doing though so > > comments/thoughts welcome here. > > > > - Related to that point what I think we'd really like is to decouple > > "interactive" from the minimum/hardware buffer size, so that for example we > can > > leave default buffer size values as they are now for most users but allow > people > > to request lower than "interactive" buffer sizes via this explicit latencyHint > > value when very low-latency audio is required. I suspect this is too big a > > change for this particular CL but just mentioning it as it's the direction > we're > > most interested in going here, if what I'm thinking makes sense. > > > As the spec currently stands, we're free to do whatever we want that makes sense > for the latency value. So, I'm ok if we allow latency values that are smaller > than "interactive". Of course, this has a lower bound of 128 frames, which is > the minimum size for WebAudio. On mac, it would be nice to be able to have 128 > frames again instead of the current 256, if the user wanted it. On Android, the > interactive latency is really larger (2048 and up), but I know some devices like > a Nexus 4 and 9 can do 256 frames without any real trouble. (We'll try lowering > the limits on Android in a different CL, which won't affect this CL.) > > And I agree that we need an upper bound. A limit of "playback" seems good, but > maybe we can allow a larger value if the user really wants it? Allowing the values smaller than interactive won't necessarily change output buffer size. It may require changes in platform-specific AudioManager implementations.
https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_mixer_manager.cc:68: media::AudioLatency::GetHighLatencyBufferSize( On 2017/03/14 15:55:43, o1ka wrote: > HighLatencyBufferSize is 20 ms. Is it the the maximum value WebAudio want to > have? > > rtoy@: latencies more than 10 ms will expose > https://bugs.chromium.org/p/chromium/issues/detail?id=701000 - something to be > aware of. I marked it as blocking > https://bugs.chromium.org/p/chromium/issues/detail?id=564276 (which does not > have an owner?) Is 20ms the value for "playback"? In any case, there are Android devices with huge HW buffer sizes, much greater than 20 ms long. We need to support these. IIRC Andrew said in some cases Android "interactive" was greater than "balanced". We'll need to fix these issus (in a different CL).
On 2017/03/14 16:14:35, Raymond Toy wrote: > https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... > File content/renderer/media/audio_renderer_mixer_manager.cc (right): > > https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... > content/renderer/media/audio_renderer_mixer_manager.cc:68: > media::AudioLatency::GetHighLatencyBufferSize( > On 2017/03/14 15:55:43, o1ka wrote: > > HighLatencyBufferSize is 20 ms. Is it the the maximum value WebAudio want to > > have? > > > > rtoy@: latencies more than 10 ms will expose > > https://bugs.chromium.org/p/chromium/issues/detail?id=701000 - something to be > > aware of. I marked it as blocking > > https://bugs.chromium.org/p/chromium/issues/detail?id=564276 (which does not > > have an owner?) > > Is 20ms the value for "playback"? > > In any case, there are Android devices with huge HW buffer sizes, much greater > than 20 ms long. We need to support these. > > IIRC Andrew said in some cases Android "interactive" was greater than > "balanced". We'll need to fix these issus (in a different CL). Here are all the buffer sizes: https://cs.chromium.org/chromium/src/media/base/audio_latency.cc?type=cs&q=me... AFAIK Platform output buffer size is fine-tuned per platform to provide optimal "interactive" playback latency - except of Android which has this extra calculation for interactive.
On 2017/03/14 16:25:29, o1ka wrote: > On 2017/03/14 16:14:35, Raymond Toy wrote: > > > https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... > > File content/renderer/media/audio_renderer_mixer_manager.cc (right): > > > > > https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... > > content/renderer/media/audio_renderer_mixer_manager.cc:68: > > media::AudioLatency::GetHighLatencyBufferSize( > > On 2017/03/14 15:55:43, o1ka wrote: > > > HighLatencyBufferSize is 20 ms. Is it the the maximum value WebAudio want to > > > have? > > > > > > rtoy@: latencies more than 10 ms will expose > > > https://bugs.chromium.org/p/chromium/issues/detail?id=701000 - something to > be > > > aware of. I marked it as blocking > > > https://bugs.chromium.org/p/chromium/issues/detail?id=564276 (which does not > > > have an owner?) > > > > Is 20ms the value for "playback"? > > > > In any case, there are Android devices with huge HW buffer sizes, much greater > > than 20 ms long. We need to support these. > > > > IIRC Andrew said in some cases Android "interactive" was greater than > > "balanced". We'll need to fix these issus (in a different CL). > > Here are all the buffer sizes: > https://cs.chromium.org/chromium/src/media/base/audio_latency.cc?type=cs&q=me... > AFAIK Platform output buffer size is fine-tuned per platform to provide optimal > "interactive" playback latency - except of Android which has this extra > calculation for interactive. Yeah, this needs to be fixed. Interactive shouldn't be longer than the "balanced" RTC sizes. We'll have to do some tests where the HW buffer sizes are greater than the RTC sizes and see what happens if we choose the RTC sizes instead. AFAIK, these huge sizes only happen when Android reports that the low-latency audio path isn't supported. The original N7 doesn't support the low-latency path, but webaudio worked pretty well with a size of 256 or so.
Thanks for taking a look olka, rtoy and hongchan! On 2017/03/14 at 15:59:53, olka wrote: > On 2017/03/14 15:26:49, Raymond Toy wrote: > > As the spec currently stands, we're free to do whatever we want that makes sense > > for the latency value. So, I'm ok if we allow latency values that are smaller > > than "interactive". Of course, this has a lower bound of 128 frames, which is > > the minimum size for WebAudio. On mac, it would be nice to be able to have 128 > > frames again instead of the current 256, if the user wanted it. On Android, the > > interactive latency is really larger (2048 and up), but I know some devices like > > a Nexus 4 and 9 can do 256 frames without any real trouble. (We'll try lowering > > the limits on Android in a different CL, which won't affect this CL.) > > > > And I agree that we need an upper bound. A limit of "playback" seems good, but > > maybe we can allow a larger value if the user really wants it? > > Allowing the values smaller than interactive won't necessarily change output buffer size. It may require changes in platform-specific AudioManager implementations. What olka mentions is the reason I don't think it makes sense to allow buffer sizes lower than 'interactive' right now. What we'd like to see is the minimums pushed down in the AudioManager implementations (for OSX and ChromeOS, in a subsequent CL), even if we left the 'interactive' size at the per-platform values it has today. Then someone requesting a lower value explicitly would be able to use it (keeping in mind the WebAudio lower bound of 128). For the 'playback' upper bound this can certainly be higher but I wasn't sure how best to determine the limit if we don't use the 'playback' value. Some AudioManagers seem to have a maximum output buffer size specified but I don't see any way to access these values where we need them. Any thoughts? https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_mixer_manager.cc:71: hardware_params.frames_per_buffer()); On 2017/03/14 at 15:19:39, Raymond Toy wrote: > Blink has a clampTo() function that's easier to read than this. Don't know if there's a Chromium equivalent. > > Also, you do very similar clamping in several places. Perhaps refactoring this clamping into its own function would be good. Makes sense, I've refactored the AudioContextTest in Blink to use clampTo() and I've added a media::AudioLatency::GetExactBufferSize() as suggested by olka for the Chromium side. https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_mixer_manager.h:123: } else if (a.latency != b.latency) On 2017/03/14 at 15:55:43, o1ka wrote: > Let's not touch mixers at all. WebAudio is not supposed to go through them as of now - the experiment for that is closed. If later it's re-opened, we'll hit a DCHECK and fix the code. Ok great, I've reverted this now. https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/rend... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/renderer/media/rend... content/renderer/media/renderer_webaudiodevice_impl.cc:136: hardware_params.frames_per_buffer()); On 2017/03/14 at 15:55:43, o1ka wrote: > Looks like we need something like media::AudioLatency::GetExactBufferSize()? Done, I've added this now taking a parameter for the preferred size, min, and max, let me know if that makes sense! https://codereview.chromium.org/2750543003/diff/1/content/shell/renderer/layo... File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc (right): https://codereview.chromium.org/2750543003/diff/1/content/shell/renderer/layo... content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:201: const double hw_sample_rate = 44100; On 2017/03/14 at 15:19:39, Raymond Toy wrote: > Where do these numbers come from? And why these particular values? These are completely arbitrary, my understanding is that the WebAudioDevice has its own default hardware sample rate and buffer size so this is creating a mock of a device that has these particular values. Would there be a better way to handle this? https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right): https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:62: Should("double-constructor baseLatency ok", context.baseLatency).beEqualTo(0.02); On 2017/03/14 at 15:19:39, Raymond Toy wrote: > Doesn't this really depend on the actual sampleRate of the audio context? I run the layout tests in a browser all the time, and the context sample rate changes quite a bit when I'm experimenting. Usually 44.1 or 48 kHz, but I sometimes use 88.2, 96, or even 192. Hmm, I think I may not understand where the values here come from then. I thought that the sample rate was always the one provided in the layout_test_content_renderer_client.cc but I can take a closer look. Agreed that the code is sloppy though, I've refactored it to use numbers based on existing known latency type values rather than these hardcoded ones. https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:66: Should("context = new AudioContext({'latencyHint': 5})", function () { On 2017/03/14 at 15:19:39, Raymond Toy wrote: > Nit: I think it would be slightly better to compute this instead of a fixed 5. Use something like 2*playbackLatency or something definitely bigger than playbackLatency. Done! https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2750543003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:85: m_callbackBufferSize = m_webAudioDevice->framesPerBuffer(); On 2017/03/14 at 14:36:34, hongchan wrote: > If you rebase the patch, you'll see this is fixed. However, we're still missing the test coverage as you mentioned. Great, just rebased now and can see this, thanks! Regarding the unit tests it looks like because the mock doesn't try to render anything the render() method is never called and the crashing CHECK_EQ() is never hit. I tried to very trivially force it to render a fake frame and ran into issues around thread checking so left it for now. I assume it may require refactoring the render() method a bit to make it more unit testable and wanted to check with you before doing anything there.
>>For the 'playback' upper bound this can certainly be higher but I wasn't sure >>how best to determine the limit if we don't use the 'playback' value. Some >>AudioManagers seem to have a maximum output buffer size specified but I don't >>see any way to access these values where we need them. Any thoughts? There may be some platform-specific calculations required, see high latency buffer size. https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:109: int output_buffer_size = 0; Would be nice to wrap |output_buffer_size| calculation into a helper function GetOutputBufferSize(latency, hardware_params) https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:130: output_buffer_size = output_buffer_size = extra "output_buffer_size ="? https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:135: hardware_params.frames_per_buffer()); Can it be GetExactBufferSize(duration, hardware_params)? https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:144: hardware_params.sample_rate(), 16, output_buffer_size); We need top-to-bottom browser tests to see if a platform can deal with certain buffer sizes and how audio quality looks like. https://codereview.chromium.org/2750543003/diff/20001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/20001/media/base/audio_latenc... media/base/audio_latency.cc:129: int AudioLatency::GetExactBufferSize(int preferred_buffer_size, Take a look at AudioLatency::GetHighLatencyBufferSize(): I'm not sure if every platform will be happy with a buffer size like 101, for example. This needs some investigation and testing.
Thanks olka! https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:109: int output_buffer_size = 0; On 2017/03/16 at 10:51:23, o1ka wrote: > Would be nice to wrap |output_buffer_size| calculation into a helper function GetOutputBufferSize(latency, hardware_params) Done, I had to pass the latency_hint as well since we need to access its .seconds() method if its type is 'exact', even though this is a bit redundant. The alternative is to create 'latency' in both the constructor and GetOutputBufferSize since it's needed in both places. https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:130: output_buffer_size = output_buffer_size = On 2017/03/16 at 10:51:23, o1ka wrote: > extra "output_buffer_size ="? Oops, fixed! https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:135: hardware_params.frames_per_buffer()); On 2017/03/16 at 10:51:24, o1ka wrote: > Can it be GetExactBufferSize(duration, hardware_params)? Done. https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:144: hardware_params.sample_rate(), 16, output_buffer_size); On 2017/03/16 at 10:51:24, o1ka wrote: > We need top-to-bottom browser tests to see if a platform can deal with certain buffer sizes and how audio quality looks like. Do you mean like a renderer_webaudiodevice_impl_browsertest.cc or something else? I can certainly look into adding this and making it try unusual buffer sizes, I might need some pointers around adding checks for audio quality though. https://codereview.chromium.org/2750543003/diff/20001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/20001/media/base/audio_latenc... media/base/audio_latency.cc:129: int AudioLatency::GetExactBufferSize(int preferred_buffer_size, On 2017/03/16 at 10:51:24, o1ka wrote: > Take a look at AudioLatency::GetHighLatencyBufferSize(): I'm not sure if every platform will be happy with a buffer size like 101, for example. This needs some investigation and testing. Interesting, should we do this for Exact then (use a power of 2) or should the testing be done first to confirm that it's an issue? Would the browser tests you mentioned in the earlier comment be enough to cover this?
Description was changed from ========== Support AudioContextOptions latencyHint as double. Follow-up to https://codereview.chromium.org/2501863003 This adds support for the latencyHint value to be passed as a double in order to request an exact latency amount. BUG=564276 R=rtoy@chromium.org ========== to ========== Support AudioContextOptions latencyHint as double. Follow-up to https://codereview.chromium.org/2501863003 This adds support for the latencyHint value to be passed as a double in order to request an exact latency amount. BUG=564276 R=rtoy@chromium.org ==========
dalecurtis@: Is it ok for each of the supported platforms if AudioOutputDevice is configured with an arbitrary buffer size, or do you see any issues with it? (I assume that platform-specific implementations of AudioOutputStream should take care of that, but comments like this https://cs.chromium.org/chromium/src/media/base/audio_latency.cc?type=cs&l=42 make me doubt) Should we do some testing for arbitrary buffer sizes? https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:144: hardware_params.sample_rate(), 16, output_buffer_size); On 2017/03/16 17:02:09, Andrew MacPherson wrote: > On 2017/03/16 at 10:51:24, o1ka wrote: > > We need top-to-bottom browser tests to see if a platform can deal with certain > buffer sizes and how audio quality looks like. > > Do you mean like a renderer_webaudiodevice_impl_browsertest.cc or something > else? I can certainly look into adding this and making it try unusual buffer > sizes, I might need some pointers around adding checks for audio quality though. Let's see what dalecurtis@ thinks about testing it. https://codereview.chromium.org/2750543003/diff/20001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/20001/media/base/audio_latenc... media/base/audio_latency.cc:129: int AudioLatency::GetExactBufferSize(int preferred_buffer_size, On 2017/03/16 17:02:09, Andrew MacPherson wrote: > On 2017/03/16 at 10:51:24, o1ka wrote: > > Take a look at AudioLatency::GetHighLatencyBufferSize(): I'm not sure if every > platform will be happy with a buffer size like 101, for example. This needs some > investigation and testing. > > Interesting, should we do this for Exact then (use a power of 2) or should the > testing be done first to confirm that it's an issue? Would the browser tests you > mentioned in the earlier comment be enough to cover this? dalecurtis@ - WDYT? https://codereview.chromium.org/2750543003/diff/40001/media/base/audio_latency.h File media/base/audio_latency.h (right): https://codereview.chromium.org/2750543003/diff/40001/media/base/audio_latenc... media/base/audio_latency.h:41: static int GetExactBufferSize(double duration, |duration_in_seconds| would be more descriptive
Friendly ping for dalecurtis@ re: olka@'s questions around testing for different audio buffer sizes, and thanks!
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
When we originally discussed this proposal it was my understanding that the latency hint is just that, a hint. It's exact shape will by necessity, fixed Windows 10ms callbacks, power of 2 buffer sizes, be platform specific. Olga is right to point out the code in the high latency calculations, all calculated values from the latency hint should follow that model. I.e., multiples of 10ms on Windows, powers of 2 on other platforms (ideally multiples of the minimum supported buffer sizes). Doing otherwise will result in back-to-back buffering callbacks during which the client may be unprepared to respond.
On 2017/03/22 17:57:49, DaleCurtis wrote: > When we originally discussed this proposal it was my understanding that the > latency hint is just that, a hint. It's exact shape will by necessity, fixed > Windows 10ms callbacks, power of 2 buffer sizes, be platform specific. Olga is > right to point out the code in the high latency calculations, all calculated > values from the latency hint should follow that model. > > I.e., multiples of 10ms on Windows, powers of 2 on other platforms (ideally > multiples of the minimum supported buffer sizes). Doing otherwise will result in > back-to-back buffering callbacks during which the client may be unprepared to > respond. This is also, mostly, my understanding. However, I was kind of expecting from WebAudio's view point that the buffer sizes would be multiples of 128, not just powers of two, with some kind of lower and upper limit TBD. In addition, on Android devices that don't support low-latency audio, the buffer sizes are already not necessarily a power of two. We see sizes like 2112 (?), 2205 and lots of other unexpected sizes. Since the low-latency path is not available, we just use these values as is. (At least that's what we used to do. Not exactly sure what happens today.)
On 2017/03/22 at 21:49:07, rtoy wrote: > On 2017/03/22 17:57:49, DaleCurtis wrote: > > When we originally discussed this proposal it was my understanding that the > > latency hint is just that, a hint. It's exact shape will by necessity, fixed > > Windows 10ms callbacks, power of 2 buffer sizes, be platform specific. Olga is > > right to point out the code in the high latency calculations, all calculated > > values from the latency hint should follow that model. > > > > I.e., multiples of 10ms on Windows, powers of 2 on other platforms (ideally > > multiples of the minimum supported buffer sizes). Doing otherwise will result in > > back-to-back buffering callbacks during which the client may be unprepared to > > respond. > > This is also, mostly, my understanding. However, I was kind of expecting from WebAudio's view point that the buffer sizes would be multiples of 128, not just powers of two, with some kind of lower and upper limit TBD. > > In addition, on Android devices that don't support low-latency audio, the buffer sizes are already not necessarily a power of two. We see sizes like 2112 (?), 2205 and lots of other unexpected sizes. Since the low-latency path is not available, we just use these values as is. (At least that's what we used to do. Not exactly sure what happens today.) Using an "exact" buffer size that's a power of 2, or multiple of 128, or of the hardware buffer size all sound fine to me. Looking at the other AudioLatency calculations, in GetRtcBufferSize the size on OSX and Linux is (sampleRate / 10) which gives 441 on my OSX dev machine, and a comment suggests that these platforms can support any buffer size. For GetHighLatencyBufferSize it looks like it's a multiple of the hardware buffer size on Windows and a power of 2 on other platforms. From the AudioLatency implementation right now it looks like using multiples of the hardware buffer size would work for Windows (currently using this for high latency) and OSX/Linux (comment says any buffer size supported), as well as Android. ChromeOS doesn't appear to be using multiples of the hw size right now (though it may be supported) but seems to use either the hw buffer size (Interactive and Rtc) or else a power of 2 (HighLatency). Would making it a rounded-up multiple of the hardware buffer size in all cases work? I'll implement whatever you think is best. Thanks!
The nearest fixed multiple of the hardware buffer size should be sufficient in all cases to prevent back-to-back reads hitting the shared memory.
On 2017/03/27 at 18:52:30, dalecurtis wrote: > The nearest fixed multiple of the hardware buffer size should be sufficient in all cases to prevent back-to-back reads hitting the shared memory. Ok great, I've pushed a change now that rounds the requested buffer size up to the next multiple of the hardware buffer size. Thanks!
https://codereview.chromium.org/2750543003/diff/80001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:64: return media::AudioLatency::GetHighLatencyBufferSize( I wonder if we should just replace this with GetExactBufferSize(0.2s, params). We can do that in a followup though. https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... media/base/audio_latency.cc:130: int AudioLatency::GetExactBufferSize(double duration_in_seconds, This should take TimeDelta since it's a chromium class. https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... media/base/audio_latency.cc:140: return std::max(std::min(buffer_size, GetHighLatencyBufferSize( Hmm, I don't think these methods should call each other. This should just be the contents of the if WIN block in GetHighLatencyBufferSize today.
Thanks dalecurtis@! https://codereview.chromium.org/2750543003/diff/80001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:64: return media::AudioLatency::GetHighLatencyBufferSize( On 2017/03/28 at 17:34:07, DaleCurtis wrote: > I wonder if we should just replace this with GetExactBufferSize(0.2s, params). We can do that in a followup though. Sounds good! https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... media/base/audio_latency.cc:130: int AudioLatency::GetExactBufferSize(double duration_in_seconds, On 2017/03/28 at 17:34:07, DaleCurtis wrote: > This should take TimeDelta since it's a chromium class. Done. https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... media/base/audio_latency.cc:140: return std::max(std::min(buffer_size, GetHighLatencyBufferSize( On 2017/03/28 at 17:34:07, DaleCurtis wrote: > Hmm, I don't think these methods should call each other. This should just be the contents of the if WIN block in GetHighLatencyBufferSize today. Ok, I've modified this as you suggest to just use a 20ms buffer rounded up to the nearest multiple of the hardware buffer size. Note that this means that we're clamping the "exact" buffer size maximum to a value lower than the current "playback" buffer size for some OSes now, since non-Windows OSes were rounding up the 20ms size to the next power of 2 and not the next multiple of the hw size before. This is fine with me but just pointing it out.
https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... media/base/audio_latency.cc:139: hardware_buffer_size; It's actually not the nearest multiply (round), but nearest which does not exceed the requested size. Is it what we want? https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... media/base/audio_latency.cc:143: std::ceil(twenty_ms_size / hardware_buffer_size) * hardware_buffer_size; In fact, we are just limiting |duration| to 20 ms, right? 1) Is it really a limit we want to set for the users? 2) If so, we can just use min (duration, 0.02) https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... media/base/audio_latency.cc:146: } Dale - Is our intention to use multiplies of HW buffer size on all the platforms, no more "power of 2" in GetHighLatencyBufferSize()?
On 2017/03/29 08:34:10, Andrew MacPherson wrote: > Thanks dalecurtis@! > > https://codereview.chromium.org/2750543003/diff/80001/content/renderer/media/... > File content/renderer/media/renderer_webaudiodevice_impl.cc (right): > > https://codereview.chromium.org/2750543003/diff/80001/content/renderer/media/... > content/renderer/media/renderer_webaudiodevice_impl.cc:64: return > media::AudioLatency::GetHighLatencyBufferSize( > On 2017/03/28 at 17:34:07, DaleCurtis wrote: > > I wonder if we should just replace this with GetExactBufferSize(0.2s, params). > We can do that in a followup though. > > Sounds good! > > https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... > File media/base/audio_latency.cc (right): > > https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... > media/base/audio_latency.cc:130: int AudioLatency::GetExactBufferSize(double > duration_in_seconds, > On 2017/03/28 at 17:34:07, DaleCurtis wrote: > > This should take TimeDelta since it's a chromium class. > > Done. > > https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc... > media/base/audio_latency.cc:140: return std::max(std::min(buffer_size, > GetHighLatencyBufferSize( > On 2017/03/28 at 17:34:07, DaleCurtis wrote: > > Hmm, I don't think these methods should call each other. This should just be > the contents of the if WIN block in GetHighLatencyBufferSize today. > > Ok, I've modified this as you suggest to just use a 20ms buffer rounded up to > the nearest multiple of the hardware buffer size. Note that this means that > we're clamping the "exact" buffer size maximum to a value lower than the current > "playback" buffer size for some OSes now, since non-Windows OSes were rounding > up the 20ms size to the next power of 2 and not the next multiple of the hw size > before. This is fine with me but just pointing it out. Question. Say I do c = new AudioContext() and examine c.baseLatency. I get some value and do c1 = new AudioContext({latencyHint: c.baseLatency}). (Or is that c.baseLatency/2?) Then the baseLatency for c1 could be different from the latency for c? If so, that seems very odd that I can't create an audio context with an explicit latency hint and produce the original. The spec doesn't require this, but it certainly would break my expectations on what a latencyHint value would do in these cases. Similar questions hold if I c is created with a latencyHint of "playback" and "balanced". I would expect I could create c1 with a latency value that would produce the same latency as c.
https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... media/base/audio_latency.cc:143: std::ceil(twenty_ms_size / hardware_buffer_size) * hardware_buffer_size; On 2017/03/29 at 10:56:05, o1ka wrote: > In fact, we are just limiting |duration| to 20 ms, right? > 1) Is it really a limit we want to set for the users? > 2) If so, we can just use min (duration, 0.02) Hmm, this wasn't what I was proposing. I was proposing only doing what l.136 does. This code should just always return the nearest multiple (higher or lower) to requested_buffer_size. https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... media/base/audio_latency.cc:146: } On 2017/03/29 at 10:56:05, o1ka wrote: > Dale - Is our intention to use multiplies of HW buffer size on all the platforms, no more "power of 2" in GetHighLatencyBufferSize()? Yes, I think that should be okay and likely will result in the same result since platforms which don't care already specify a power of 2 buffer size. https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... media/base/audio_latency.cc:146: } On 2017/03/29 at 10:56:05, o1ka wrote: > Dale - Is our intention to use multiplies of HW buffer size on all the platforms, no more "power of 2" in GetHighLatencyBufferSize()? Yes, I think that should be okay and likely will result in the same result since platforms which don't care already specify a power of 2 buffer size.
Thanks rtoy@, olka@ and dalecurtis@!
On 2017/03/29 at 16:18:29, rtoy wrote:
> Question. Say I do c = new AudioContext() and examine c.baseLatency. I get
some value and do c1 = new AudioContext({latencyHint: c.baseLatency}). (Or is
that c.baseLatency/2?) Then the baseLatency for c1 could be different from the
latency for c? If so, that seems very odd that I can't create an audio context
with an explicit latency hint and produce the original.
>
> The spec doesn't require this, but it certainly would break my expectations on
what a latencyHint value would do in these cases.
>
> Similar questions hold if I c is created with a latencyHint of "playback" and
"balanced". I would expect I could create c1 with a latency value that would
produce the same latency as c.
Right now for 'interactive', creating c1 from c.baseLatency/2 will return a
context where c and c1 latencies are the same. After the most recent commit to
remove the "power of 2" calculation the same thing applies for 'playback'.
However 'balanced' (at least on OSX) is returning a buffer size of
sampleRate/10, so 441 and creating a context with this explicit value will
return a context with the latency set to 512.
https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten...
File media/base/audio_latency.cc (right):
https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten...
media/base/audio_latency.cc:139: hardware_buffer_size;
On 2017/03/29 at 10:56:05, o1ka wrote:
> It's actually not the nearest multiply (round), but nearest which does not
exceed the requested size. Is it what we want?
This was my intent but since both you and dalecurtis@ have mentioned "nearest
multiple" I've updated it to use a round() now, thanks!
https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten...
media/base/audio_latency.cc:143: std::ceil(twenty_ms_size /
hardware_buffer_size) * hardware_buffer_size;
On 2017/03/29 at 22:42:41, DaleCurtis wrote:
> On 2017/03/29 at 10:56:05, o1ka wrote:
> > In fact, we are just limiting |duration| to 20 ms, right?
> > 1) Is it really a limit we want to set for the users?
> > 2) If so, we can just use min (duration, 0.02)
>
> Hmm, this wasn't what I was proposing. I was proposing only doing what l.136
does. This code should just always return the nearest multiple (higher or lower)
to requested_buffer_size.
Sorry, my mistake here. I can remove this but I believe we'll still need to cap
the buffer size to some maximum. In Web Audio's AudioDestination.cpp there's a
FIFO used that can only hold up to 8064 frames and will crash otherwise. Also in
(for example) audio_manager_mac.cc there is kMaximumInputOutputBufferSize =
4096, I guess that higher values are just ignored and the maximum is used but
this would mean that the AudioContext.baseLatency would be incorrect if this
value is too high since the system will actually be using a lower value than the
one requested. Thoughts?
https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten...
media/base/audio_latency.cc:146: }
On 2017/03/29 at 22:42:41, DaleCurtis wrote:
> On 2017/03/29 at 10:56:05, o1ka wrote:
> > Dale - Is our intention to use multiplies of HW buffer size on all the
platforms, no more "power of 2" in GetHighLatencyBufferSize()?
>
> Yes, I think that should be okay and likely will result in the same result
since platforms which don't care already specify a power of 2 buffer size.
Ok, I've updated GetHighLatencyBufferSize() to always round up to a multiple of
the HW buffer size, this also required updating
audio_renderer_mixer_manager_unittest.cc as it was expecting the "power of 2"
calculation. Let me know if I've misunderstood what was intended though.
https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/100001/media/base/audio_laten... media/base/audio_latency.cc:143: std::ceil(twenty_ms_size / hardware_buffer_size) * hardware_buffer_size; On 2017/03/30 at 08:07:18, Andrew MacPherson wrote: > On 2017/03/29 at 22:42:41, DaleCurtis wrote: > > On 2017/03/29 at 10:56:05, o1ka wrote: > > > In fact, we are just limiting |duration| to 20 ms, right? > > > 1) Is it really a limit we want to set for the users? > > > 2) If so, we can just use min (duration, 0.02) > > > > Hmm, this wasn't what I was proposing. I was proposing only doing what l.136 does. This code should just always return the nearest multiple (higher or lower) to requested_buffer_size. > > Sorry, my mistake here. I can remove this but I believe we'll still need to cap the buffer size to some maximum. In Web Audio's AudioDestination.cpp there's a FIFO used that can only hold up to 8064 frames and will crash otherwise. Also in (for example) audio_manager_mac.cc there is kMaximumInputOutputBufferSize = 4096, I guess that higher values are just ignored and the maximum is used but this would mean that the AudioContext.baseLatency would be incorrect if this value is too high since the system will actually be using a lower value than the one requested. Thoughts? This should be removed from this code. We don't want to bake WebAudio expectations into this piece. Instead WebAudio can clamp this as necessary in it's own code. Oversized buffers will be clamped during output, but you'll need to test what happens when that occurs. There should be some sanity check on the values passed in from WebAudio, i.e. perhaps the timedelta is clamped to < .5seconds or something. Or even .2 if you want to max at high latency buffer size, but that's a WebAudio decision and not something we should clamp in this function. https://codereview.chromium.org/2750543003/diff/120001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/120001/media/base/audio_laten... media/base/audio_latency.cc:18: int AudioLatency::GetHighLatencyBufferSize(int sample_rate, I wasn't proposing you make this change in this CL; since it's something we may need to revert. You're welcome to include it here, but I think it may be a bit risky. https://codereview.chromium.org/2750543003/diff/120001/media/base/audio_laten... media/base/audio_latency.cc:29: #if defined(OS_CHROMEOS) Is this necessary now? We'll always be at least 1 multiple of the preferred buffer size right? https://codereview.chromium.org/2750543003/diff/120001/media/base/audio_laten... media/base/audio_latency.cc:96: const AudioParameters& hardware_params) { Should probably just take int sample_rate, int frames_per_buffer like the other methods.
On 2017/03/30 at 18:53:47, dalecurtis wrote: > > Sorry, my mistake here. I can remove this but I believe we'll still need to cap the buffer size to some maximum. In Web Audio's AudioDestination.cpp there's a FIFO used that can only hold up to 8064 frames and will crash otherwise. Also in (for example) audio_manager_mac.cc there is kMaximumInputOutputBufferSize = 4096, I guess that higher values are just ignored and the maximum is used but this would mean that the AudioContext.baseLatency would be incorrect if this value is too high since the system will actually be using a lower value than the one requested. Thoughts? > > This should be removed from this code. We don't want to bake WebAudio expectations into this piece. Instead WebAudio can clamp this as necessary in it's own code. > > Oversized buffers will be clamped during output, but you'll need to test what happens when that occurs. There should be some sanity check on the values passed in from WebAudio, i.e. perhaps the timedelta is clamped to < .5seconds or something. Or even .2 if you want to max at high latency buffer size, but that's a WebAudio decision and not something we should clamp in this function. Clamping the buffer size either here or in Web Audio is already a bit of a workaround since I think what we really want is to be able to query the audio system for the buffer size that it's really using. The purpose of this clamping is to try to guess at a "known good" value that won't be further clamped by the lower levels, so that this buffer size can then be made available to users via the AudioContext API. If the RendererWebAudioDeviceImpl's framesPerBuffer() method could be made to return the real buffer size chosen by the system then I think that would be the ideal fix. We can move this upper-bound clamping into the AudioDestination.cpp though that kind of feels like even more of a workaround since in general all decisions about the buffer size to use are being made in the content/renderer/ side, definitely no problem to do so though (clamp to 20ms max for the "exact" latencyHint case only). rtoy@ do you have any thoughts here? Thanks! https://codereview.chromium.org/2750543003/diff/120001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/120001/media/base/audio_laten... media/base/audio_latency.cc:18: int AudioLatency::GetHighLatencyBufferSize(int sample_rate, On 2017/03/30 at 18:53:47, DaleCurtis wrote: > I wasn't proposing you make this change in this CL; since it's something we may need to revert. You're welcome to include it here, but I think it may be a bit risky. Sorry, my mistake, I wasn't sure if you were suggesting doing this now or not. I've reverted this for now to keep this CL as straightforward as possible, thanks! https://codereview.chromium.org/2750543003/diff/120001/media/base/audio_laten... media/base/audio_latency.cc:96: const AudioParameters& hardware_params) { On 2017/03/30 at 18:53:47, DaleCurtis wrote: > Should probably just take int sample_rate, int frames_per_buffer like the other methods. Done.
On 2017/03/31 at 12:31:01, andrew.macpherson wrote: > Clamping the buffer size either here or in Web Audio is already a bit of a workaround since I think what we really want is to be able to query the audio system for the buffer size that it's really using. The purpose of this clamping is to try to guess at a "known good" value that won't be further clamped by the lower levels, so that this buffer size can then be made available to users via the AudioContext API. If the RendererWebAudioDeviceImpl's framesPerBuffer() method could be made to return the real buffer size chosen by the system then I think that would be the ideal fix. I don't follow your logic here. It seems fine to return via some new parameter, but really my argument is that AudioLatency is the wrong place to clamp unless you plan to plumb the platform specific limits. If you instead have some arbitrary limit from WebAudio, you should instead clamp in RendererWebAudioDeviceImpl.
On 2017/03/31 at 19:46:39, dalecurtis wrote: > On 2017/03/31 at 12:31:01, andrew.macpherson wrote: > > > Clamping the buffer size either here or in Web Audio is already a bit of a workaround since I think what we really want is to be able to query the audio system for the buffer size that it's really using. The purpose of this clamping is to try to guess at a "known good" value that won't be further clamped by the lower levels, so that this buffer size can then be made available to users via the AudioContext API. If the RendererWebAudioDeviceImpl's framesPerBuffer() method could be made to return the real buffer size chosen by the system then I think that would be the ideal fix. > > I don't follow your logic here. It seems fine to return via some new parameter, but really my argument is that AudioLatency is the wrong place to clamp unless you plan to plumb the platform specific limits. If you instead have some arbitrary limit from WebAudio, you should instead clamp in RendererWebAudioDeviceImpl. Ideally I think we would like to plumb the platform-specific limits as you mention. I'll take one more stab at explaining what I mean then will implement whatever you tell me to. :) I think I confused the issue by mentioning the FIFO size in Web Audio, which I believe could simply be made bigger if necessary and wasn't strictly related to my point. If user passes in a latencyHint resulting in a buffer size of 5120 right now on OSX, Web Audio can handle this and GetExactBufferSize() thinks this is fine as it's a multiple of the HW buffer size 256. The RendereWebAudioDeviceImpl will then store this as its framesPerBuffer() size and so when Web Audio asks the RAWDI what the chosen audio buffer size was it will say 5120. However audio_manager_mac is actually going to clamp this to 4096 (its maximum), so RWADI is not returning the real buffer size being used to Web Audio. I have no strong feelings about this as at Soundtrap we're not concerned with high buffer sizes (only much lower ones) but the clamping I was doing in AudioLatency was only an attempt to clamp well below the "maximum buffer size" that could be encountered on any OS, since they doesn't appear to be any way to query the audio manager from the RWADI to know what the current audio buffer size is. This is of course a kludge but I'm not sure what the right thing to do is, since we would really like AudioContext.baseLatency to be able to tell the user the device's actual buffer size. If this discrepancy is okay though then we can just clamp to some high limit in Web Audio and not care that baseLatency doesn't reflect the device's real buffer size for values above the OS max.
On 2017/04/03 at 09:49:00, andrew.macpherson wrote: > On 2017/03/31 at 19:46:39, dalecurtis wrote: > > On 2017/03/31 at 12:31:01, andrew.macpherson wrote: > > > > > Clamping the buffer size either here or in Web Audio is already a bit of a workaround since I think what we really want is to be able to query the audio system for the buffer size that it's really using. The purpose of this clamping is to try to guess at a "known good" value that won't be further clamped by the lower levels, so that this buffer size can then be made available to users via the AudioContext API. If the RendererWebAudioDeviceImpl's framesPerBuffer() method could be made to return the real buffer size chosen by the system then I think that would be the ideal fix. > > > > I don't follow your logic here. It seems fine to return via some new parameter, but really my argument is that AudioLatency is the wrong place to clamp unless you plan to plumb the platform specific limits. If you instead have some arbitrary limit from WebAudio, you should instead clamp in RendererWebAudioDeviceImpl. > > Ideally I think we would like to plumb the platform-specific limits as you mention. I'll take one more stab at explaining what I mean then will implement whatever you tell me to. :) I think I confused the issue by mentioning the FIFO size in Web Audio, which I believe could simply be made bigger if necessary and wasn't strictly related to my point. > > If user passes in a latencyHint resulting in a buffer size of 5120 right now on OSX, Web Audio can handle this and GetExactBufferSize() thinks this is fine as it's a multiple of the HW buffer size 256. The RendereWebAudioDeviceImpl will then store this as its framesPerBuffer() size and so when Web Audio asks the RAWDI what the chosen audio buffer size was it will say 5120. However audio_manager_mac is actually going to clamp this to 4096 (its maximum), so RWADI is not returning the real buffer size being used to Web Audio. > > I have no strong feelings about this as at Soundtrap we're not concerned with high buffer sizes (only much lower ones) but the clamping I was doing in AudioLatency was only an attempt to clamp well below the "maximum buffer size" that could be encountered on any OS, since they doesn't appear to be any way to query the audio manager from the RWADI to know what the current audio buffer size is. This is of course a kludge but I'm not sure what the right thing to do is, since we would really like AudioContext.baseLatency to be able to tell the user the device's actual buffer size. If this discrepancy is okay though then we can just clamp to some high limit in Web Audio and not care that baseLatency doesn't reflect the device's real buffer size for values above the OS max. I must be missing something, sorry :) It sounds like baseLatency should always be the hardware buffer size as provided from the browser process via GetOutputDeviceInfo; that value is unchanged by anything user-provided if I understand correctly. framesPerBuffer() instead should reflect the request from the user and any clamping or rounding that needs to occur. To this end I think we want to specify maximum audio buffer sizes either in the AudioManager, or via media/base/limits.h and then change the AudioManager to use the values in media/base/limits.h
On 2017/04/03 at 22:33:23, dalecurtis wrote: > I must be missing something, sorry :) It sounds like baseLatency should always be the hardware buffer size as provided from the browser process via GetOutputDeviceInfo; that value is unchanged by anything user-provided if I understand correctly. framesPerBuffer() instead should reflect the request from the user and any clamping or rounding that needs to occur. To this end I think we want to specify maximum audio buffer sizes either in the AudioManager, or via media/base/limits.h and then change the AudioManager to use the values in media/base/limits.h Making the per-OS maximum sizes available somewhere (either audio_manager.h or media/base/limits.h) would solve the problem as these could be then used in the RendererWebAudioDeviceImpl to clamp the "exact" buffer size. Is that too big a change for this CL though (moving these values to a common location)? If so would it be okay to just clamp to 4096 in RWADI for now (I think the current smallest of these maximums) with a note that the real maximums will be used in an upcoming CL? Thanks again for all the help here! :)
On 2017/04/04 at 12:18:25, andrew.macpherson wrote: > On 2017/04/03 at 22:33:23, dalecurtis wrote: > > I must be missing something, sorry :) It sounds like baseLatency should always be the hardware buffer size as provided from the browser process via GetOutputDeviceInfo; that value is unchanged by anything user-provided if I understand correctly. framesPerBuffer() instead should reflect the request from the user and any clamping or rounding that needs to occur. To this end I think we want to specify maximum audio buffer sizes either in the AudioManager, or via media/base/limits.h and then change the AudioManager to use the values in media/base/limits.h > > Making the per-OS maximum sizes available somewhere (either audio_manager.h or media/base/limits.h) would solve the problem as these could be then used in the RendererWebAudioDeviceImpl to clamp the "exact" buffer size. Is that too big a change for this CL though (moving these values to a common location)? If so would it be okay to just clamp to 4096 in RWADI for now (I think the current smallest of these maximums) with a note that the real maximums will be used in an upcoming CL? Thanks again for all the help here! :) 4096 + todo sgtm for now.
On 2017/04/04 at 16:56:47, dalecurtis wrote: > On 2017/04/04 at 12:18:25, andrew.macpherson wrote: > > Making the per-OS maximum sizes available somewhere (either audio_manager.h or media/base/limits.h) would solve the problem as these could be then used in the RendererWebAudioDeviceImpl to clamp the "exact" buffer size. Is that too big a change for this CL though (moving these values to a common location)? If so would it be okay to just clamp to 4096 in RWADI for now (I think the current smallest of these maximums) with a note that the real maximums will be used in an upcoming CL? Thanks again for all the help here! :) > > 4096 + todo sgtm for now. Ok great! I've removed the max value clamping from AudioLatency and moved it into the WebAudioDeviceImpl and am using 4096 as the max now with a comment that this should be updated to be the OS-specific max values. Thanks again!
dalecurtis@chromium.org changed required reviewers: + olka@chromium.org, rtoy@chromium.org
audio latency stuff lgtm, defer to rtoy and olka for rest. https://codereview.chromium.org/2750543003/diff/160001/content/renderer/media... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/160001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:68: // TODO: replace 4096 with the OS-specific maximum buffer size limits. TODO(<your user id>): http://crbug.com/xxxx tracking this.
Thanks dalecurtis@! https://codereview.chromium.org/2750543003/diff/160001/content/renderer/media... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/160001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:68: // TODO: replace 4096 with the OS-specific maximum buffer size limits. On 2017/04/05 at 23:12:07, DaleCurtis wrote: > TODO(<your user id>): http://crbug.com/xxxx tracking this. Done.
lgtm % nits. Thanks Andrew! (Except third_party/WebKit/* I'm unfamiliar with) https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:51: const media::AudioLatency::LatencyType latency, const not needed? https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:137: media::AudioParameters hardware_params(device_params_cb.Run( const? https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:140: media::AudioLatency::LatencyType latency = const? https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:143: int output_buffer_size = const? https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:145: DCHECK_NE(output_buffer_size, 0); I think usually it's DCHECK_NE(0, output_buffer_size) (first a target value, then a verified one) https://codereview.chromium.org/2750543003/diff/180001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/180001/media/base/audio_laten... media/base/audio_latency.cc:137: std::round(std::max(requested_buffer_size, 1.0) / hardware_buffer_size) * DCHECK_NE(0, hardware_buffer_size) at l.134?
Almost there; just a few questions on the tests. https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right): https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:33: context.close(); Are we running out of online contexts without this? close() returns a promise, so we might want to modify this so that the following bits happen after the promise resolves. Or move the following bits into its own audit task which are run sequentially. Same thing below wherever you call context.close(). https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:59: var validLatency = (interactiveLatency + playbackLatency) / 4; Why divide by 4? Don't you want to divide by 2 so you're definitely between interactive and playback (in general)? https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:63: Should("double-constructor baseLatency inrange 1", context.baseLatency).beGreaterThanOrEqualTo(defaultLatency); defaultLatency or interactiveLatency?
Thanks olka@ and rtoy@! https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:51: const media::AudioLatency::LatencyType latency, On 2017/04/06 at 16:02:30, o1ka wrote: > const not needed? Done. https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:137: media::AudioParameters hardware_params(device_params_cb.Run( On 2017/04/06 at 16:02:30, o1ka wrote: > const? Done. https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:140: media::AudioLatency::LatencyType latency = On 2017/04/06 at 16:02:30, o1ka wrote: > const? Done. https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:143: int output_buffer_size = On 2017/04/06 at 16:02:30, o1ka wrote: > const? Done. https://codereview.chromium.org/2750543003/diff/180001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:145: DCHECK_NE(output_buffer_size, 0); On 2017/04/06 at 16:02:30, o1ka wrote: > I think usually it's DCHECK_NE(0, output_buffer_size) > (first a target value, then a verified one) Done. https://codereview.chromium.org/2750543003/diff/180001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2750543003/diff/180001/media/base/audio_laten... media/base/audio_latency.cc:137: std::round(std::max(requested_buffer_size, 1.0) / hardware_buffer_size) * On 2017/04/06 at 16:02:30, o1ka wrote: > DCHECK_NE(0, hardware_buffer_size) at l.134? Done. https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right): https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:33: context.close(); On 2017/04/06 at 17:31:59, Raymond Toy wrote: > Are we running out of online contexts without this? > > close() returns a promise, so we might want to modify this so that the following bits happen after the promise resolves. Or move the following bits into its own audit task which are run sequentially. > > Same thing below wherever you call context.close(). Yes that's the reason for this. I've split the test into two audit tasks now with less than 6 context created/closed in each, does that work? https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:59: var validLatency = (interactiveLatency + playbackLatency) / 4; On 2017/04/06 at 17:32:00, Raymond Toy wrote: > Why divide by 4? Don't you want to divide by 2 so you're definitely between interactive and playback (in general)? Currently AudioContext.baseLatency is returning the latency*2 to account for double-buffering so I'm using 4 here, I agree that this is confusing though. I've added a comment around the use of 4 rather than 2. https://codereview.chromium.org/2750543003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:63: Should("double-constructor baseLatency inrange 1", context.baseLatency).beGreaterThanOrEqualTo(defaultLatency); On 2017/04/06 at 17:32:00, Raymond Toy wrote: > defaultLatency or interactiveLatency? Done.
The WebAudio parts look great. But I think I've forgotten what latencyHint = value actually means and the spec gives lots of leeway in this. My question is if latencyHint is in "units" that include the double buffering? So, if the HW buffer size is x and the sample rate is Fs, what value should I use for latencyHint to get a baseLatency value of 2*x/Fs? Is it x/Fs or 2*x/Fs? Whatever it is, can we also add a comment, perhaps in AudioContext.cpp on what the value actually means? Or perhaps also in AudioContextOptions.idl? Otherwise, it's really hard to tell without reading and understanding the tests.
On 2017/04/07 at 15:12:14, rtoy wrote: > The WebAudio parts look great. > > But I think I've forgotten what latencyHint = value actually means and the spec gives lots of leeway in this. > > My question is if latencyHint is in "units" that include the double buffering? So, if the HW buffer size is x and the sample rate is Fs, what value should I use for latencyHint to get a baseLatency value of 2*x/Fs? Is it x/Fs or 2*x/Fs? > > Whatever it is, can we also add a comment, perhaps in AudioContext.cpp on what the value actually means? Or perhaps also in AudioContextOptions.idl? Otherwise, it's really hard to tell without reading and understanding the tests. Right now the latencyHint "units" don't include double buffering but the baseLatency ones do, so you would use latencyHint=x/Fs to get a baseLatency value of 2*x/Fs. Let me know if this should be changed though, in the meantime I've added a comment to AudioContext.cpp and AudioContextOptions.idl. Thanks!
On 2017/04/09 09:59:50, Andrew MacPherson wrote: > On 2017/04/07 at 15:12:14, rtoy wrote: > > The WebAudio parts look great. > > > > But I think I've forgotten what latencyHint = value actually means and the > spec gives lots of leeway in this. > > > > My question is if latencyHint is in "units" that include the double buffering? > So, if the HW buffer size is x and the sample rate is Fs, what value should I > use for latencyHint to get a baseLatency value of 2*x/Fs? Is it x/Fs or 2*x/Fs? > > > > Whatever it is, can we also add a comment, perhaps in AudioContext.cpp on what > the value actually means? Or perhaps also in AudioContextOptions.idl? > Otherwise, it's really hard to tell without reading and understanding the tests. > > Right now the latencyHint "units" don't include double buffering but the > baseLatency ones do, so you would use latencyHint=x/Fs to get a baseLatency > value of 2*x/Fs. Let me know if this should be changed though, in the meantime > I've added a comment to AudioContext.cpp and AudioContextOptions.idl. Thanks! I certainly prefer x/Fs because I think in units for the HW buffer size, it's probably less confusing to the user that a chosen latencyHint will produce a same baseLatency value. hongchan, WDYT?
On 2017/04/10 15:21:19, Raymond Toy wrote: > On 2017/04/09 09:59:50, Andrew MacPherson wrote: > > On 2017/04/07 at 15:12:14, rtoy wrote: > > > The WebAudio parts look great. > > > > > > But I think I've forgotten what latencyHint = value actually means and the > > spec gives lots of leeway in this. > > > > > > My question is if latencyHint is in "units" that include the double > buffering? > > So, if the HW buffer size is x and the sample rate is Fs, what value should I > > use for latencyHint to get a baseLatency value of 2*x/Fs? Is it x/Fs or > 2*x/Fs? > > > > > > Whatever it is, can we also add a comment, perhaps in AudioContext.cpp on > what > > the value actually means? Or perhaps also in AudioContextOptions.idl? > > Otherwise, it's really hard to tell without reading and understanding the > tests. > > > > Right now the latencyHint "units" don't include double buffering but the > > baseLatency ones do, so you would use latencyHint=x/Fs to get a baseLatency > > value of 2*x/Fs. Let me know if this should be changed though, in the meantime > > I've added a comment to AudioContext.cpp and AudioContextOptions.idl. Thanks! > > I certainly prefer x/Fs because I think in units for the HW buffer size, it's > probably less confusing to the user that a chosen latencyHint will produce a > same baseLatency value. > > hongchan, WDYT? Here's what I think: 1) From user's perspective, it's confusing to see 2x when you set x. 2) We're using double buffering at the moment, but it might change in the future. Making it less confusing and more transparent is better at the end, I believe.
On 2017/04/10 at 16:40:05, hongchan wrote: > On 2017/04/10 15:21:19, Raymond Toy wrote: > > On 2017/04/09 09:59:50, Andrew MacPherson wrote: > > > Right now the latencyHint "units" don't include double buffering but the > > > baseLatency ones do, so you would use latencyHint=x/Fs to get a baseLatency > > > value of 2*x/Fs. Let me know if this should be changed though, in the meantime > > > I've added a comment to AudioContext.cpp and AudioContextOptions.idl. Thanks! > > > > I certainly prefer x/Fs because I think in units for the HW buffer size, it's > > probably less confusing to the user that a chosen latencyHint will produce a > > same baseLatency value. > > > > hongchan, WDYT? > > Here's what I think: > > 1) From user's perspective, it's confusing to see 2x when you set x. > 2) We're using double buffering at the moment, but it might change in the future. > > Making it less confusing and more transparent is better at the end, I believe. Thanks hongchan and rtoy, I've updated baseLatency to reflect the HW buffer size directly and not the size x2 now. Note that this is a change in baseLatency behavior WRT what is in Chrome 58 in case that's important.
On 2017/04/11 09:38:26, Andrew MacPherson wrote: > On 2017/04/10 at 16:40:05, hongchan wrote: > > On 2017/04/10 15:21:19, Raymond Toy wrote: > > > On 2017/04/09 09:59:50, Andrew MacPherson wrote: > > > > Right now the latencyHint "units" don't include double buffering but the > > > > baseLatency ones do, so you would use latencyHint=x/Fs to get a > baseLatency > > > > value of 2*x/Fs. Let me know if this should be changed though, in the > meantime > > > > I've added a comment to AudioContext.cpp and AudioContextOptions.idl. > Thanks! > > > > > > I certainly prefer x/Fs because I think in units for the HW buffer size, > it's > > > probably less confusing to the user that a chosen latencyHint will produce a > > > same baseLatency value. > > > > > > hongchan, WDYT? > > > > Here's what I think: > > > > 1) From user's perspective, it's confusing to see 2x when you set x. > > 2) We're using double buffering at the moment, but it might change in the > future. > > > > Making it less confusing and more transparent is better at the end, I believe. > > Thanks hongchan and rtoy, I've updated baseLatency to reflect the HW buffer size > directly and not the size x2 now. Note that this is a change in baseLatency > behavior WRT what is in Chrome 58 in case that's important. That makes baseLatency incorrect according to the spec. But on thinking more about this, I'm inclined to say the spec got it wrong. Double buffering only delays the output by the HW buffer size, not twice because we output a HW buffer of silence before outputting the real generated audio. So, lgtm for me. Hongchan, WDYT?
WebAudio lgtm with nits on layout test. Sorry for being absent in the review and thanks so much for your contribution, Andrew! https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right): https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:7: <script src="resources/audio-testing.js"></script> Can you consider to use |audit.js|? We are deprecating |audio-testing.js|. https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:57: // Task: test AudioContextOptions (2). Nit: please wrap by 80 column. I suggest to apply clang-format, unless you want to learn Google JS style guide. :) https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:61: context = new AudioContext({'latencyHint': interactiveLatency/2}); The try bots do not have an actual audio hardware. Not sure how this latency calculation will work out when you don't have the audio device? https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:96: done(); I am slightly concerned that you're creating more than 6 realtime audio contexts back to back. This might fail if the number of concurrent context goes beyond 6 for some reasons. If that happens, we should consider to split this test into two files. (to suppress the number of concurrent audio contexts).
On 2017/04/11 14:54:47, Raymond Toy wrote: > On 2017/04/11 09:38:26, Andrew MacPherson wrote: > > On 2017/04/10 at 16:40:05, hongchan wrote: > > > On 2017/04/10 15:21:19, Raymond Toy wrote: > > > > On 2017/04/09 09:59:50, Andrew MacPherson wrote: > > > > > Right now the latencyHint "units" don't include double buffering but the > > > > > baseLatency ones do, so you would use latencyHint=x/Fs to get a > > baseLatency > > > > > value of 2*x/Fs. Let me know if this should be changed though, in the > > meantime > > > > > I've added a comment to AudioContext.cpp and AudioContextOptions.idl. > > Thanks! > > > > > > > > I certainly prefer x/Fs because I think in units for the HW buffer size, > > it's > > > > probably less confusing to the user that a chosen latencyHint will produce > a > > > > same baseLatency value. > > > > > > > > hongchan, WDYT? > > > > > > Here's what I think: > > > > > > 1) From user's perspective, it's confusing to see 2x when you set x. > > > 2) We're using double buffering at the moment, but it might change in the > > future. > > > > > > Making it less confusing and more transparent is better at the end, I > believe. > > > > Thanks hongchan and rtoy, I've updated baseLatency to reflect the HW buffer > size > > directly and not the size x2 now. Note that this is a change in baseLatency > > behavior WRT what is in Chrome 58 in case that's important. > > That makes baseLatency incorrect according to the spec. But on thinking more > about this, I'm inclined to say the spec got it wrong. Double buffering only > delays the output by the HW buffer size, not twice because we output a HW buffer > of silence before outputting the real generated audio. > > So, lgtm for me. > > Hongchan, WDYT? Sounds good to me.
Great, thanks hongchan! https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right): https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:7: <script src="resources/audio-testing.js"></script> On 2017/04/11 at 17:27:37, hongchan wrote: > Can you consider to use |audit.js|? We are deprecating |audio-testing.js|. Done, I've updated the tests to use the new audit.js. https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:57: // Task: test AudioContextOptions (2). On 2017/04/11 at 17:27:37, hongchan wrote: > Nit: please wrap by 80 column. I suggest to apply clang-format, unless you want to learn Google JS style guide. :) Neat, I didn't realize that clang-format could be used with JS code. :) I ran it on the JS portion of the file after updating to the new audit.js framework, it actually broke the code in one place (splitting a long quoted string across two lines) but after fixing that by hand the rest was great. https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:61: context = new AudioContext({'latencyHint': interactiveLatency/2}); On 2017/04/11 at 17:27:37, hongchan wrote: > The try bots do not have an actual audio hardware. Not sure how this latency calculation will work out when you don't have the audio device? I assumed that the layout_test_content_renderer_client.cc would be used as a mock here but I think I'm not totally clear on what happens on the trybots. As long as the trybots are able to create an AudioContext though I believe this should work correctly. https://codereview.chromium.org/2750543003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:96: done(); On 2017/04/11 at 17:27:37, hongchan wrote: > I am slightly concerned that you're creating more than 6 realtime audio contexts back to back. This might fail if the number of concurrent context goes beyond 6 for some reasons. > > If that happens, we should consider to split this test into two files. (to suppress the number of concurrent audio contexts). I've added a Promise.all() at the end of each test to wait for all the context.close() promises to return, does that work better?
The CQ bit was checked by andrew.macpherson@soundtrap.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. The layout test looks great. Thanks, Andrew!
The CQ bit was checked by andrew.macpherson@soundtrap.com
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, olka@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2750543003/#ps280001 (title: "Refactor audiocontextoptions LayoutTest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
andrew.macpherson@soundtrap.com changed reviewers: + dgozman@chromium.org
Hi dgozman@, would you be able to take a quick look at these files from the CL, which still need an OWNER's l-g-t-m, "git cl owners" suggested you as a good candidate: content/public/renderer/content_renderer_client.cc content/public/renderer/content_renderer_client.h content/renderer/renderer_blink_platform_impl.cc content/shell/renderer/layout_test/layout_test_content_renderer_client.cc content/shell/renderer/layout_test/layout_test_content_renderer_client.h Thanks a lot for any help!
content lgtm
The CQ bit was checked by andrew.macpherson@soundtrap.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1492451716558080,
"parent_rev": "ffe776a3943ecf175a54317cbebbdd30ef1d7f37", "commit_rev":
"67c41de111691e8cfb173c6cb1f31ee1d24b5dfe"}
Message was sent while issue was closed.
Description was changed from ========== Support AudioContextOptions latencyHint as double. Follow-up to https://codereview.chromium.org/2501863003 This adds support for the latencyHint value to be passed as a double in order to request an exact latency amount. BUG=564276 R=rtoy@chromium.org ========== to ========== Support AudioContextOptions latencyHint as double. Follow-up to https://codereview.chromium.org/2501863003 This adds support for the latencyHint value to be passed as a double in order to request an exact latency amount. BUG=564276 R=rtoy@chromium.org Review-Url: https://codereview.chromium.org/2750543003 Cr-Commit-Position: refs/heads/master@{#464988} Committed: https://chromium.googlesource.com/chromium/src/+/67c41de111691e8cfb173c6cb1f3... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/67c41de111691e8cfb173c6cb1f3... |
