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

Issue 2908073002: Make OS audio buffer size limits visible.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 3 days ago by Andrew MacPherson
Modified:
3 days, 6 hours 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. 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

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: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+127 lines, -50 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 1 chunk +6 lines, -0 lines 0 comments Download
M media/audio/cras/audio_manager_cras.cc View 1 2 3 3 chunks +2 lines, -6 lines 0 comments Download
M media/audio/mac/audio_manager_mac.cc View 1 2 3 5 chunks +17 lines, -24 lines 0 comments Download
M media/audio/pulse/audio_manager_pulse.cc View 1 2 3 4 chunks +3 lines, -6 lines 0 comments Download
M media/base/audio_latency.cc View 1 2 3 2 chunks +24 lines, -3 lines 2 comments Download
M media/base/limits.h View 1 2 3 2 chunks +14 lines, -0 lines 0 comments Download
A media/base/mac/audio_util_mac.h View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A media/base/mac/audio_util_mac.cc View 1 2 3 1 chunk +27 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/audio/AudioDestination.cpp View 1 2 1 chunk +6 lines, -0 lines 4 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 35 (3 generated)
Andrew MacPherson
Hi dalecurtis@, olka@, rtoy@ and hongchan@, This is a proposed fix for the issue discussed ...
3 weeks, 3 days ago (2017-05-29 07:18:00 UTC) #3
o1ka OOOJun 23-25
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 ...
3 weeks, 3 days ago (2017-05-29 16:11:33 UTC) #4
o1ka OOOJun 23-25
not lgtm
3 weeks, 3 days 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 ...
3 weeks, 2 days ago (2017-05-30 11:20:31 UTC) #6
o1ka OOOJun 23-25
GetPreferredOutputStreamParameters is written to return an optimal buffer size for WebAudio. And optimal for WebAudio ...
3 weeks, 2 days ago (2017-05-30 16:04:33 UTC) #7
Raymond Toy (OOO Jun 19-22)
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 ...
3 weeks, 2 days 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 ...
3 weeks, 1 day 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 ...
3 weeks, 1 day 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 ...
3 weeks, 1 day 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 ...
3 weeks, 1 day ago (2017-05-31 12:48:52 UTC) #12
Raymond Toy (OOO Jun 19-22)
On Wed, May 31, 2017 at 12:41 AM, <andrew.macpherson@soundtrap.com> wrote: > On 2017/05/30 at 16:04:33, ...
3 weeks, 1 day ago (2017-05-31 14:58:13 UTC) #13
o1ka OOOJun 23-25
On 2017/05/31 12:48:52, Andrew MacPherson wrote: > On 2017/05/31 at 10:43:22, olka wrote: > > ...
3 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 ...
3 weeks ago (2017-06-01 12:47:04 UTC) #15
o1ka OOOJun 23-25
It's unfortunate that we have to report actual buffer size to the user. We were ...
2 weeks, 6 days ago (2017-06-02 15:21:13 UTC) #16
Raymond Toy (OOO Jun 19-22)
On 2017/06/01 at 10:46:27, olka wrote: > On 2017/05/31 12:48:52, Andrew MacPherson wrote: > > ...
2 weeks, 6 days ago (2017-06-02 16:42:35 UTC) #17
o1ka OOOJun 23-25
>> Also there is no guarantee that actual HW buffer size remains constant throughout audio ...
2 weeks, 6 days 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 ...
2 weeks, 5 days 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 ...
2 weeks, 5 days ago (2017-06-03 12:22:11 UTC) #20
o1ka OOOJun 23-25
On 2017/06/02 20:38:30, DaleCurtis wrote: > If we want to modernize this API for mojo, ...
2 weeks, 1 day ago (2017-06-07 15:37:50 UTC) #21
o1ka OOOJun 23-25
On 2017/06/03 12:22:11, Andrew MacPherson wrote: > On 2017/06/02 at 15:21:13, olka wrote: > > ...
2 weeks, 1 day ago (2017-06-07 15:54:51 UTC) #22
o1ka OOOJun 23-25
How about modifying AudioManagerMac::GetPreferredOutputStreamParameters() to return 128 as buffer size if |input_parameters| specify it as ...
2 weeks 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 ...
1 week, 6 days ago (2017-06-09 09:22:36 UTC) #24
o1ka OOOJun 23-25
Yes, it's what I meant, but I'm not sure yet how good it is :) ...
1 week, 6 days ago (2017-06-09 10:46:55 UTC) #26
o1ka OOOJun 23-25
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 ...
1 week, 6 days ago (2017-06-09 10:52:39 UTC) #27
henrika
> I couldn't find maximum buffer sizes specified for Windows or Android (see note > ...
1 week, 6 days ago (2017-06-09 11:27:34 UTC) #28
henrika
In addition, storing a list of min/max buffer size without unit seems wrong to me. ...
1 week, 6 days 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 ...
1 week, 3 days 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 ...
1 week ago (2017-06-15 08:49:30 UTC) #31
o1ka OOOJun 23-25
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: ...
1 week ago (2017-06-15 15:33:37 UTC) #32
hongchan (OOO Jun 19-22)
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 ...
1 week ago (2017-06-15 16:22:04 UTC) #33
Raymond Toy (OOO Jun 19-22)
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 ...
1 week ago (2017-06-15 17:50:06 UTC) #34
Andrew MacPherson
3 days, 6 hours ago (2017-06-19 12:36:19 UTC) #35
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@.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318