Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2908073002:
Make OS audio buffer size limits visible. (Closed)
|
Created:
3 years, 6 months ago by Andrew MacPherson Modified:
3 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, feature-media-reviews_chromium.org, jam, jochen+watch_chromium.org, mac-reviews_chromium.org, 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. |
DescriptionMake OS audio buffer size limits visible.
Make platform-specific audio buffer size limits visible in limits.h and update AudioLatency::GetExactBufferSize() to allow requesting buffer sizes down to the minimum. Update OSX and CRAS to allow audio buffer sizes below their previous minimums when using GetExactBufferSize(), for use in Web Audio AudioContext creation with a latencyHint.
BUG=708917
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2908073002
Cr-Original-Commit-Position: refs/heads/master@{#484231}
Committed: https://chromium.googlesource.com/chromium/src/+/3fe2409358437193a07496c2d0a0b559ef399760
Review-Url: https://codereview.chromium.org/2908073002
Cr-Commit-Position: refs/heads/master@{#484895}
Committed: https://chromium.googlesource.com/chromium/src/+/0a4bc5d41049c519193b922b67777f4efb3870fd
Patch Set 1 #
Total comments: 1
Patch Set 2 : Move all buffer size calculations to AudioLatency. #
Total comments: 2
Patch Set 3 : Different approach as suggested by olka. #
Total comments: 8
Patch Set 4 : Updates based on reviewer feedback. #
Total comments: 10
Patch Set 5 : Add unit test for minimum audio buffer sizes. #
Total comments: 16
Patch Set 6 : Updates based on reviewer feedback. #
Total comments: 15
Patch Set 7 : More fixes based on reviewer comments. #Patch Set 8 : Rename audio_util_mac to audio_latency_mac. #Patch Set 9 : Fix CRAS calculation and audio_manager_unittest. #
Total comments: 8
Patch Set 10 : Updates to unit test from reviewer comments. #Patch Set 11 : Fixes for trybot failures. #Patch Set 12 : More trybot fixes. #Patch Set 13 : Refactor and simplify audio_manager_unittest. #Patch Set 14 : Remove pulse-related changes. #Messages
Total messages: 106 (44 generated)
Description was changed from ========== Make OS audio buffer size limits visible. Add static methods GetMinimumAudioBufferSize and GetMaximumAudioBufferSize to the AudioManager for use in latencyHint-based buffer size calculations. BUG=708917 ========== to ========== Make OS audio buffer size limits visible. Add static methods GetMinimumAudioBufferSize and GetMaximumAudioBufferSize to the AudioManager for use in latencyHint-based buffer size calculations. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
andrew.macpherson@soundtrap.com changed reviewers: + dalecurtis@chromium.org, hongchan@chromium.org, olka@chromium.org, rtoy@chromium.org
Hi dalecurtis@, olka@, rtoy@ and hongchan@, This is a proposed fix for the issue discussed in https://codereview.chromium.org/2750543003/ and filed as https://crbug.com/708917 around providing access to platform-specific minimum and maximum audio buffer sizes when choosing a latencyHint-dependent buffer size. The only behaviour change in this CL would be that there are now platform-specific maximum buffer sizes instead of the earlier fixed value of 4096. A subsequent CL would then update the GetMinimumAudioBufferSize() first on OSX to return 128 instead of 256, and later (if possible) on ChromeOS to return at least 256 but ideally 128 there too, so that these values can be requested explicitly when creating an AudioContext. The implementations are filled in for OSX and CRAS only, the other implementations can be filled in if it's agreed that this is the right way to go. As we (at Soundtrap) are only interested for now in OSX and ChromeOS the minimums for other platforms would just remain equal to their current defaults. Let me know if this looks reasonable or if we should look into other alternatives and thanks a lot for any help!
https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/rend... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/rend... content/renderer/media/renderer_webaudiodevice_impl.cc:69: return std::min(media::AudioManager::GetMaximumAudioBufferSize( AudioManager is browser side only entity and cannot be used in the renderer. Why cannot the code live in AudioLatency?
not lgtm
On 2017/05/29 at 16:11:33, olka wrote: > https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/rend... > File content/renderer/media/renderer_webaudiodevice_impl.cc (right): > > https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/rend... > content/renderer/media/renderer_webaudiodevice_impl.cc:69: return std::min(media::AudioManager::GetMaximumAudioBufferSize( > AudioManager is browser side only entity and cannot be used in the renderer. > Why cannot the code live in AudioLatency? Thanks olka@! The main reason I kept everything in AudioManager was to minimize the amount of code changed but I didn't realize that this couldn't be used from the renderer. I've tried moving all buffer size calculation code into AudioLatency for OSX and CRAS (the other platforms would need to be moved there too). Does it make more sense like this? I'm not clear on what the best solution is here so I'm very open to any suggestions if you see a better way to approach the problem. Thanks again!
GetPreferredOutputStreamParameters is written to return an optimal buffer size for WebAudio. And optimal for WebAudio is as small as possible. So the minimum buffer size allowed to specify would be the current HW buffer size. Dale's recommendation on putting kMinimumOutputBufferSize and kMaximumOutputBufferSize to limits.h and switching AudioManager (and AudioLatency::GetExactBufferSize) to using them should be enough, isn't it? https://codereview.chromium.org/2908073002/diff/20001/media/audio/cras/audio_... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2908073002/diff/20001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:303: int buffer_size = AudioLatency::GetDefaultAudioBufferSize(false, sample_rate); Sorry, now I lost the track of what is going on. Why is the need to modify GetPreferredOutputStreamParameters? They are not WebAudio-only. For WebAudio GetPreferredOutputStreamParameters() result is returned to AudioManagerBase::GetDefaultOutputStreamParameters() and ends up in AudioOutputDevice::GetOutputDeviceInfo() on the renderer side. So HW buffer size will be minimum buffer size both on ChromeOS and Mac if I'm not mistaken. Why not to follow dalecurtis@'s recommendation (which I've just noticed) described in the bug the CL is referencing (https://bugs.chromium.org/p/chromium/issues/detail?id=708917)? AudioLatency is renderer-side application-aware concept, and AudioManager is browser-side platform audio system abstraction; none of them can depend on the other.
https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (left): https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:69: return std::min(4096, Can't remember the actual value, but make this consistent with the playback latency value. The user should be able to specify a value up to the playback latency. Perhaps even using GetHighLatencyBufferSize to figure this out.
On 2017/05/30 at 16:04:33, olka wrote: > GetPreferredOutputStreamParameters is written to return an optimal buffer size for WebAudio. And optimal for WebAudio is as small as possible. So the minimum buffer size allowed to specify would be the current HW buffer size. > > Dale's recommendation on putting kMinimumOutputBufferSize and kMaximumOutputBufferSize to limits.h and switching AudioManager (and AudioLatency::GetExactBufferSize) to using them should be enough, isn't it? I had intended to put these as constants in limits.h as Dale suggested but found that the real minimum buffer size is determined via a calculation in some cases (it's dependent on the sample rate on OSX and Windows, and the board type for CRAS). Would you suggest moving the functions into limits.h? The maximums appear to be constants though (where they exist) so should be easy enough to move there. > https://codereview.chromium.org/2908073002/diff/20001/media/audio/cras/audio_... > File media/audio/cras/audio_manager_cras.cc (right): > > https://codereview.chromium.org/2908073002/diff/20001/media/audio/cras/audio_... > media/audio/cras/audio_manager_cras.cc:303: int buffer_size = AudioLatency::GetDefaultAudioBufferSize(false, sample_rate); > Sorry, now I lost the track of what is going on. > Why is the need to modify GetPreferredOutputStreamParameters? They are not WebAudio-only. > > For WebAudio GetPreferredOutputStreamParameters() result is returned to AudioManagerBase::GetDefaultOutputStreamParameters() and ends up in AudioOutputDevice::GetOutputDeviceInfo() on the renderer side. So HW buffer size will be minimum buffer size both on ChromeOS and Mac if I'm not mistaken. Right now that's correct, what I was proposing was allowing the default HW buffer size to have a different value than the minimum. To take a step back, what we want is access to a buffer size of 128 on OSX and CRAS (for live monitoring of recorded instruments) even if this can glitch on some hardware. We're fine with leaving the current OSX default at 256 (today's output buffer size) if that's what others need, as long as we can request 128 explicitly via the latency hint. This CL is an attempt to move in the direction of decoupling the minimum from the default, and I've verified that on OSX this approach behaves as we'd like. If this isn't an acceptable approach though then we'll instead work on getting the default OSX buffer size reduced globally back to 128 (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?typ...), and do testing to push the defaults down for specific boards with CRAS as well (https://bugs.chromium.org/p/chromium/issues/detail?id=581679). > Why not to follow dalecurtis@'s recommendation (which I've just noticed) described in the bug the CL is referencing (https://bugs.chromium.org/p/chromium/issues/detail?id=708917)? > > AudioLatency is renderer-side application-aware concept, and AudioManager is browser-side platform audio system abstraction; none of them can depend on the other. Thanks again olka@!
On 2017/05/30 at 18:05:58, rtoy wrote: > https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/... > File content/renderer/media/renderer_webaudiodevice_impl.cc (left): > > https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/... > content/renderer/media/renderer_webaudiodevice_impl.cc:69: return std::min(4096, > Can't remember the actual value, but make this consistent with the playback latency value. The user should be able to specify a value up to the playback latency. Perhaps even using GetHighLatencyBufferSize to figure this out. We could definitely make this code use the playback latency as its max and then avoid the need to make the hardware max available, though dalecurtis@ mentioned in the previous CL that he wasn't sure if the AudioLatency methods should call each other: https://codereview.chromium.org/2750543003/diff/80001/media/base/audio_latenc...
(1) > I had intended to put these as constants in limits.h as Dale suggested but found > that the real minimum buffer size is determined via a calculation in some cases > (it's dependent on the sample rate on OSX and Windows, and the board type for > CRAS). Would you suggest moving the functions into limits.h? (2) > To take a step back, what we want is access to a buffer size of 128 on OSX and > CRAS (for live monitoring of recorded instruments) even if this can glitch on > some hardware. We're fine with leaving the current OSX default at 256 (today's > output buffer size) if that's what others need, as long as we can request 128 > explicitly via the latency hint. This CL is an attempt to move in the direction > of decoupling the minimum from the default, and I've verified that on OSX this > approach behaves as we'd like. (1) and (2) sound a bit contradictory to me. If you are ok with glitching and going to overwrite the minimum (2), why would you care about the real mimimum value (1)? And if the real minimum is important, then why to overwrite it? BTW, another reason to care about the real minimum besides glitches is that lower buffer size means higher power consumption, and it may be a problem for low-end devices when on battery.
On 2017/05/31 at 10:43:22, olka wrote: > (1) > > I had intended to put these as constants in limits.h as Dale suggested but found > > that the real minimum buffer size is determined via a calculation in some cases > > (it's dependent on the sample rate on OSX and Windows, and the board type for > > CRAS). Would you suggest moving the functions into limits.h? > > (2) > > To take a step back, what we want is access to a buffer size of 128 on OSX and > > CRAS (for live monitoring of recorded instruments) even if this can glitch on > > some hardware. We're fine with leaving the current OSX default at 256 (today's > > output buffer size) if that's what others need, as long as we can request 128 > > explicitly via the latency hint. This CL is an attempt to move in the direction > > of decoupling the minimum from the default, and I've verified that on OSX this > > approach behaves as we'd like. > > (1) and (2) sound a bit contradictory to me. If you are ok with glitching and going to overwrite the minimum (2), why would you care about the real mimimum value (1)? > And if the real minimum is important, then why to overwrite it? > BTW, another reason to care about the real minimum besides glitches is that lower buffer size means higher power consumption, and it may be a problem for low-end devices when on battery. For (1) we need to know the minimum value that the platform-specific AudioManager will allow us to request so that GetExactBufferSize() can return an accurate value. Right now this is the same as the default (the HW buffer size) but if we allow the minimum to be different then we need a way to get this additional value. For (2) we don't want to overwrite the real minimum, we just want to leave the current minimum (256 on OSX) as the "default" or "preferred" size and add an additional new real minimum (128), which the AudioManager will give us if we request it explicitly. Of course requesting a lower value than the default would change the current "HW buffer size" though, so I agree that this may be confusing. Regarding glitching, we don't want glitches but do want control in Javascript to try lower buffer size values, and increase the value if a low one causes bad user experience. Battery life is also a concern but we'd like to have control here if possible. We would be happy if the minimum OSX buffer size were just brought back to 128 for everyone as per xians' comment in audio_manager_mac, however if most other OSX audio users are happy with the current size (for battery life and glitching) I have thought that it makes more sense to leave the default alone but allow explicit lower values in cases like ours (browser-based DAWs). I guess another option would be the reverse of what I've done, so reduce the minimum to 128 on OSX but update GetInteractiveBufferSize() to return 256, though I don't know how that would affect other parts of the Chromium audio system. Thanks again for the help here! :)
On Wed, May 31, 2017 at 12:41 AM, <andrew.macpherson@soundtrap.com> wrote: > On 2017/05/30 at 16:04:33, olka wrote: > > GetPreferredOutputStreamParameters is written to return an optimal > buffer size > for WebAudio. And optimal for WebAudio is as small as possible. So the > minimum > buffer size allowed to specify would be the current HW buffer size. > > > > Dale's recommendation on putting kMinimumOutputBufferSize and > kMaximumOutputBufferSize to limits.h and switching AudioManager (and > AudioLatency::GetExactBufferSize) to using them should be enough, isn't > it? > > I had intended to put these as constants in limits.h as Dale suggested but > found > that the real minimum buffer size is determined via a calculation in some > cases > (it's dependent on the sample rate on OSX and Windows, and the board type > for > CRAS). Would you suggest moving the functions into limits.h? > > The maximums appear to be constants though (where they exist) so should be > easy > enough to move there. > > > > https://codereview.chromium.org/2908073002/diff/20001/ > media/audio/cras/audio_manager_cras.cc > > File media/audio/cras/audio_manager_cras.cc (right): > > > > > https://codereview.chromium.org/2908073002/diff/20001/ > media/audio/cras/audio_manager_cras.cc#newcode303 > > media/audio/cras/audio_manager_cras.cc:303: int buffer_size = > AudioLatency::GetDefaultAudioBufferSize(false, sample_rate); > > Sorry, now I lost the track of what is going on. > > Why is the need to modify GetPreferredOutputStreamParameters? They are > not > WebAudio-only. > > > > For WebAudio GetPreferredOutputStreamParameters() result is returned to > AudioManagerBase::GetDefaultOutputStreamParameters() and ends up in > AudioOutputDevice::GetOutputDeviceInfo() on the renderer side. So HW > buffer size > will be minimum buffer size both on ChromeOS and Mac if I'm not mistaken. > > Right now that's correct, what I was proposing was allowing the default HW > buffer size to have a different value than the minimum. > > To take a step back, what we want is access to a buffer size of 128 on OSX > and > CRAS (for live monitoring of recorded instruments) even if this can glitch > on > some hardware. We're fine with leaving the current OSX default at 256 > (today's > output buffer size) if that's what others need, as long as we can request > 128 > explicitly via the latency hint. This CL is an attempt to move in the > direction > of decoupling the minimum from the default, and I've verified that on OSX > this > approach behaves as we'd like. > > If this isn't an acceptable approach though then we'll instead work on > getting > the default OSX buffer size reduced globally back to 128 > (https://cs.chromium.org/chromium/src/media/audio/mac/ > audio_manager_mac.cc?type=cs&l=895), > and do testing to push the defaults down for specific boards with CRAS as > well > (https://bugs.chromium.org/p/chromium/issues/detail?id=581679). > For those unfamiliar with how the latencyHint came to be, the use case that Andrew wants is *exactly* why the latencyHint allows a float value in addition to the categories. It is intended for experienced people who know what they are doing to request an appropriate latency. And for the first few years, OSX used 128 just fine. It was changed to 256 to work better with webrtc. And on linux a few years ago, I used to have a custom .asoundrc file that allowed me to use a size of 256 just fine. (I tried again lately and my .asoundrc no longer works, but I also don't remember exactly how I used it before.) > > > > Why not to follow dalecurtis@'s recommendation (which I've just noticed) > described in the bug the CL is referencing > (https://bugs.chromium.org/p/chromium/issues/detail?id=708917)? > > > > AudioLatency is renderer-side application-aware concept, and > AudioManager is > browser-side platform audio system abstraction; none of them can depend on > the > other. > > Thanks again olka@! > > https://codereview.chromium.org/2908073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/31 12:48:52, Andrew MacPherson wrote: > On 2017/05/31 at 10:43:22, olka wrote: > > (1) > > > I had intended to put these as constants in limits.h as Dale suggested but > found > > > that the real minimum buffer size is determined via a calculation in some > cases > > > (it's dependent on the sample rate on OSX and Windows, and the board type > for > > > CRAS). Would you suggest moving the functions into limits.h? > > > > (2) > > > To take a step back, what we want is access to a buffer size of 128 on OSX > and > > > CRAS (for live monitoring of recorded instruments) even if this can glitch > on > > > some hardware. We're fine with leaving the current OSX default at 256 > (today's > > > output buffer size) if that's what others need, as long as we can request > 128 > > > explicitly via the latency hint. This CL is an attempt to move in the > direction > > > of decoupling the minimum from the default, and I've verified that on OSX > this > > > approach behaves as we'd like. > > > > (1) and (2) sound a bit contradictory to me. If you are ok with glitching and > going to overwrite the minimum (2), why would you care about the real mimimum > value (1)? > > And if the real minimum is important, then why to overwrite it? > > BTW, another reason to care about the real minimum besides glitches is that > lower buffer size means higher power consumption, and it may be a problem for > low-end devices when on battery. > > For (1) we need to know the minimum value that the platform-specific > AudioManager will allow us to request so that GetExactBufferSize() can return an > accurate value. Right now this is the same as the default (the HW buffer size) > but if we allow the minimum to be different then we need a way to get this > additional value. > > For (2) we don't want to overwrite the real minimum, we just want to leave the > current minimum (256 on OSX) as the "default" or "preferred" size and add an > additional new real minimum (128), which the AudioManager will give us if we > request it explicitly. Of course requesting a lower value than the default would > change the current "HW buffer size" though, so I agree that this may be > confusing. > > Regarding glitching, we don't want glitches but do want control in Javascript to > try lower buffer size values, and increase the value if a low one causes bad > user experience. Battery life is also a concern but we'd like to have control > here if possible. > > We would be happy if the minimum OSX buffer size were just brought back to 128 > for everyone as per xians' comment in audio_manager_mac, however if most other > OSX audio users are happy with the current size (for battery life and glitching) > I have thought that it makes more sense to leave the default alone but allow > explicit lower values in cases like ours (browser-based DAWs). > > I guess another option would be the reverse of what I've done, so reduce the > minimum to 128 on OSX but update GetInteractiveBufferSize() to return 256, > though I don't know how that would affect other parts of the Chromium audio > system. > > Thanks again for the help here! :) Thank you both for clarification! So you are modifying GetPreferredOutputStreamParameters() to set output buffer size to |input_params| buffer size cropped to whatever minimum WebAudio prefers, which may be lower than "safe minimum" value used normally. Right? 1) "we need to know the minimum value that the platform-specific AudioManager will allow us to request so that GetExactBufferSize() can return an accurate value" AudioRendererSink interface does not give any guarantee that the physical signal will be rendered at the buffer size specified in Initialize() call (https://cs.chromium.org/chromium/src/content/renderer/media/renderer_webaudio...). It only guarantees the buffer size requested in Render() callback will match the value specified in Initialize(). So repeating AudioManager calculations on the renderer side to guess HW buffer size value violates abstractions. Also there is no guarantee that actual HW buffer size remains constant throughout audio stream lifetime. Why WebAudio cares about knowing the actual minimum? Why not to limit buffer size to 128 (which is the chunk at which WebAudio graph is pulled) and let GetPreferredOutputStreamParameters() figure the actual minimum? Does WebAudio intend to report the actual physical buffer size to the user? (I remember there were discussion about it, what was the conclusion?) 2) rtoy@: I missed that in the previous reviews: What is the difference between BaseAudioContext::framesPerBuffer() and BaseAudioContext::callbackBufferSize()? And same for AudioDestination::FramesPerBuffer(), AudioDestination::CallbackBufferSize() and AudioDestination::HardwareBufferSize()? 3) rtoy@: "for the first few years, OSX used 128 just fine. It was changed to 256 to work better with webrtc." I'm not familiar with a history of the change; from what I see in the comment (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=A...) it does not look like WebRTC-specific. Could you clarify? Also, if it was important for WebRTC before, why it's not anymore? 4) Andrew - I'm not fluent in this code, so when you refer to something like "per xians' comment in audio_manager_mac" - could you please add a link to the line of code: it would save me some time. Thanks!
On 2017/06/01 at 10:46:27, olka wrote: > Thank you both for clarification! > > So you are modifying GetPreferredOutputStreamParameters() to set output buffer size to |input_params| buffer size cropped to whatever minimum WebAudio prefers, which may be lower than "safe minimum" value used normally. Right? Yes, that's right. > 1) "we need to know the minimum value that the platform-specific > AudioManager will allow us to request so that GetExactBufferSize() can return an > accurate value" > > AudioRendererSink interface does not give any guarantee that the physical signal will be rendered at the buffer size specified in Initialize() call > (https://cs.chromium.org/chromium/src/content/renderer/media/renderer_webaudio...). > It only guarantees the buffer size requested in Render() callback will match the value specified in Initialize(). > > So repeating AudioManager calculations on the renderer side to guess HW buffer size value violates abstractions. > Also there is no guarantee that actual HW buffer size remains constant throughout audio stream lifetime. I agree that this isn't great and this came up in the earlier CL as well with no clear solution. What we really want is a way to query the "real" buffer size instead of trying to guess what the AudioManager will do. > Why WebAudio cares about knowing the actual minimum? Why not to limit buffer size to 128 (which is the chunk at which WebAudio graph is pulled) and let GetPreferredOutputStreamParameters() figure the actual minimum? > Does WebAudio intend to report the actual physical buffer size to the user? (I remember there were discussion about it, what was the conclusion?) That's exactly it, we need to report the chosen value back up to the AudioContext so that the user knows the real size that the system is using in case it differs from the requested size. For example today if I request 128 on OSX via latencyHint, AudioContext.baseLatency tells me that the real chosen size is 256. > 2) rtoy@: I missed that in the previous reviews: > What is the difference between BaseAudioContext::framesPerBuffer() and BaseAudioContext::callbackBufferSize()? > And same for AudioDestination::FramesPerBuffer(), AudioDestination::CallbackBufferSize() and AudioDestination::HardwareBufferSize()? > > 3) rtoy@: "for the first few years, OSX used 128 just fine. It was changed to 256 to work better with webrtc." > I'm not familiar with a history of the change; from what I see in the comment > (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=A...) > it does not look like WebRTC-specific. Could you clarify? > Also, if it was important for WebRTC before, why it's not anymore? > > 4) Andrew - I'm not fluent in this code, so when you refer to something like "per xians' comment in audio_manager_mac" - could you please add a link to the line of code: it would save me some time. Thanks! Sorry about that, I'd included a link in my previous comment but it's the line you're referring to in your link above (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=A...). Thanks again olka@!
It's unfortunate that we have to report actual buffer size to the user. We were against that, since HW rendering parameters are hidden from upper layers by the current design. I don't see any clean solution to it at the moment. Even if AudioManager calculations are repeated on renderer side, there is no guarantee they reflect the actual buffer size (though probability of that is quite high). In any case 1) AudioManager and AudioLatency cannot depend on each other. 2) Tearing out platform-specific parts of AudioManager and combining them in one file under conditional compiling reduces readability. 3) Not sure if limits.h is a good place for such calculations. I would probably introduce some "BufferSizeEstimator" interface with platform-specific implementations, used by both AudioManager and WebAudio rendering. But I'm not even sure if it would look fine. Andrew, could try to figure out something else? dalecurtis@ could you advice? rtoy@ could you answer my question in the previous comment? Thanks! On 2017/06/01 12:47:04, Andrew MacPherson wrote: > On 2017/06/01 at 10:46:27, olka wrote: > > Thank you both for clarification! > > > > So you are modifying GetPreferredOutputStreamParameters() to set output buffer > size to |input_params| buffer size cropped to whatever minimum WebAudio prefers, > which may be lower than "safe minimum" value used normally. Right? > > Yes, that's right. > > > 1) "we need to know the minimum value that the platform-specific > > AudioManager will allow us to request so that GetExactBufferSize() can return > an > > accurate value" > > > > AudioRendererSink interface does not give any guarantee that the physical > signal will be rendered at the buffer size specified in Initialize() call > > > (https://cs.chromium.org/chromium/src/content/renderer/media/renderer_webaudio...). > > It only guarantees the buffer size requested in Render() callback will match > the value specified in Initialize(). > > > > So repeating AudioManager calculations on the renderer side to guess HW buffer > size value violates abstractions. > > Also there is no guarantee that actual HW buffer size remains constant > throughout audio stream lifetime. > > I agree that this isn't great and this came up in the earlier CL as well with no > clear solution. What we really want is a way to query the "real" buffer size > instead of trying to guess what the AudioManager will do. > > > Why WebAudio cares about knowing the actual minimum? Why not to limit buffer > size to 128 (which is the chunk at which WebAudio graph is pulled) and let > GetPreferredOutputStreamParameters() figure the actual minimum? > > Does WebAudio intend to report the actual physical buffer size to the user? (I > remember there were discussion about it, what was the conclusion?) > > That's exactly it, we need to report the chosen value back up to the > AudioContext so that the user knows the real size that the system is using in > case it differs from the requested size. For example today if I request 128 on > OSX via latencyHint, AudioContext.baseLatency tells me that the real chosen size > is 256. > > > 2) rtoy@: I missed that in the previous reviews: > > What is the difference between BaseAudioContext::framesPerBuffer() and > BaseAudioContext::callbackBufferSize()? > > And same for AudioDestination::FramesPerBuffer(), > AudioDestination::CallbackBufferSize() and > AudioDestination::HardwareBufferSize()? > > > > 3) rtoy@: "for the first few years, OSX used 128 just fine. It was changed to > 256 to work better with webrtc." > > I'm not familiar with a history of the change; from what I see in the comment > > > (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=A...) > > it does not look like WebRTC-specific. Could you clarify? > > Also, if it was important for WebRTC before, why it's not anymore? > > > > 4) Andrew - I'm not fluent in this code, so when you refer to something like > "per xians' comment in audio_manager_mac" - could you please add a link to the > line of code: it would save me some time. Thanks! > > Sorry about that, I'd included a link in my previous comment but it's the line > you're referring to in your link above > (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=A...). > > Thanks again olka@!
On 2017/06/01 at 10:46:27, olka wrote: > On 2017/05/31 12:48:52, Andrew MacPherson wrote: > > On 2017/05/31 at 10:43:22, olka wrote: > > > (1) > > > > I had intended to put these as constants in limits.h as Dale suggested but > > found > > > > that the real minimum buffer size is determined via a calculation in some > > cases > > > > (it's dependent on the sample rate on OSX and Windows, and the board type > > for > > > > CRAS). Would you suggest moving the functions into limits.h? > > > > > > (2) > > > > To take a step back, what we want is access to a buffer size of 128 on OSX > > and > > > > CRAS (for live monitoring of recorded instruments) even if this can glitch > > on > > > > some hardware. We're fine with leaving the current OSX default at 256 > > (today's > > > > output buffer size) if that's what others need, as long as we can request > > 128 > > > > explicitly via the latency hint. This CL is an attempt to move in the > > direction > > > > of decoupling the minimum from the default, and I've verified that on OSX > > this > > > > approach behaves as we'd like. > > > > > > (1) and (2) sound a bit contradictory to me. If you are ok with glitching and > > going to overwrite the minimum (2), why would you care about the real mimimum > > value (1)? > > > And if the real minimum is important, then why to overwrite it? > > > BTW, another reason to care about the real minimum besides glitches is that > > lower buffer size means higher power consumption, and it may be a problem for > > low-end devices when on battery. > > > > For (1) we need to know the minimum value that the platform-specific > > AudioManager will allow us to request so that GetExactBufferSize() can return an > > accurate value. Right now this is the same as the default (the HW buffer size) > > but if we allow the minimum to be different then we need a way to get this > > additional value. > > > > For (2) we don't want to overwrite the real minimum, we just want to leave the > > current minimum (256 on OSX) as the "default" or "preferred" size and add an > > additional new real minimum (128), which the AudioManager will give us if we > > request it explicitly. Of course requesting a lower value than the default would > > change the current "HW buffer size" though, so I agree that this may be > > confusing. > > > > Regarding glitching, we don't want glitches but do want control in Javascript to > > try lower buffer size values, and increase the value if a low one causes bad > > user experience. Battery life is also a concern but we'd like to have control > > here if possible. > > > > We would be happy if the minimum OSX buffer size were just brought back to 128 > > for everyone as per xians' comment in audio_manager_mac, however if most other > > OSX audio users are happy with the current size (for battery life and glitching) > > I have thought that it makes more sense to leave the default alone but allow > > explicit lower values in cases like ours (browser-based DAWs). > > > > I guess another option would be the reverse of what I've done, so reduce the > > minimum to 128 on OSX but update GetInteractiveBufferSize() to return 256, > > though I don't know how that would affect other parts of the Chromium audio > > system. > > > > Thanks again for the help here! :) > > Thank you both for clarification! > > So you are modifying GetPreferredOutputStreamParameters() to set output buffer size to |input_params| buffer size cropped to whatever minimum WebAudio prefers, which may be lower than "safe minimum" value used normally. Right? > > 1) "we need to know the minimum value that the platform-specific > AudioManager will allow us to request so that GetExactBufferSize() can return an > accurate value" > > AudioRendererSink interface does not give any guarantee that the physical signal will be rendered at the buffer size specified in Initialize() call > (https://cs.chromium.org/chromium/src/content/renderer/media/renderer_webaudio...). > It only guarantees the buffer size requested in Render() callback will match the value specified in Initialize(). > > So repeating AudioManager calculations on the renderer side to guess HW buffer size value violates abstractions. > Also there is no guarantee that actual HW buffer size remains constant throughout audio stream lifetime. I didn't know this. Why would that change? Because the output device changed in the middle of the stream? The user changed the sample rate of the output device? (WebAudio on OSX used to become silent if you changed the sample rate of the output. It seems that OSX handles this better now because the output isn't silent anymore.) And if the HW buffer size does change how does it affect all of the callbacks? (This isn't relevant to this CL right now.) > > Why WebAudio cares about knowing the actual minimum? Why not to limit buffer size to 128 (which is the chunk at which WebAudio graph is pulled) and let GetPreferredOutputStreamParameters() figure the actual minimum? > Does WebAudio intend to report the actual physical buffer size to the user? (I remember there were discussion about it, what was the conclusion?) It's not really the physical buffer size, I think, but the callback size. Ideally it should be 128 because that gives the best latency. And for interactive musical applications that many people want to do with WebAudio, that's really important. Some musicians claim they can hear a latency of 3-5 ms (which is about 128 samples at 44.1 kHz). > > 2) rtoy@: I missed that in the previous reviews: > What is the difference between BaseAudioContext::framesPerBuffer() and BaseAudioContext::callbackBufferSize()? > And same for AudioDestination::FramesPerBuffer(), AudioDestination::CallbackBufferSize() and AudioDestination::HardwareBufferSize()? I'm also now confused. Can you answer that Andrew? > > 3) rtoy@: "for the first few years, OSX used 128 just fine. It was changed to 256 to work better with webrtc." > I'm not familiar with a history of the change; from what I see in the comment > (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=A...) > it does not look like WebRTC-specific. Could you clarify? > Also, if it was important for WebRTC before, why it's not anymore? I will have to find the actual CL that made this change. My memory is that 128 didn't work well with WebRTC. I think the issue was that if WebAudio and WebRTC were both being used, the audio system was forced to use 128 (by WebAudio) and that made WebRTC glitch. And I believe it's still important which is why the default is "interactive" and implies 256. For people who know what they're doing, we want to allow 128. They get to pick up the pieces when something breaks. :-)
>> Also there is no guarantee that actual HW buffer size remains constant throughout audio stream lifetime. >I didn't know this. Why would that change? Because the output device changed in >the middle of the stream? The user changed the sample rate of the output >device? (WebAudio on OSX used to become silent if you changed the sample rate >of the output. It seems that OSX handles this better now because the output >isn't silent anymore.) >And if the HW buffer size does change how does it affect all of the callbacks? >(This isn't relevant to this CL right now.) There are rebuffering layers on browser side. Renderer-side callback buffer size does not change; but physical output buffer size does. On Mac for example the actual buffer size is the minimum of all buffer sizes requested by output streams rendering to that device. And it changes for everybody when lowest-latency stream joins or goes away.
If we want to modernize this API for mojo, the right thing would be to define some new API as part of the AudioSystem interface that this class can talk to. The minimums could continue to live adjacent to the AudioManager in that case. This is obviously a bit complicated for a static interface like this though, so I think it's okay to have this information in AudioLatency and have the AM and WebAudio use it like is proposed here.
On 2017/06/02 at 15:21:13, olka wrote: > I don't see any clean solution to it at the moment. > Even if AudioManager calculations are repeated on renderer side, there is no guarantee they reflect the actual buffer size (though probability of that is quite high). > > In any case > 1) AudioManager and AudioLatency cannot depend on each other. > 2) Tearing out platform-specific parts of AudioManager and combining them in one file under conditional compiling reduces readability. > 3) Not sure if limits.h is a good place for such calculations. > > I would probably introduce some "BufferSizeEstimator" interface with platform-specific implementations, used by both AudioManager and WebAudio rendering. But I'm not even sure if it would look fine. > > Andrew, could try to figure out something else? This makes sense and I can definitely create the BufferSizeEstimator if that's the best approach. Another option could be to expose the maximums as constants in limits.h, then change the OSX minimum buffer size back to 128 but modify GetInteractiveBufferSize() to #ifdef OSX return 256. I'm not sure if there are other parts of the audio system that would be affected by this but this could be a pretty simple solution I think. On 2017/06/02 at 16:42:35, rtoy wrote: > On 2017/06/01 at 10:46:27, olka wrote: > > 2) rtoy@: I missed that in the previous reviews: > > What is the difference between BaseAudioContext::framesPerBuffer() and BaseAudioContext::callbackBufferSize()? > > And same for AudioDestination::FramesPerBuffer(), AudioDestination::CallbackBufferSize() and AudioDestination::HardwareBufferSize()? > > I'm also now confused. Can you answer that Andrew? It looks like FramesPerBuffer() and CallbackBufferSize() both refer to the same thing, i.e. WebAudioDevice::FramesPerBuffer(). I think FramesPerBuffer() should just be removed from both AudioDestination and BaseAudioContext and any code referencing it should use CallbackBufferSize() instead. AudioDestination::HardwareBufferSize() always refers to the device's minimum/"interactive" buffer size. So on OSX (for example) this will always be 256 regardless of the real buffer size being used due to the latencyHint. I believe the only reason that this value is still relevant is because it's used to generate a histogram in AudioDestination. > > 3) rtoy@: "for the first few years, OSX used 128 just fine. It was changed to 256 to work better with webrtc." > > I'm not familiar with a history of the change; from what I see in the comment > > (https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=A...) > > it does not look like WebRTC-specific. Could you clarify? > > Also, if it was important for WebRTC before, why it's not anymore? > > I will have to find the actual CL that made this change. My memory is that 128 didn't work well with WebRTC. I think the issue was that if WebAudio and WebRTC were both being used, the audio system was forced to use 128 (by WebAudio) and that made WebRTC glitch. > > And I believe it's still important which is why the default is "interactive" and implies 256. For people who know what they're doing, we want to allow 128. They get to pick up the pieces when something breaks. :-) It looks like this is the original CL for the OSX buffer size change: https://codereview.chromium.org/673183002 On 2017/06/02 at 20:38:30, dalecurtis wrote: > If we want to modernize this API for mojo, the right thing would be to define some new API as part of the AudioSystem interface that this class can talk to. The minimums could continue to live adjacent to the AudioManager in that case. > > This is obviously a bit complicated for a static interface like this though, so I think it's okay to have this information in AudioLatency and have the AM and WebAudio use it like is proposed here. If leaving it in AudioLatency is an acceptable solution then I will look at moving the code from the other platforms in there too, I'll wait to hear back from olka@ first though (or you) around the idea of changing GetInteractiveBufferSize() instead. Thanks again everyone!
On 2017/06/02 20:38:30, DaleCurtis wrote: > If we want to modernize this API for mojo, the right thing would be to define > some new API as part of the AudioSystem interface that this class can talk to. > The minimums could continue to live adjacent to the AudioManager in that case. > > This is obviously a bit complicated for a static interface like this though, so > I think it's okay to have this information in AudioLatency and have the AM and > WebAudio use it like is proposed here. There is no plan to make AudioSystem accessible from renderer, since AudioSystem operates on the real device ids and does not check for permissions. So if we introduce such an interface, it should be some browser-side service on top of AudioSystem.
On 2017/06/03 12:22:11, Andrew MacPherson wrote: > On 2017/06/02 at 15:21:13, olka wrote: > > I don't see any clean solution to it at the moment. > > Even if AudioManager calculations are repeated on renderer side, there is no > guarantee they reflect the actual buffer size (though probability of that is > quite high). > > > > In any case > > 1) AudioManager and AudioLatency cannot depend on each other. > > 2) Tearing out platform-specific parts of AudioManager and combining them in > one file under conditional compiling reduces readability. > > 3) Not sure if limits.h is a good place for such calculations. > > > > I would probably introduce some "BufferSizeEstimator" interface with > platform-specific implementations, used by both AudioManager and WebAudio > rendering. But I'm not even sure if it would look fine. > > > > Andrew, could try to figure out something else? > > This makes sense and I can definitely create the BufferSizeEstimator if that's > the best approach. Another option could be to expose the maximums as constants > in limits.h, then change the OSX minimum buffer size back to 128 but modify > GetInteractiveBufferSize() to #ifdef OSX return 256. I'm not sure if there are > other parts of the audio system that would be affected by this but this could be > a pretty simple solution I think. > This would be the best approach, but it would effectively revert https://codereview.chromium.org/673183002 (thanks for digging it up) Let me get back to you after I figure out the consequences.
How about modifying AudioManagerMac::GetPreferredOutputStreamParameters() to return 128 as buffer size if |input_parameters| specify it as 128 (and leave the return value as it is now in all other cases) And make AudioLatency::GetExactBufferSize to crop to 128 on Mac (and publish all buffer size limits in limits.h)? |input_parameters| are specified only during output stream creation here (https://cs.chromium.org/chromium/src/media/audio/audio_manager_base.cc?type=c...)
On 2017/06/08 at 16:08:27, olka wrote: > How about modifying AudioManagerMac::GetPreferredOutputStreamParameters() to return 128 as buffer size if |input_parameters| specify it as 128 (and leave the return value as it is now in all other cases) > And make AudioLatency::GetExactBufferSize to crop to 128 on Mac (and publish all buffer size limits in limits.h)? > > |input_parameters| are specified only during output stream creation here (https://cs.chromium.org/chromium/src/media/audio/audio_manager_base.cc?type=c...) Thanks olka, that sounds great! I've made the changes here, let me know if this is what you were thinking. I couldn't find maximum buffer sizes specified for Windows or Android (see note in limits.h, which I'll remove after discussion). Also note that this CL currently means that a buffer size of 128 can be requested on OSX even for high samples rates, something that wasn't possible before. We don't need this but it's because there is a calculation to determine the "real" minimum buffer size here that we'd have to expose as a function somewhere: https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=a... I'd love to drop the minimum buffer size on CRAS as well to 256, leaving the default at 512 exactly as we've done for OSX but if there's any concern with that it can wait for a follow-up CL. Actually allowing the minimum on OSX could wait for a small follow-up CL as well if you prefer, since the bug associated with this CL was just about exposing the min/max values for use. rtoy@/hongchan@: I think we'll need to modify AudioDestination since some platforms allow a max buffer size of 8192 (see note there too, which I'll also remove). Thanks again for the help here!
Description was changed from ========== Make OS audio buffer size limits visible. Add static methods GetMinimumAudioBufferSize and GetMaximumAudioBufferSize to the AudioManager for use in latencyHint-based buffer size calculations. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Make OS audio buffer size limits visible. Add static methods GetMinimumAudioBufferSize and GetMaximumAudioBufferSize to the AudioManager for use in latencyHint-based buffer size calculations. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Yes, it's what I meant, but I'm not sure yet how good it is :) > I couldn't find maximum buffer sizes specified for Windows or Android (see note > in limits.h, which I'll remove after discussion). Not sure about this. +henrika@ for Android. BTW, if I read the code correctly (https://cs.chromium.org/chromium/src/media/audio/win/audio_manager_win.cc?typ...) we do not control output buffer size on Windows at all, so 'Exact" latency may work incorrectly there. If it's so, we AudioLatency::GetExactBufferSize() should be fixed to reflect it. > > Also note that this CL currently means that a buffer size of 128 can be > requested on OSX even for high samples rates, something that wasn't possible > before. We don't need this but it's because there is a calculation to determine > the "real" minimum buffer size here that we'd have to expose as a function > somewhere: > https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=a... Right. If you are dropping buffer size for CRAS in this CL (and so don't need platform-specific calculations for any platforms other than Mac), maybe //media/base/mac would be the right place to put it in.
https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:836: if (has_valid_input_params) { combine into a conditional assignment? int buffer_size = has_valid_input_params ? .. : .. https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:840: buffer_size = std::min( Add a test to media/audio/audio_manager_unittest.cc to make sure AudioManager::MakeAudioOutputStreamProxy behaves as we expect? https://codereview.chromium.org/2908073002/diff/40001/media/base/limits.h File media/base/limits.h (right): https://codereview.chromium.org/2908073002/diff/40001/media/base/limits.h#new... media/base/limits.h:62: kMinimumInputOutputBufferSize = 128, To keep it simple, maybe just name them kMinAudioBufferSize and kMaxAudioBufferSize on all the platforms? https://codereview.chromium.org/2908073002/diff/40001/media/base/limits.h#new... media/base/limits.h:70: kMinimumOutputBufferSize = 512, This is a bit confusing, see AudioManagerCras::GetMinimumOutputBufferSizePerBoard. If you want to drop to 256 on CRAS it would probably be easier to do it in this CL. Or if we are going to have various platform-specific calculations for min/max, then it may be easier to keep minx/max in AudioLatency along with those calculations.
> I couldn't find maximum buffer sizes specified for Windows or Android (see note > in limits.h, which I'll remove after discussion). Not sure about this. +henrika@ for Android. First of all, I have not followed this discussion and might miss the context but the simple answer to the question is that it is not needed and it makes no sense to use such a value. Each platform is unique in its handling of buffer sizes and listing min and max values indicates that it is possible from a user perspective to set whatever is requested. And it is not. The selection of buffer size is much more complex and depends on: Android version, supported audio API, device etc. So. No value (max or min) is needed (or can be used) for Android.
In addition, storing a list of min/max buffer size without unit seems wrong to me. A buffer size of 128 (audio frames, audio samples per channel or something else?) is different in milliseconds depending on the sample rate. Hence: I can't see the value of maintaining such a list. How can we find one value valid for all versions of an OS and for all possible sample rates?
On 2017/06/09 at 10:46:55, olka wrote: > Yes, it's what I meant, but I'm not sure yet how good it is :) No problem, I figured this may take a few iterations to find the right solution. :) > > I couldn't find maximum buffer sizes specified for Windows or Android (see note > > in limits.h, which I'll remove after discussion). > > Not sure about this. +henrika@ for Android. > > BTW, if I read the code correctly (https://cs.chromium.org/chromium/src/media/audio/win/audio_manager_win.cc?typ...) > we do not control output buffer size on Windows at all, so 'Exact" latency may work incorrectly there. > If it's so, we AudioLatency::GetExactBufferSize() should be fixed to reflect it. That's what it looks like to me too. I guess this also means that the other latencyHint values have no effect on Windows (and Android) either? Should the other AudioLatency methods be updated to reflect this? > > Also note that this CL currently means that a buffer size of 128 can be > > requested on OSX even for high samples rates, something that wasn't possible > > before. We don't need this but it's because there is a calculation to determine > > the "real" minimum buffer size here that we'd have to expose as a function > > somewhere: > > https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?q=a... > > Right. If you are dropping buffer size for CRAS in this CL (and so don't need platform-specific calculations for any platforms other than Mac), > maybe //media/base/mac would be the right place to put it in. Done, I've added //media/base/mac/audio_util_mac.cc (not sure what the name should be). On 2017/06/09 at 11:36:23, henrika wrote: > In addition, storing a list of min/max buffer size without unit seems wrong to me. > A buffer size of 128 (audio frames, audio samples per channel or something else?) > is different in milliseconds depending on the sample rate. > > Hence: I can't see the value of maintaining such a list. How can we find one value > valid for all versions of an OS and for all possible sample rates? From our side we're only interested (for now) in OSX and CRAS, so if that means that GetExactBufferSize() always just returns the "hardware buffer size" on Windows and Android (and even Pulse) that would be fine. That would mean we don't need minimums or maximums for those platforms exposed anywhere. These mins and maxes already exist in the CRAS and OSX AudioManagers though so this CL is just about making them available elsewhere too. Does that make more sense? Thanks henrika@ and olka@! https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:836: if (has_valid_input_params) { On 2017/06/09 at 10:52:39, o1ka wrote: > combine into a conditional assignment? > int buffer_size = has_valid_input_params ? .. : .. Done. https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:840: buffer_size = std::min( On 2017/06/09 at 10:52:39, o1ka wrote: > Add a test to media/audio/audio_manager_unittest.cc to make sure AudioManager::MakeAudioOutputStreamProxy behaves as we expect? No problem, as soon as we agree that this approach is the right one I'll add a few tests here. https://codereview.chromium.org/2908073002/diff/40001/media/base/limits.h File media/base/limits.h (right): https://codereview.chromium.org/2908073002/diff/40001/media/base/limits.h#new... media/base/limits.h:62: kMinimumInputOutputBufferSize = 128, On 2017/06/09 at 10:52:39, o1ka wrote: > To keep it simple, maybe just name them kMinAudioBufferSize and kMaxAudioBufferSize on all the platforms? Done. https://codereview.chromium.org/2908073002/diff/40001/media/base/limits.h#new... media/base/limits.h:70: kMinimumOutputBufferSize = 512, On 2017/06/09 at 10:52:39, o1ka wrote: > This is a bit confusing, see > AudioManagerCras::GetMinimumOutputBufferSizePerBoard. > If you want to drop to 256 on CRAS it would probably be easier to do it in this CL. > Or if we are going to have various platform-specific calculations for min/max, then it may be easier to keep minx/max in AudioLatency along with those calculations. Done, I've dropped the minimum to 256 now. Maybe GetMinimumOutputBufferSizePerBoard should be renamed to GetPreferredOutputBufferSizePerBoard? I don't currently have a Chromium for ChromeOS development environment set up so I haven't verified that this change behaves as expected there. I will add unit tests as you suggest above though to ensure that this case is tested.
Friendly ping for olka@ re: updated buffer size calc on OSX and henrika@ re: disallowing exact buffer sizes on Android (and Windows). Thanks again!
Sorry for the delay. Approach SGTM; one question re:ChromeOS. https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; Hmm, but AudioManagerCras::GetPreferredOutputStreamParameters() does not always have 256 as a minimum, does it? It's defined by GetMinimumOutputBufferSizePerBoard(). And are we sure we always want to have 256 as minimum? Will WebAudio users really realize that 256 can result in a very poor experience on some boards on ChromeOS? https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:59: const size_t kFIFOSize = 8192; Should we set it to limits::kMaxAudioBufferSize? Or maybe max(8192,limits::kMaxAudioBufferSize)?
https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:58: // value and not a buffer size in the latencyHint. Could you file a bug and add the URL in your comment above?
https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/15 15:33:37, o1ka - slow wrote: > Hmm, but AudioManagerCras::GetPreferredOutputStreamParameters() does not always > have 256 as a minimum, does it? It's defined by > GetMinimumOutputBufferSizePerBoard(). > > And are we sure we always want to have 256 as minimum? Will WebAudio users > really realize that 256 can result in a very poor experience on some boards on > ChromeOS? > I think that's ok. Linux desktop can have really bad performance if we set the size to 256, even on my z840. (In fact even today, one of our demos is essentially broken on Linux because the pulseaudio callbacks are just too irregular. I think.)
Thanka olka@, rtoy@ and hongchan@! I'll aim to get another patch uploaded this week with additional unit tests as suggested earlier by olka@. https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:58: // value and not a buffer size in the latencyHint. On 2017/06/15 at 16:22:03, hongchan (OOO Jun 19-22) wrote: > Could you file a bug and add the URL in your comment above? No problem, I'll take care of this when I push the next patch. Just to note that until this is fixed though it will likely mean that it's possible to crash the process by requesting latency that results in a buffer size of 8192 on CRAS and Pulse. https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:59: const size_t kFIFOSize = 8192; On 2017/06/15 at 15:33:37, o1ka - slow wrote: > Should we set it to limits::kMaxAudioBufferSize? Or maybe max(8192,limits::kMaxAudioBufferSize)? It would need to be kMaxAudioBufferSize + AudioUtilities::kRenderQuantumFrames but I believe that would resolve the issue. Otherwise just 8192 + AudioUtilities::kRenderQuantumFrames, but I'll leave that up to hongchan@ and rtoy@.
Sorry for the delay here. olka@: I've added a test for this in audio_manager_unittest.cc, I wasn't sure how best to do it but I just added a wrapper class to be able to query the AudioManager's preferred buffer size and then another small test to ensure that we receive callbacks when using the minimum buffer size. Let me know if this needs anything else. hongchan@: I've filed https://crbug.com/737047 now and added a comment to the AudioDestination. Thanks!
Thanks so much for your contribution, Andrew! On Tue, Jun 27, 2017 at 1:20 AM <andrew.macpherson@soundtrap.com> wrote: > Sorry for the delay here. > > olka@: I've added a test for this in audio_manager_unittest.cc, I wasn't > sure > how best to do it but I just added a wrapper class to be able to query the > AudioManager's preferred buffer size and then another small test to ensure > that > we receive callbacks when using the minimum buffer size. Let me know if > this > needs anything else. > > hongchan@: I've filed https://crbug.com/737047 now and added a comment to > the > AudioDestination. > > Thanks! > > https://codereview.chromium.org/2908073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Just a couple of nits. Is there anything else that needs to be done? https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h File media/base/limits.h (right): https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h#new... media/base/limits.h:67: kMaxAudioBufferSize = 4096, Any particular reason why 4096 and not 8192 like you have for the other platforms? A comment on how you picked 8192 would be good too. https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h#new... media/base/limits.h:69: kMinAudioBufferSize = 512, A comment here explaining why the min is 512 would be good. Something as simple as saying that experiments with smaller size frequently produces glitches. https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h#new... media/base/limits.h:72: kMinAudioBufferSize = 256, Likewise a comment on the min value would be good.
https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; The point was to return here exactly the buffer size which will be used on the browser side, right? But we are always returning 256 here, instead of the real minimum which is GetMinimumOutputBufferSizePerBoard(). The current approach is not consistent with what is done for Mac, or am I missing something? https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:740: // as the minimum in some cases. The minimum can always be requested. "The minimum can always be requested." - what does it actually mean here? https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:748: AudioParameters params; Make it a separate test maybe? https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:765: // that no errors are returned. I'm not sure I'm following. min_params have (media::limits::kMinAudioBufferSize / 2) as a buffer size. Why would we want to create such a stream? https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:777: OnMoreData(testing::_, testing::_, testing::_, testing::_)) Do we want to test that AudioOutputStream callbacks request exactly the buffer size we asked for when we got it from AudioLatency::GetExactBufferSize()?
Thanks olka@ and rtoy@! https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > The point was to return here exactly the buffer size which will be used on the browser side, right? But we are always returning 256 here, instead of the real minimum which is GetMinimumOutputBufferSizePerBoard(). > > The current approach is not consistent with what is done for Mac, or am I missing something? I may not have been clear when I asked about this earlier but the intention here is to return per-board buffer sizes by default from GetPreferredOutputStreamParameters(), but to allow requesting as low as 256 on all boards if the user wants to. This does mean that GetMinimumOutputBufferSizePerBoard() is a confusing name though, maybe it should be updated to GetPreferredOutputBufferSizePerBoard()? https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:740: // as the minimum in some cases. The minimum can always be requested. On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > "The minimum can always be requested." - what does it actually mean here? I've updated the comment, I meant that the "preferred buffer size" on CRAS has different values depending on the board but the kMinAudioBufferSize can always be requested explicitly (via the latencyHint) regardless of per-board default values. https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:748: AudioParameters params; On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > Make it a separate test maybe? Done. https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:765: // that no errors are returned. On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > I'm not sure I'm following. min_params have (media::limits::kMinAudioBufferSize / 2) as a buffer size. Why would we want to create such a stream? I've added a comment here now, I wanted to test that requesting values below the minimum will be clamped to the minimum value. https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:777: OnMoreData(testing::_, testing::_, testing::_, testing::_)) On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > Do we want to test that AudioOutputStream callbacks request exactly the buffer size we asked for when we got it from AudioLatency::GetExactBufferSize()? Yes that would be ideal, I wasn't totally clear on how to accomplish that though, is there another test doing anything similar that I could look at as an example? https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h File media/base/limits.h (right): https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h#new... media/base/limits.h:67: kMaxAudioBufferSize = 4096, On 2017/06/27 at 16:06:48, Raymond Toy (OOO Jun 29-Jul 7) wrote: > Any particular reason why 4096 and not 8192 like you have for the other platforms? > > A comment on how you picked 8192 would be good too. 4096 was just copied from audio_manager_mac.cc, I've added a comment just saying it was taken from there. I'm not sure what the underlying reason for 4096 or 8192 is, these were just the values that were already being used by the different AudioManagers. https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h#new... media/base/limits.h:69: kMinAudioBufferSize = 512, On 2017/06/27 at 16:06:48, Raymond Toy (OOO Jun 29-Jul 7) wrote: > A comment here explaining why the min is 512 would be good. Something as simple as saying that experiments with smaller size frequently produces glitches. Same as above, this is the value that was already in use and I'm not sure what the original reasoning behind it was. I can definitely add a comment if you think that's best but I can't confirm the reasons for these buffer sizes. https://codereview.chromium.org/2908073002/diff/80001/media/base/limits.h#new... media/base/limits.h:72: kMinAudioBufferSize = 256, On 2017/06/27 at 16:06:48, Raymond Toy (OOO Jun 29-Jul 7) wrote: > Likewise a comment on the min value would be good. I've added a short comment here, does that work?
https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn#newcode763 media/BUILD.gn:763: if (is_mac) { We have a base/mac/BUILD.gn this should go in. https://codereview.chromium.org/2908073002/diff/100001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/2908073002/diff/100001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:836: int buffer_size = Might be worth breaking this out of a ternary for clarity. https://codereview.chromium.org/2908073002/diff/100001/media/base/limits.h File media/base/limits.h (right): https://codereview.chromium.org/2908073002/diff/100001/media/base/limits.h#ne... media/base/limits.h:66: // Values taken from AudioManagerMac. This is now the source of these values, so no need to list where they came from. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... File media/base/mac/audio_util_mac.cc (right): https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c) anymore. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... File media/base/mac/audio_util_mac.h (right): https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c) anymore. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.h:13: namespace audio_util_mac { Use internal as the ns if you don't want this to be generally accessible. Or just name the method as GetMinAudioBufferSizeMacOS() for clarity. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.h:15: int MEDIA_EXPORT GetMinAudioBufferSizeForSampleRate(int min_buffer_size, MEDIA_EXPORT int
Thanks dalecurtis@! https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn#newcode763 media/BUILD.gn:763: if (is_mac) { On 2017/06/27 at 17:25:07, DaleCurtis wrote: > We have a base/mac/BUILD.gn this should go in. I had tried this first but it didn't seem to work as AudioLatency depends on audio_util_mac.cc and audio_latency.cc is built into the "shared_memory_support" component in this media/BUILD.gn file, so linking failed without including it here. Does that make sense? https://codereview.chromium.org/2908073002/diff/100001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/2908073002/diff/100001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:836: int buffer_size = On 2017/06/27 at 17:25:08, DaleCurtis wrote: > Might be worth breaking this out of a ternary for clarity. Done. https://codereview.chromium.org/2908073002/diff/100001/media/base/limits.h File media/base/limits.h (right): https://codereview.chromium.org/2908073002/diff/100001/media/base/limits.h#ne... media/base/limits.h:66: // Values taken from AudioManagerMac. On 2017/06/27 at 17:25:08, DaleCurtis wrote: > This is now the source of these values, so no need to list where they came from. Done. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... File media/base/mac/audio_util_mac.cc (right): https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/27 at 17:25:08, DaleCurtis wrote: > No (c) anymore. Done. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... File media/base/mac/audio_util_mac.h (right): https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/27 at 17:25:08, DaleCurtis wrote: > No (c) anymore. Done. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.h:13: namespace audio_util_mac { On 2017/06/27 at 17:25:08, DaleCurtis wrote: > Use internal as the ns if you don't want this to be generally accessible. Or just name the method as GetMinAudioBufferSizeMacOS() for clarity. Done, renamed to GetMinAudioBufferSizeMacOS() and removed the namespace. https://codereview.chromium.org/2908073002/diff/100001/media/base/mac/audio_u... media/base/mac/audio_util_mac.h:15: int MEDIA_EXPORT GetMinAudioBufferSizeForSampleRate(int min_buffer_size, On 2017/06/27 at 17:25:08, DaleCurtis wrote: > MEDIA_EXPORT int Done.
https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn#newcode763 media/BUILD.gn:763: if (is_mac) { On 2017/06/27 at 17:46:49, Andrew MacPherson wrote: > On 2017/06/27 at 17:25:07, DaleCurtis wrote: > > We have a base/mac/BUILD.gn this should go in. > > I had tried this first but it didn't seem to work as AudioLatency depends on audio_util_mac.cc and audio_latency.cc is built into the "shared_memory_support" component in this media/BUILD.gn file, so linking failed without including it here. Does that make sense? Ah I didn't notice this was in shmem support. This seems fine in that case. Leave a comment here about why this is the case. Since we've announced the NaCl deprecation we can hopefully clean this up in a few years. Lets rename the file to audio_latency_mac.cc for clarity though; util classes are too vague.
On 2017/06/27 at 17:54:24, dalecurtis wrote: > https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn > File media/BUILD.gn (right): > > https://codereview.chromium.org/2908073002/diff/100001/media/BUILD.gn#newcode763 > media/BUILD.gn:763: if (is_mac) { > On 2017/06/27 at 17:46:49, Andrew MacPherson wrote: > > On 2017/06/27 at 17:25:07, DaleCurtis wrote: > > > We have a base/mac/BUILD.gn this should go in. > > > > I had tried this first but it didn't seem to work as AudioLatency depends on audio_util_mac.cc and audio_latency.cc is built into the "shared_memory_support" component in this media/BUILD.gn file, so linking failed without including it here. Does that make sense? > > Ah I didn't notice this was in shmem support. This seems fine in that case. Leave a comment here about why this is the case. Since we've announced the NaCl deprecation we can hopefully clean this up in a few years. > > Lets rename the file to audio_latency_mac.cc for clarity though; util classes are too vague. No problem, done now. Thanks again!
dalecurtis@chromium.org changed required reviewers: + olka@chromium.org
media/ lgtm, but defer to olka@ for final approval.
https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/27 17:03:40, Andrew MacPherson wrote: > On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > > The point was to return here exactly the buffer size which will be used on the > browser side, right? But we are always returning 256 here, instead of the real > minimum which is GetMinimumOutputBufferSizePerBoard(). > > > > The current approach is not consistent with what is done for Mac, or am I > missing something? > > I may not have been clear when I asked about this earlier but the intention here > is to return per-board buffer sizes by default from > GetPreferredOutputStreamParameters(), but to allow requesting as low as 256 on > all boards if the user wants to. This does mean that > GetMinimumOutputBufferSizePerBoard() is a confusing name though, maybe it should > be updated to GetPreferredOutputBufferSizePerBoard()? I must be missing something: I don't see how AudioManagerCras::GetPreferredOutputStreamParameters() will allow 256 on 'kevin' board if requested? https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:777: OnMoreData(testing::_, testing::_, testing::_, testing::_)) On 2017/06/27 17:03:40, Andrew MacPherson wrote: > On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > > Do we want to test that AudioOutputStream callbacks request exactly the buffer > size we asked for when we got it from AudioLatency::GetExactBufferSize()? > > Yes that would be ideal, I wasn't totally clear on how to accomplish that > though, is there another test doing anything similar that I could look at as an > example? Are there some problems with checking AudioBus size passed to OnMoreData?
Thanks again olka@! https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latenc... media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/28 at 17:38:22, o1ka wrote: > On 2017/06/27 17:03:40, Andrew MacPherson wrote: > > On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > > > The point was to return here exactly the buffer size which will be used on the > > browser side, right? But we are always returning 256 here, instead of the real > > minimum which is GetMinimumOutputBufferSizePerBoard(). > > > > > > The current approach is not consistent with what is done for Mac, or am I > > missing something? > > > > I may not have been clear when I asked about this earlier but the intention here > > is to return per-board buffer sizes by default from > > GetPreferredOutputStreamParameters(), but to allow requesting as low as 256 on > > all boards if the user wants to. This does mean that > > GetMinimumOutputBufferSizePerBoard() is a confusing name though, maybe it should > > be updated to GetPreferredOutputBufferSizePerBoard()? > > I must be missing something: I don't see how AudioManagerCras::GetPreferredOutputStreamParameters() will allow 256 on 'kevin' board if requested? You're completely right, my mistake. I've updated AudioManagerCras::GetPreferredOutputStreamParameters() to use the kMinAudioBufferSize rather than the board-based minimum, thanks for catching this! https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/2908073002/diff/80001/media/audio/audio_manag... media/audio/audio_manager_unittest.cc:777: OnMoreData(testing::_, testing::_, testing::_, testing::_)) On 2017/06/28 at 17:38:22, o1ka wrote: > On 2017/06/27 17:03:40, Andrew MacPherson wrote: > > On 2017/06/27 at 16:31:19, o1ka OOOJun 23-25 wrote: > > > Do we want to test that AudioOutputStream callbacks request exactly the buffer > > size we asked for when we got it from AudioLatency::GetExactBufferSize()? > > > > Yes that would be ideal, I wasn't totally clear on how to accomplish that > > though, is there another test doing anything similar that I could look at as an > > example? > Are there some problems with checking AudioBus size passed to OnMoreData? That works great, I've updated the test now to do this, does that make more sense?
Looks great! Thank you Andrew. LGTM. https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... media/audio/audio_manager_unittest.cc:725: base::WaitableEvent& event() { return event_; } Would be cleaner to pass a pointer to an event in the constructor instead of providing an accessor to it here, but up to you. https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... media/audio/audio_manager_unittest.cc:731: int expected_frames_per_buffer_; const int https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... media/audio/audio_manager_unittest.cc:784: TEST_F(AudioManagerTest, CheckMinimumAudioBufferSizesCallbacks) { Could you add a comment describing is verified in the test? https://codereview.chromium.org/2908073002/diff/160001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2908073002/diff/160001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:323: std::max(static_cast<int>(limits::kMinAudioBufferSize), rtoy@: Do we need a "buffer size is below recommended" UMA stat to track the usage of this feature? https://codereview.chromium.org/2908073002/diff/160001/media/audio/mac/audio_... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/2908073002/diff/160001/media/audio/mac/audio_... media/audio/mac/audio_manager_mac.cc:844: static_cast<int>(limits::kMinAudioBufferSize))); rtoy@: Same here, do we need a "buffer size is below recommended" UMA?
That's great, thanks olka@! rtoy@ or hongchan@: If either of you have a chance to l-g-t-m the small change (comment only) to AudioDestionation.cpp that would be great! https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... media/audio/audio_manager_unittest.cc:725: base::WaitableEvent& event() { return event_; } On 2017/06/29 at 15:29:58, o1ka wrote: > Would be cleaner to pass a pointer to an event in the constructor instead of providing an accessor to it here, but up to you. Done. https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... media/audio/audio_manager_unittest.cc:731: int expected_frames_per_buffer_; On 2017/06/29 at 15:29:58, o1ka wrote: > const int Done. https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_mana... media/audio/audio_manager_unittest.cc:784: TEST_F(AudioManagerTest, CheckMinimumAudioBufferSizesCallbacks) { On 2017/06/29 at 15:29:58, o1ka wrote: > Could you add a comment describing is verified in the test? Done.
lgtm on webaudio/AudioDestination
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...
Description was changed from ========== Make OS audio buffer size limits visible. Add static methods GetMinimumAudioBufferSize and GetMaximumAudioBufferSize to the AudioManager for use in latencyHint-based buffer size calculations. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Make OS audio buffer size limits visible. Make platform-specific audio buffer size limits visible in limits.h and update AudioLatency::GetExactBufferSize() to allow requesting buffer sizes down to the minimum. Update OSX and CRAS to allow audio buffer sizes below their previous minimums when using GetExactBufferSize(), for use in Web Audio AudioContext creation with a latencyHint. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
andrew.macpherson@soundtrap.com changed reviewers: + peter@chromium.org
Hi peter@, would you be able to take a quick look at this file from the CL, which still needs an OWNER's l-g-t-m? "git cl owners" suggested you as a good candidate: content/shell/renderer/layout_test/layout_test_content_renderer_client.cc Thanks a lot for any help!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
andrew.macpherson@soundtrap.com changed reviewers: + mkwst@chromium.org
Hi mkwst@, would you be able to take a quick look at this one file from the CL, which still needs an OWNER's l-g-t-m? "git cl owners" suggested you as a good candidate: content/shell/renderer/layout_test/layout_test_content_renderer_client.cc Thanks a lot for any help!
//content/shell LGTM, thanks!
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, hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/2908073002/#ps240001 (title: "Refactor and simplify audio_manager_unittest.")
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": 240001, "attempt_start_ts": 1499240752970610,
"parent_rev": "2733ee3f00fbb5201eb94c294bfc913c998401f0", "commit_rev":
"3fe2409358437193a07496c2d0a0b559ef399760"}
Message was sent while issue was closed.
Description was changed from ========== Make OS audio buffer size limits visible. Make platform-specific audio buffer size limits visible in limits.h and update AudioLatency::GetExactBufferSize() to allow requesting buffer sizes down to the minimum. Update OSX and CRAS to allow audio buffer sizes below their previous minimums when using GetExactBufferSize(), for use in Web Audio AudioContext creation with a latencyHint. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Make OS audio buffer size limits visible. Make platform-specific audio buffer size limits visible in limits.h and update AudioLatency::GetExactBufferSize() to allow requesting buffer sizes down to the minimum. Update OSX and CRAS to allow audio buffer sizes below their previous minimums when using GetExactBufferSize(), for use in Web Audio AudioContext creation with a latencyHint. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2908073002 Cr-Commit-Position: refs/heads/master@{#484231} Committed: https://chromium.googlesource.com/chromium/src/+/3fe2409358437193a07496c2d0a0... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/3fe2409358437193a07496c2d0a0...
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2973763002/ by kbr@google.com. The reason for reverting is: New test is asserting on Linux GPU bots (physical machines); see http://crbug.com/708917 . .
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2973773002/ by kbr@chromium.org. The reason for reverting is: The reason for reverting is: New test is asserting on Linux GPU bots (physical machines); see http://crbug.com/708917 ..
andrew.macpherson@soundtrap.com changed reviewers: - peter@chromium.org
Hi dalecurtis@ and olka@: the CL had to be reverted after a failure on one of the physical Linux machines in the new AudioManagerTest.CheckMinimumAudioBufferSizesCallbacks. The problem was when verifying that we can successfully create an output stream with a buffer size of kMaxAudioBufferSize. What appears to be happening is that in pulse_output.cc:136 (where the CHECK fails) we want to create a Pulse buffer of the required size, which in this case is 65536 (8192 frames per buffer * 2 bytes per sample * 4 channels). I am guessing about the 4 channels, not sure if there's any way to verify this. However Pulse internally has a max memory pool block size of 65536 - 64 (size of the block header) = 65472, which is the size returned by pa_stream_begin_write() and reported by the failing test. The easiest fix if this is correct would be to just drop the max buffer size for Pulse (to 4096?). Alternatively the docs for pa_stream_begin_write() indicate that calling code should expect that the requested byte size may be lowered by the call so we would need to add handling for this case. What do you think? Thanks again for the help here!
On 2017/07/06 at 08:06:31, andrew.macpherson wrote: > Hi dalecurtis@ and olka@: the CL had to be reverted after a failure on one of the physical Linux machines in the new AudioManagerTest.CheckMinimumAudioBufferSizesCallbacks. The problem was when verifying that we can successfully create an output stream with a buffer size of kMaxAudioBufferSize. > > What appears to be happening is that in pulse_output.cc:136 (where the CHECK fails) we want to create a Pulse buffer of the required size, which in this case is 65536 (8192 frames per buffer * 2 bytes per sample * 4 channels). I am guessing about the 4 channels, not sure if there's any way to verify this. However Pulse internally has a max memory pool block size of 65536 - 64 (size of the block header) = 65472, which is the size returned by pa_stream_begin_write() and reported by the failing test. > > The easiest fix if this is correct would be to just drop the max buffer size for Pulse (to 4096?). Alternatively the docs for pa_stream_begin_write() indicate that calling code should expect that the requested byte size may be lowered by the call so we would need to add handling for this case. > > What do you think? Thanks again for the help here! Fixing the pulse code is the right thing to do if we want these new buffer sizes to be supported there. On very rare occasion someone hits that check with the current buffer sizes, so that's another reason for fixing the pulse code. I would do that in a separate CL before landing this again though.
On 2017/07/06 at 20:44:22, dalecurtis wrote: > On 2017/07/06 at 08:06:31, andrew.macpherson wrote: > > Hi dalecurtis@ and olka@: the CL had to be reverted after a failure on one of the physical Linux machines in the new AudioManagerTest.CheckMinimumAudioBufferSizesCallbacks. The problem was when verifying that we can successfully create an output stream with a buffer size of kMaxAudioBufferSize. > > > > What appears to be happening is that in pulse_output.cc:136 (where the CHECK fails) we want to create a Pulse buffer of the required size, which in this case is 65536 (8192 frames per buffer * 2 bytes per sample * 4 channels). I am guessing about the 4 channels, not sure if there's any way to verify this. However Pulse internally has a max memory pool block size of 65536 - 64 (size of the block header) = 65472, which is the size returned by pa_stream_begin_write() and reported by the failing test. > > > > The easiest fix if this is correct would be to just drop the max buffer size for Pulse (to 4096?). Alternatively the docs for pa_stream_begin_write() indicate that calling code should expect that the requested byte size may be lowered by the call so we would need to add handling for this case. > > > > What do you think? Thanks again for the help here! > > Fixing the pulse code is the right thing to do if we want these new buffer sizes to be supported there. On very rare occasion someone hits that check with the current buffer sizes, so that's another reason for fixing the pulse code. I would do that in a separate CL before landing this again though. Thanks dalecurtis@! For our case we're only concerned with CRAS and OSX, I had included pulse because it had clear min/max buffer sizes specified in its AudioManager but it would be fine with me to undo this and just return the hardware_buffer_size for pulse as we do for Windows and others in GetExactBufferSize(). I can file an issue around adding pulse support as well but it would be great to be able to land this before the pulse work is done. What do you think?
I think leaving the pulse on alone is fine then.
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...
On 2017/07/06 at 22:05:16, dalecurtis wrote: > I think leaving the pulse on alone is fine then. Sounds great, I've reverted the Pulse-specific work for now and filed https://crbug.com/740054, let me know if you're okay with me re-landing this and thanks again for the help!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/07 10:13:34, Andrew MacPherson wrote: > On 2017/07/06 at 22:05:16, dalecurtis wrote: > > I think leaving the pulse on alone is fine then. > > Sounds great, I've reverted the Pulse-specific work for now and filed > https://crbug.com/740054, let me know if you're okay with me re-landing this and > thanks again for the help! lgtm
The CQ bit was checked by andrew.macpherson@soundtrap.com
The patchset sent to the CQ was uploaded after l-g-t-m from hongchan@chromium.org, mkwst@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2908073002/#ps260001 (title: "Remove pulse-related changes.")
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": 260001, "attempt_start_ts": 1499430521561670,
"parent_rev": "d0f43e95839d59d443be8ad29542c788d5faad13", "commit_rev":
"0a4bc5d41049c519193b922b67777f4efb3870fd"}
Message was sent while issue was closed.
Description was changed from ========== Make OS audio buffer size limits visible. Make platform-specific audio buffer size limits visible in limits.h and update AudioLatency::GetExactBufferSize() to allow requesting buffer sizes down to the minimum. Update OSX and CRAS to allow audio buffer sizes below their previous minimums when using GetExactBufferSize(), for use in Web Audio AudioContext creation with a latencyHint. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2908073002 Cr-Commit-Position: refs/heads/master@{#484231} Committed: https://chromium.googlesource.com/chromium/src/+/3fe2409358437193a07496c2d0a0... ========== to ========== Make OS audio buffer size limits visible. Make platform-specific audio buffer size limits visible in limits.h and update AudioLatency::GetExactBufferSize() to allow requesting buffer sizes down to the minimum. Update OSX and CRAS to allow audio buffer sizes below their previous minimums when using GetExactBufferSize(), for use in Web Audio AudioContext creation with a latencyHint. BUG=708917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2908073002 Cr-Original-Commit-Position: refs/heads/master@{#484231} Committed: https://chromium.googlesource.com/chromium/src/+/3fe2409358437193a07496c2d0a0... Review-Url: https://codereview.chromium.org/2908073002 Cr-Commit-Position: refs/heads/master@{#484895} Committed: https://chromium.googlesource.com/chromium/src/+/0a4bc5d41049c519193b922b6777... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/0a4bc5d41049c519193b922b6777... |
