Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 2908073002: Make OS audio buffer size limits visible. (Closed)

Created:
6 months, 2 weeks ago by Andrew MacPherson
Modified:
5 months, 1 week 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.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+217 lines, -42 lines) Patch
M content/renderer/media/renderer_webaudiodevice_impl.cc View 1 2 1 chunk +3 lines, -6 lines 0 comments Download
M content/shell/renderer/layout_test/layout_test_content_renderer_client.cc View 1 2 1 chunk +3 lines, -5 lines 0 comments Download
M media/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +7 lines, -0 lines 0 comments Download
M media/audio/audio_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +99 lines, -0 lines 0 comments Download
M media/audio/cras/audio_manager_cras.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +5 lines, -7 lines 0 comments Download
M media/audio/mac/audio_manager_mac.cc View 1 2 3 4 5 6 7 5 chunks +16 lines, -21 lines 0 comments Download
M media/base/audio_latency.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +24 lines, -3 lines 0 comments Download
M media/base/limits.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +13 lines, -0 lines 0 comments Download
A media/base/mac/audio_latency_mac.h View 1 2 3 4 5 6 7 1 chunk +18 lines, -0 lines 0 comments Download
A media/base/mac/audio_latency_mac.cc View 1 2 3 4 5 6 7 1 chunk +23 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/audio/AudioDestination.cpp View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 106 (44 generated)
Andrew MacPherson
Hi dalecurtis@, olka@, rtoy@ and hongchan@, This is a proposed fix for the issue discussed ...
6 months, 2 weeks ago (2017-05-29 07:18:00 UTC) #3
o1ka
https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/renderer_webaudiodevice_impl.cc File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/renderer_webaudiodevice_impl.cc#newcode69 content/renderer/media/renderer_webaudiodevice_impl.cc:69: return std::min(media::AudioManager::GetMaximumAudioBufferSize( AudioManager is browser side only entity and ...
6 months, 2 weeks ago (2017-05-29 16:11:33 UTC) #4
o1ka
not lgtm
6 months, 2 weeks ago (2017-05-29 16:22:24 UTC) #5
Andrew MacPherson
On 2017/05/29 at 16:11:33, olka wrote: > https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/renderer_webaudiodevice_impl.cc > File content/renderer/media/renderer_webaudiodevice_impl.cc (right): > > https://codereview.chromium.org/2908073002/diff/1/content/renderer/media/renderer_webaudiodevice_impl.cc#newcode69 ...
6 months, 2 weeks ago (2017-05-30 11:20:31 UTC) #6
o1ka
GetPreferredOutputStreamParameters is written to return an optimal buffer size for WebAudio. And optimal for WebAudio ...
6 months, 2 weeks ago (2017-05-30 16:04:33 UTC) #7
Raymond Toy
https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/renderer_webaudiodevice_impl.cc File content/renderer/media/renderer_webaudiodevice_impl.cc (left): https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/renderer_webaudiodevice_impl.cc#oldcode69 content/renderer/media/renderer_webaudiodevice_impl.cc:69: return std::min(4096, Can't remember the actual value, but make ...
6 months, 2 weeks ago (2017-05-30 18:05:58 UTC) #8
Andrew MacPherson
On 2017/05/30 at 16:04:33, olka wrote: > GetPreferredOutputStreamParameters is written to return an optimal buffer ...
6 months, 2 weeks ago (2017-05-31 07:41:50 UTC) #9
Andrew MacPherson
On 2017/05/30 at 18:05:58, rtoy wrote: > https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/renderer_webaudiodevice_impl.cc > File content/renderer/media/renderer_webaudiodevice_impl.cc (left): > > https://codereview.chromium.org/2908073002/diff/20001/content/renderer/media/renderer_webaudiodevice_impl.cc#oldcode69 ...
6 months, 2 weeks ago (2017-05-31 07:44:11 UTC) #10
olka
(1) > I had intended to put these as constants in limits.h as Dale suggested ...
6 months, 2 weeks ago (2017-05-31 10:43:22 UTC) #11
Andrew MacPherson
On 2017/05/31 at 10:43:22, olka wrote: > (1) > > I had intended to put ...
6 months, 2 weeks ago (2017-05-31 12:48:52 UTC) #12
Raymond Toy
On Wed, May 31, 2017 at 12:41 AM, <andrew.macpherson@soundtrap.com> wrote: > On 2017/05/30 at 16:04:33, ...
6 months, 2 weeks ago (2017-05-31 14:58:13 UTC) #13
o1ka
On 2017/05/31 12:48:52, Andrew MacPherson wrote: > On 2017/05/31 at 10:43:22, olka wrote: > > ...
6 months, 2 weeks ago (2017-06-01 10:46:27 UTC) #14
Andrew MacPherson
On 2017/06/01 at 10:46:27, olka wrote: > Thank you both for clarification! > > So ...
6 months, 2 weeks ago (2017-06-01 12:47:04 UTC) #15
o1ka
It's unfortunate that we have to report actual buffer size to the user. We were ...
6 months, 2 weeks ago (2017-06-02 15:21:13 UTC) #16
Raymond Toy
On 2017/06/01 at 10:46:27, olka wrote: > On 2017/05/31 12:48:52, Andrew MacPherson wrote: > > ...
6 months, 2 weeks ago (2017-06-02 16:42:35 UTC) #17
o1ka
>> Also there is no guarantee that actual HW buffer size remains constant throughout audio ...
6 months, 2 weeks ago (2017-06-02 16:54:26 UTC) #18
DaleCurtis
If we want to modernize this API for mojo, the right thing would be to ...
6 months, 2 weeks ago (2017-06-02 20:38:30 UTC) #19
Andrew MacPherson
On 2017/06/02 at 15:21:13, olka wrote: > I don't see any clean solution to it ...
6 months, 1 week ago (2017-06-03 12:22:11 UTC) #20
o1ka
On 2017/06/02 20:38:30, DaleCurtis wrote: > If we want to modernize this API for mojo, ...
6 months, 1 week ago (2017-06-07 15:37:50 UTC) #21
o1ka
On 2017/06/03 12:22:11, Andrew MacPherson wrote: > On 2017/06/02 at 15:21:13, olka wrote: > > ...
6 months, 1 week ago (2017-06-07 15:54:51 UTC) #22
o1ka
How about modifying AudioManagerMac::GetPreferredOutputStreamParameters() to return 128 as buffer size if |input_parameters| specify it as ...
6 months, 1 week ago (2017-06-08 16:08:27 UTC) #23
Andrew MacPherson
On 2017/06/08 at 16:08:27, olka wrote: > How about modifying AudioManagerMac::GetPreferredOutputStreamParameters() to return 128 as ...
6 months, 1 week ago (2017-06-09 09:22:36 UTC) #24
o1ka
Yes, it's what I meant, but I'm not sure yet how good it is :) ...
6 months, 1 week ago (2017-06-09 10:46:55 UTC) #26
o1ka
https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_manager_mac.cc File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/2908073002/diff/40001/media/audio/mac/audio_manager_mac.cc#newcode836 media/audio/mac/audio_manager_mac.cc:836: if (has_valid_input_params) { combine into a conditional assignment? int ...
6 months, 1 week ago (2017-06-09 10:52:39 UTC) #27
henrika (OOO until Aug 14)
> I couldn't find maximum buffer sizes specified for Windows or Android (see note > ...
6 months, 1 week ago (2017-06-09 11:27:34 UTC) #28
henrika (OOO until Aug 14)
In addition, storing a list of min/max buffer size without unit seems wrong to me. ...
6 months, 1 week ago (2017-06-09 11:36:23 UTC) #29
Andrew MacPherson
On 2017/06/09 at 10:46:55, olka wrote: > Yes, it's what I meant, but I'm not ...
6 months ago (2017-06-12 11:23:33 UTC) #30
Andrew MacPherson
Friendly ping for olka@ re: updated buffer size calc on OSX and henrika@ re: disallowing ...
6 months ago (2017-06-15 08:49:30 UTC) #31
o1ka
Sorry for the delay. Approach SGTM; one question re:ChromeOS. https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc#newcode153 media/base/audio_latency.cc:153: ...
6 months ago (2017-06-15 15:33:37 UTC) #32
hongchan
https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2908073002/diff/60001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode58 third_party/WebKit/Source/platform/audio/AudioDestination.cpp:58: // value and not a buffer size in the ...
6 months ago (2017-06-15 16:22:04 UTC) #33
Raymond Toy
https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc#newcode153 media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/15 15:33:37, o1ka - slow ...
6 months ago (2017-06-15 17:50:06 UTC) #34
Andrew MacPherson
Thanka olka@, rtoy@ and hongchan@! I'll aim to get another patch uploaded this week with ...
5 months, 3 weeks ago (2017-06-19 12:36:19 UTC) #35
Andrew MacPherson
Sorry for the delay here. olka@: I've added a test for this in audio_manager_unittest.cc, I ...
5 months, 2 weeks ago (2017-06-27 08:20:55 UTC) #36
hongchan
Thanks so much for your contribution, Andrew! On Tue, Jun 27, 2017 at 1:20 AM ...
5 months, 2 weeks ago (2017-06-27 15:52:14 UTC) #37
Raymond Toy
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 ...
5 months, 2 weeks ago (2017-06-27 16:06:48 UTC) #38
o1ka
https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc#newcode153 media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; The point was to return here ...
5 months, 2 weeks ago (2017-06-27 16:31:19 UTC) #39
Andrew MacPherson
Thanks olka@ and rtoy@! https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc#newcode153 media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/27 ...
5 months, 2 weeks ago (2017-06-27 17:03:41 UTC) #40
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) { We have a base/mac/BUILD.gn this should ...
5 months, 2 weeks ago (2017-06-27 17:25:08 UTC) #41
Andrew MacPherson
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, ...
5 months, 2 weeks ago (2017-06-27 17:46:50 UTC) #42
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:46:49, Andrew MacPherson ...
5 months, 2 weeks ago (2017-06-27 17:54:24 UTC) #43
Andrew MacPherson
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 ...
5 months, 2 weeks ago (2017-06-27 18:02:35 UTC) #44
DaleCurtis
media/ lgtm, but defer to olka@ for final approval.
5 months, 2 weeks ago (2017-06-27 18:59:57 UTC) #46
o1ka
https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc#newcode153 media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/27 17:03:40, Andrew MacPherson wrote: ...
5 months, 2 weeks ago (2017-06-28 17:38:22 UTC) #47
Andrew MacPherson
Thanks again olka@! https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc File media/base/audio_latency.cc (right): https://codereview.chromium.org/2908073002/diff/60001/media/base/audio_latency.cc#newcode153 media/base/audio_latency.cc:153: minimum_buffer_size = limits::kMinimumOutputBufferSize; On 2017/06/28 at ...
5 months, 2 weeks ago (2017-06-28 18:19:04 UTC) #48
o1ka
Looks great! Thank you Andrew. LGTM. https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_manager_unittest.cc File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/2908073002/diff/160001/media/audio/audio_manager_unittest.cc#newcode725 media/audio/audio_manager_unittest.cc:725: base::WaitableEvent& event() { ...
5 months, 2 weeks ago (2017-06-29 15:29:59 UTC) #49
Andrew MacPherson
That's great, thanks olka@! rtoy@ or hongchan@: If either of you have a chance to ...
5 months, 2 weeks ago (2017-06-29 15:56:37 UTC) #50
hongchan
lgtm on webaudio/AudioDestination
5 months, 2 weeks ago (2017-06-29 16:16:46 UTC) #51
Andrew MacPherson
Hi peter@, would you be able to take a quick look at this file from ...
5 months, 1 week ago (2017-07-03 10:51:46 UTC) #76
Andrew MacPherson
Hi mkwst@, would you be able to take a quick look at this one file ...
5 months, 1 week ago (2017-07-05 07:25:41 UTC) #80
Mike West
//content/shell LGTM, thanks!
5 months, 1 week ago (2017-07-05 07:39:29 UTC) #81
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908073002/240001
5 months, 1 week ago (2017-07-05 07:46:13 UTC) #84
commit-bot: I haz the power
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/3fe2409358437193a07496c2d0a0b559ef399760
5 months, 1 week ago (2017-07-05 09:50:08 UTC) #87
Ken Russell (Google)
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2973763002/ by kbr@google.com. ...
5 months, 1 week ago (2017-07-06 04:44:25 UTC) #88
Ken Russell (switch to Gerrit)
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2973773002/ by kbr@chromium.org. ...
5 months, 1 week ago (2017-07-06 04:49:33 UTC) #89
Andrew MacPherson
Hi dalecurtis@ and olka@: the CL had to be reverted after a failure on one ...
5 months, 1 week ago (2017-07-06 08:06:31 UTC) #91
DaleCurtis
On 2017/07/06 at 08:06:31, andrew.macpherson wrote: > Hi dalecurtis@ and olka@: the CL had to ...
5 months, 1 week ago (2017-07-06 20:44:22 UTC) #92
Andrew MacPherson
On 2017/07/06 at 20:44:22, dalecurtis wrote: > On 2017/07/06 at 08:06:31, andrew.macpherson wrote: > > ...
5 months, 1 week ago (2017-07-06 21:40:58 UTC) #93
DaleCurtis
I think leaving the pulse on alone is fine then.
5 months, 1 week ago (2017-07-06 22:05:16 UTC) #94
Andrew MacPherson
On 2017/07/06 at 22:05:16, dalecurtis wrote: > I think leaving the pulse on alone is ...
5 months, 1 week ago (2017-07-07 10:13:34 UTC) #97
o1ka
On 2017/07/07 10:13:34, Andrew MacPherson wrote: > On 2017/07/06 at 22:05:16, dalecurtis wrote: > > ...
5 months, 1 week ago (2017-07-07 12:27:30 UTC) #100
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908073002/260001
5 months, 1 week ago (2017-07-07 12:28:57 UTC) #103
commit-bot: I haz the power
5 months, 1 week ago (2017-07-07 12:33:25 UTC) #106
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/0a4bc5d41049c519193b922b6777...

Powered by Google App Engine
This is Rietveld 0eb02b776