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

Issue 236123002: Allow pass through buffer sizes on OSX. (Closed)

Created:
6 years, 8 months ago by DaleCurtis
Modified:
6 years, 8 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Allow pass through buffer sizes on OSX. Previously we've prevented streams from being created with arbitrary buffer sizes since one size is shared across all input and output streams in the process. As of a few months ago the input and output paths now both have FIFO queues to overcome circumstances where the buffer size requested from OSX doesn't match what it actually asks for. With the FIFOs in place, it's no longer necessary for us to ensure a single buffer size is used across the process. What is instead done is to only raise the buffer size if an input or output stream is the sole active output stream. As measured by coreaudiod on my MacBook Pro this results in a savings of 1 - 5% of CPU! Going from 3->5% total to 0.3%. BUG=362261 TEST=Extensive manual testing. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264741

Patch Set 1 : Fix comments. #

Total comments: 11

Patch Set 2 : Comments. Rebase. #

Total comments: 2

Patch Set 3 : Comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+80 lines, -43 lines) Patch
M media/audio/audio_manager_base.h View 2 chunks +4 lines, -4 lines 0 comments Download
M media/audio/mac/audio_auhal_mac.cc View 1 2 2 chunks +33 lines, -18 lines 0 comments Download
M media/audio/mac/audio_low_latency_input_mac.cc View 1 2 2 chunks +25 lines, -13 lines 0 comments Download
M media/audio/mac/audio_manager_mac.cc View 1 2 4 chunks +16 lines, -6 lines 0 comments Download
M media/base/audio_hardware_config.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M media/base/audio_hardware_config_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 10 (0 generated)
DaleCurtis
6 years, 8 months ago (2014-04-16 00:14:46 UTC) #1
no longer working on chromium
https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc#newcode516 media/audio/mac/audio_auhal_mac.cc:516: result = AudioUnitGetProperty(audio_unit_, curiously, what will the value of ...
6 years, 8 months ago (2014-04-16 14:57:11 UTC) #2
DaleCurtis
https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc#newcode516 media/audio/mac/audio_auhal_mac.cc:516: result = AudioUnitGetProperty(audio_unit_, On 2014/04/16 14:57:11, xians1 wrote: > ...
6 years, 8 months ago (2014-04-16 22:56:02 UTC) #3
henrika (OOO until Aug 14)
LGTM to ensure that I don't block any work. I will revisit next week and ...
6 years, 8 months ago (2014-04-17 09:38:30 UTC) #4
no longer working on chromium
lgtm with some nits. https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc#newcode511 media/audio/mac/audio_auhal_mac.cc:511: // WARNING: Setting this value ...
6 years, 8 months ago (2014-04-17 10:25:46 UTC) #5
DaleCurtis
https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc File media/audio/mac/audio_auhal_mac.cc (right): https://codereview.chromium.org/236123002/diff/80001/media/audio/mac/audio_auhal_mac.cc#newcode511 media/audio/mac/audio_auhal_mac.cc:511: // WARNING: Setting this value changes the frame size ...
6 years, 8 months ago (2014-04-17 20:50:29 UTC) #6
DaleCurtis
The CQ bit was checked by dalecurtis@chromium.org
6 years, 8 months ago (2014-04-17 21:15:32 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/236123002/120001
6 years, 8 months ago (2014-04-17 21:18:20 UTC) #8
commit-bot: I haz the power
Change committed as 264741
6 years, 8 months ago (2014-04-18 05:23:50 UTC) #9
henrika (OOO until Aug 14)
6 years, 8 months ago (2014-04-22 07:39:27 UTC) #10
Message was sent while issue was closed.
Checked again. LGTM++

Powered by Google App Engine
This is Rietveld 408576698