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

Issue 27605002: Improve and simplify AudioOutputDispatcher. (Closed)

Created:
7 years, 2 months ago by DaleCurtis
Modified:
7 years ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, miu+watch_chromium.org, no longer working on chromium, henrika (OOO until Aug 14)
Visibility:
Public.

Description

Improve and simplify AudioOutputDispatcher. AudioOutputDispatcher does a number of things which no longer make sense in a world where we don't create tons of physical streams: - Streams are not immediately recycled but instead inserted into a paused queue. This was done to improve cycle time when physical streams were rapidly paused and played. - If any stream was paused previously and then closed, every new stream opened will open two streams. Again this was to improve cycle time for paused and played streams. For HTML5, this has been handled by AudioRendererMixer for some time. WebRTC, WebAudio, and PPAPI generally don't start and stop streams repeatedly in a way that this code is beneficial. As such, all of that code has been removed. As a hedge, one stream is left open after Close() until the close timer fires. This allows cycle time to be improved in cases of interleaved Start/Stop cycles. I've additionally cleaned up the code to use std::algorithm where applicable, removed unnecessary WeakPtr setup, and cleaned up the unittests. BUG=313834 TEST=audio plays, unittests pass. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230334 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239647

Patch Set 1 : Remove dummy function. #

Patch Set 2 : Separate MLP changes. #

Total comments: 10

Patch Set 3 : Use vector instead. #

Total comments: 6

Patch Set 4 : Comments. #

Total comments: 11

Patch Set 5 : Remove std::for_each(). #

Patch Set 6 : Rebase. Cleanup. #

Total comments: 6

Patch Set 7 : Rebase. Comments. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+156 lines, -277 lines) Patch
M media/audio/audio_output_dispatcher.h View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M media/audio/audio_output_dispatcher_impl.h View 1 2 3 4 5 6 4 chunks +18 lines, -22 lines 0 comments Download
M media/audio/audio_output_dispatcher_impl.cc View 1 2 3 4 5 6 5 chunks +38 lines, -80 lines 4 comments Download
M media/audio/audio_output_proxy_unittest.cc View 1 2 3 4 5 6 27 chunks +100 lines, -173 lines 0 comments Download

Messages

Total messages: 40 (0 generated)
DaleCurtis
henrika: Please review from a WebRTC perspective. I think my assumptions are true, but let ...
7 years, 2 months ago (2013-10-17 00:35:24 UTC) #1
henrika (OOO until Aug 14)
+xians Dale, I have to admit that I have never touched this code or worked ...
7 years, 2 months ago (2013-10-17 08:55:35 UTC) #2
scherkus (not reviewing)
can you split cleanup (e.g., message loop changes) into a separate CLs?
7 years, 2 months ago (2013-10-17 17:22:30 UTC) #3
DaleCurtis
You mean the unittest changes or the BelongsToCurrentThread() ones? The unittest ones are a packaged ...
7 years, 2 months ago (2013-10-17 18:28:57 UTC) #4
scherkus (not reviewing)
On 2013/10/17 18:28:57, DaleCurtis wrote: > You mean the unittest changes or the BelongsToCurrentThread() ones? ...
7 years, 2 months ago (2013-10-17 18:40:44 UTC) #5
DaleCurtis
Thanks for the info Henrik. henrika -> cc, +miu for review. Separated the MLP changes ...
7 years, 2 months ago (2013-10-17 18:57:00 UTC) #6
scherkus (not reviewing)
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc#newcode117 media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( do you have a strong desire to use ...
7 years, 2 months ago (2013-10-17 19:19:53 UTC) #7
DaleCurtis
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc#newcode117 media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( On 2013/10/17 19:19:53, scherkus wrote: > do you ...
7 years, 2 months ago (2013-10-17 21:11:54 UTC) #8
scherkus (not reviewing)
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc#newcode117 media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( On 2013/10/17 21:11:55, DaleCurtis wrote: > On 2013/10/17 ...
7 years, 2 months ago (2013-10-17 23:01:47 UTC) #9
DaleCurtis
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.cc#newcode117 media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( On 2013/10/17 23:01:47, scherkus wrote: > On 2013/10/17 ...
7 years, 2 months ago (2013-10-17 23:09:10 UTC) #10
scherkus (not reviewing)
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.h File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.h#newcode74 media/audio/audio_output_dispatcher_impl.h:74: typedef std::deque<AudioOutputStream*> AudioOutputStreamList; On 2013/10/17 23:09:10, DaleCurtis wrote: > ...
7 years, 2 months ago (2013-10-17 23:25:05 UTC) #11
DaleCurtis
Any comments on the dispatcher feature-level changes? :) https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.h File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_dispatcher_impl.h#newcode74 media/audio/audio_output_dispatcher_impl.h:74: typedef ...
7 years, 2 months ago (2013-10-17 23:33:55 UTC) #12
scherkus (not reviewing)
can you reupload rebased ontop of the MLP CL?
7 years, 2 months ago (2013-10-17 23:36:48 UTC) #13
DaleCurtis
Hmm? The patchset you're commenting on is already rebased.
7 years, 2 months ago (2013-10-17 23:40:41 UTC) #14
DaleCurtis
Whoops, looks like the vector one got messed up. Hold on.
7 years, 2 months ago (2013-10-17 23:41:01 UTC) #15
DaleCurtis
Fixed, sorry!
7 years, 2 months ago (2013-10-17 23:41:33 UTC) #16
scherkus (not reviewing)
lgtm https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc#newcode29 media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, OOC is AOD as an interface ...
7 years, 2 months ago (2013-10-18 00:01:19 UTC) #17
DaleCurtis
https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc#newcode29 media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, On 2013/10/18 00:01:19, scherkus wrote: > OOC ...
7 years, 2 months ago (2013-10-18 00:07:59 UTC) #18
scherkus (not reviewing)
https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc#newcode29 media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, On 2013/10/18 00:08:00, DaleCurtis wrote: > On ...
7 years, 2 months ago (2013-10-18 00:15:05 UTC) #19
DaleCurtis
https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_dispatcher_impl.cc#newcode29 media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, On 2013/10/18 00:15:05, scherkus wrote: > On ...
7 years, 2 months ago (2013-10-18 00:23:20 UTC) #20
miu
https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc#newcode47 media/audio/audio_output_dispatcher_impl.cc:47: idle_proxies_++; Can we just increment idle_proxies_ after the if-statement, ...
7 years, 2 months ago (2013-10-21 23:44:36 UTC) #21
DaleCurtis
https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc#newcode47 media/audio/audio_output_dispatcher_impl.cc:47: idle_proxies_++; On 2013/10/21 23:44:36, Yuri wrote: > Can we ...
7 years, 2 months ago (2013-10-22 20:51:40 UTC) #22
miu
lgtm Nice work! https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc#newcode112 media/audio/audio_output_dispatcher_impl.cc:112: idle_proxies_--; On 2013/10/22 20:51:40, DaleCurtis wrote: ...
7 years, 2 months ago (2013-10-22 21:45:29 UTC) #23
DaleCurtis
Thanks for reviews! https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_dispatcher_impl.cc#newcode112 media/audio/audio_output_dispatcher_impl.cc:112: idle_proxies_--; On 2013/10/22 21:45:29, Yuri wrote: ...
7 years, 2 months ago (2013-10-22 21:52:58 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/27605002/101001
7 years, 2 months ago (2013-10-22 21:59:25 UTC) #25
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 2 months ago (2013-10-22 23:19:46 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/27605002/101001
7 years, 2 months ago (2013-10-22 23:32:17 UTC) #27
commit-bot: I haz the power
Change committed as 230334
7 years, 2 months ago (2013-10-23 05:26:45 UTC) #28
DaleCurtis
Reverted due to potentially exposing WASAPI issues, http://crbug.com/310838
7 years, 2 months ago (2013-10-23 23:07:08 UTC) #29
DaleCurtis
Thinking about this a bit more: We may not want to save a stream open ...
7 years, 2 months ago (2013-10-24 21:58:07 UTC) #30
DaleCurtis
scherkus: PTAL. Trying to reland this now that I've fixed some WASAPI issues. (Patch set ...
7 years ago (2013-12-04 22:00:22 UTC) #31
DaleCurtis
Actually, I just realized the existing dispatcher has the same problem because it also keeps ...
7 years ago (2013-12-06 21:22:16 UTC) #32
scherkus (not reviewing)
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc#newcode88 media/audio/audio_output_dispatcher_impl.cc:88: idle_streams_.push_back(physical_stream); what's the difference between idle_streams_ and idle_proxies_? looking ...
7 years ago (2013-12-06 23:04:13 UTC) #33
DaleCurtis
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc#newcode88 media/audio/audio_output_dispatcher_impl.cc:88: idle_streams_.push_back(physical_stream); On 2013/12/06 23:04:14, scherkus wrote: > what's the ...
7 years ago (2013-12-07 00:33:32 UTC) #34
scherkus (not reviewing)
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc#newcode88 media/audio/audio_output_dispatcher_impl.cc:88: idle_streams_.push_back(physical_stream); On 2013/12/07 00:33:33, DaleCurtis wrote: > On 2013/12/06 ...
7 years ago (2013-12-09 18:53:12 UTC) #35
DaleCurtis
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output_dispatcher_impl.cc#newcode88 media/audio/audio_output_dispatcher_impl.cc:88: idle_streams_.push_back(physical_stream); On 2013/12/09 18:53:12, scherkus wrote: > On 2013/12/07 ...
7 years ago (2013-12-09 19:16:35 UTC) #36
scherkus (not reviewing)
lgtm w/ nit and thought cleaning up these APIs would be great. for example it ...
7 years ago (2013-12-09 23:48:03 UTC) #37
DaleCurtis
https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output_dispatcher_impl.cc File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output_dispatcher_impl.cc#newcode111 media/audio/audio_output_dispatcher_impl.cc:111: CloseIdleStreams(std::max(idle_proxies_, static_cast<size_t>(1))); On 2013/12/09 23:48:03, scherkus wrote: > I ...
7 years ago (2013-12-10 00:02:30 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/27605002/551001
7 years ago (2013-12-10 00:37:36 UTC) #39
commit-bot: I haz the power
7 years ago (2013-12-10 04:34:21 UTC) #40
Message was sent while issue was closed.
Change committed as 239647

Powered by Google App Engine
This is Rietveld 408576698