|
|
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) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImprove 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
Messages
Total messages: 40 (0 generated)
henrika: Please review from a WebRTC perspective. I think my assumptions are true, but let me know if not :) scherkus: Please review with an eye for historical context. I've tried to go back through the relevant CLs to make sure I'm not breaking anything: Initial AudioOutputProxy implementation: https://codereview.chromium.org/5158003/ Updated implementation for AudioOutputMixer: https://codereview.chromium.org/9691001/ Introduction of the pause_delay_: https://codereview.chromium.org/6822019
+xians Dale, I have to admit that I have never touched this code or worked with anything which, to my knowledge, has been affected by it. However, I can verify that WebRTC does not do many repeated Start/Stop calls and any cleanup in this area is good stuff. And it will not affect us in a bad way. Let me know if you would like me to review the CL more or less from a "black box perspective". I can do the best I can to come up with generic comments but might not be able to detect potential design flaws.
can you split cleanup (e.g., message loop changes) into a separate CLs?
You mean the unittest changes or the BelongsToCurrentThread() ones? The unittest ones are a packaged deal.
On 2013/10/17 18:28:57, DaleCurtis wrote: > You mean the unittest changes or the BelongsToCurrentThread() ones? The > unittest ones are a packaged deal. the switch from MessageLoop to MessageLoopProxy + corresponding dchecks also it's unclear from your CL description whether the std::algorithm changes are drive-by cleanup or actually part of this CL
Thanks for the info Henrik. henrika -> cc, +miu for review. Separated the MLP changes out. The algorithm changes don't make sense to separate most of the old code is deleted.
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( do you have a strong desire to use std::for_each()? It's not very prevalent in Chromium. We typically stick w/ plain old C++ for loops. https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:74: typedef std::deque<AudioOutputStream*> AudioOutputStreamList; OOC why the change from std::list to std::deque? I didn't see it mentioned in the CL description.
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( On 2013/10/17 19:19:53, scherkus wrote: > do you have a strong desire to use std::for_each()? It's not very prevalent in > Chromium. We typically stick w/ plain old C++ for loops. I found this reasoning behind avoiding "raw loops" pretty sound: http://channel9.msdn.com/Events/GoingNative/2013/Cpp-Seasoning I find it easier to parse as well as being less lines of code. https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:74: typedef std::deque<AudioOutputStream*> AudioOutputStreamList; On 2013/10/17 19:19:53, scherkus wrote: > OOC why the change from std::list to std::deque? I didn't see it mentioned in > the CL description. We're always popping elements off the back or erasing a couple of elements from the front (but could also from the back). At most there will be 50 elements (due to stream cap), on average probably 1 to 5. We don't need any of the iterator safety... Hmmm, as I wrote this I realized we can just use vector. We don't even need any of the fancy deque abilities either.
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( On 2013/10/17 21:11:55, DaleCurtis wrote: > On 2013/10/17 19:19:53, scherkus wrote: > > do you have a strong desire to use std::for_each()? It's not very prevalent in > > Chromium. We typically stick w/ plain old C++ for loops. > > I found this reasoning behind avoiding "raw loops" pretty sound: > http://channel9.msdn.com/Events/GoingNative/2013/Cpp-Seasoning had a sneaking suspicion -- I watched it too :) > I find it easier to parse as well as being less lines of code. do you plan on converting for loops to for_each() in all your future CLs? i.e., is this something we'll be seeing more of? https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:74: typedef std::deque<AudioOutputStream*> AudioOutputStreamList; On 2013/10/17 21:11:55, DaleCurtis wrote: > On 2013/10/17 19:19:53, scherkus wrote: > > OOC why the change from std::list to std::deque? I didn't see it mentioned in > > the CL description. > > We're always popping elements off the back or erasing a couple of elements from > the front (but could also from the back). At most there will be 50 elements > (due to stream cap), on average probably 1 to 5. We don't need any of the > iterator safety... > > Hmmm, as I wrote this I realized we can just use vector. We don't even need any > of the fancy deque abilities either. > deque will still have an advantage for exactly the reasons you describe (appending to end, removing N-1 elements from the front in CloseStream()). it'll make smaller allocations when push_back()'ing as well as make less moves when erase()'ing
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( On 2013/10/17 23:01:47, scherkus wrote: > On 2013/10/17 21:11:55, DaleCurtis wrote: > > On 2013/10/17 19:19:53, scherkus wrote: > > > do you have a strong desire to use std::for_each()? It's not very prevalent > in > > > Chromium. We typically stick w/ plain old C++ for loops. > > > > I found this reasoning behind avoiding "raw loops" pretty sound: > > http://channel9.msdn.com/Events/GoingNative/2013/Cpp-Seasoning > > had a sneaking suspicion -- I watched it too :) > > > I find it easier to parse as well as being less lines of code. > > do you plan on converting for loops to for_each() in all your future CLs? i.e., > is this something we'll be seeing more of? Where it makes sense yes. I'm on the fence about the utility of doing a wholesale pass through media/. It's probably useful and I can file a bug for it if you agree. https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:74: typedef std::deque<AudioOutputStream*> AudioOutputStreamList; On 2013/10/17 23:01:47, scherkus wrote: > On 2013/10/17 21:11:55, DaleCurtis wrote: > > On 2013/10/17 19:19:53, scherkus wrote: > > > OOC why the change from std::list to std::deque? I didn't see it mentioned > in > > > the CL description. > > > > We're always popping elements off the back or erasing a couple of elements > from > > the front (but could also from the back). At most there will be 50 elements > > (due to stream cap), on average probably 1 to 5. We don't need any of the > > iterator safety... > > > > Hmmm, as I wrote this I realized we can just use vector. We don't even need > any > > of the fancy deque abilities either. > > > > deque will still have an advantage for exactly the reasons you describe > (appending to end, removing N-1 elements from the front in CloseStream()). it'll > make smaller allocations when push_back()'ing as well as make less moves when > erase()'ing I changed the CloseStream() call to remove the last N-1 elements, so we are now only accessing the back of the structure, which obviates any of the deque advantages. Otherwise, vector and deque both have amortized constant time push_back() AFAIK, are you saying this is not the case? I was also under the impression vector and deque had similar allocation strategies. Do you have a source for "it'll make smaller allocations when push_back()'ing"
https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:74: typedef std::deque<AudioOutputStream*> AudioOutputStreamList; On 2013/10/17 23:09:10, DaleCurtis wrote: > On 2013/10/17 23:01:47, scherkus wrote: > > On 2013/10/17 21:11:55, DaleCurtis wrote: > > > On 2013/10/17 19:19:53, scherkus wrote: > > > > OOC why the change from std::list to std::deque? I didn't see it mentioned > > in > > > > the CL description. > > > > > > We're always popping elements off the back or erasing a couple of elements > > from > > > the front (but could also from the back). At most there will be 50 elements > > > (due to stream cap), on average probably 1 to 5. We don't need any of the > > > iterator safety... > > > > > > Hmmm, as I wrote this I realized we can just use vector. We don't even need > > any > > > of the fancy deque abilities either. > > > > > > > deque will still have an advantage for exactly the reasons you describe > > (appending to end, removing N-1 elements from the front in CloseStream()). > it'll > > make smaller allocations when push_back()'ing as well as make less moves when > > erase()'ing > > I changed the CloseStream() call to remove the last N-1 elements, so we are now > only accessing the back of the structure, which obviates any of the deque > advantages. > > Otherwise, vector and deque both have amortized constant time push_back() AFAIK, > are you saying this is not the case? > > I was also under the impression vector and deque had similar allocation > strategies. Do you have a source for "it'll > make smaller allocations when push_back()'ing" the biggest difference between vectors and deques is that unlike vector, deque doesn't guarantee the underlying memory for the structure is contiguous deques are typically implemented as a list of vectors. if you push_back() and capacity needs to be increased, a small vector is allocated and linked to the rest of the vectors (compared to vector which would double in size and copy all elements) same benefit happens when removing from the front: only the first vector in the list of vectors is modified http://www.cplusplus.com/reference/deque/deque/ and http://stackoverflow.com/questions/6292332/what-really-is-a-deque-in-stl are have good summaries
Any comments on the dispatcher feature-level changes? :) https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/15001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:74: typedef std::deque<AudioOutputStream*> AudioOutputStreamList; On 2013/10/17 23:25:05, scherkus wrote: > On 2013/10/17 23:09:10, DaleCurtis wrote: > > On 2013/10/17 23:01:47, scherkus wrote: > > > On 2013/10/17 21:11:55, DaleCurtis wrote: > > > > On 2013/10/17 19:19:53, scherkus wrote: > > > > > OOC why the change from std::list to std::deque? I didn't see it > mentioned > > > in > > > > > the CL description. > > > > > > > > We're always popping elements off the back or erasing a couple of elements > > > from > > > > the front (but could also from the back). At most there will be 50 > elements > > > > (due to stream cap), on average probably 1 to 5. We don't need any of the > > > > iterator safety... > > > > > > > > Hmmm, as I wrote this I realized we can just use vector. We don't even > need > > > any > > > > of the fancy deque abilities either. > > > > > > > > > > deque will still have an advantage for exactly the reasons you describe > > > (appending to end, removing N-1 elements from the front in CloseStream()). > > it'll > > > make smaller allocations when push_back()'ing as well as make less moves > when > > > erase()'ing > > > > I changed the CloseStream() call to remove the last N-1 elements, so we are > now > > only accessing the back of the structure, which obviates any of the deque > > advantages. > > > > Otherwise, vector and deque both have amortized constant time push_back() > AFAIK, > > are you saying this is not the case? > > > > I was also under the impression vector and deque had similar allocation > > strategies. Do you have a source for "it'll > > make smaller allocations when push_back()'ing" > > the biggest difference between vectors and deques is that unlike vector, deque > doesn't guarantee the underlying memory for the structure is contiguous > > deques are typically implemented as a list of vectors. if you push_back() and > capacity needs to be increased, a small vector is allocated and linked to the > rest of the vectors (compared to vector which would double in size and copy all > elements) > > same benefit happens when removing from the front: only the first vector in the > list of vectors is modified > > http://www.cplusplus.com/reference/deque/deque/ and > http://stackoverflow.com/questions/6292332/what-really-is-a-deque-in-stl are > have good summaries Thanks for the links. vector will be fine for <50 elements.
can you reupload rebased ontop of the MLP CL?
Hmm? The patchset you're commenting on is already rebased.
Whoops, looks like the vector one got messed up. Hold on.
Fixed, sorry!
lgtm https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, OOC is AOD as an interface / base class providing much value? https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:77: // Used to post delayed tasks to ourselves that we cancel inside Shutdown(). was this comment referring to weak_this_ or close_timer_? seems like it needs some updatin'
https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, On 2013/10/18 00:01:19, scherkus wrote: > OOC is AOD as an interface / base class providing much value? Kind of, definitely for unittests, by virtue of AudioOutputResampler and AudioOutputDispatcherImpl sharing an interface we get a lot of test reuse. We also switch between implementations inside of the AudioManager. https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:77: // Used to post delayed tasks to ourselves that we cancel inside Shutdown(). On 2013/10/18 00:01:19, scherkus wrote: > was this comment referring to weak_this_ or close_timer_? seems like it needs > some updatin' Done.
https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, On 2013/10/18 00:08:00, DaleCurtis wrote: > On 2013/10/18 00:01:19, scherkus wrote: > > OOC is AOD as an interface / base class providing much value? > > Kind of, definitely for unittests, by virtue of AudioOutputResampler and > AudioOutputDispatcherImpl sharing an interface we get a lot of test reuse. We > also switch between implementations inside of the AudioManager. Gotcha. Other Q's: 1) What about making it not a base class? 2) What does AODImpl actually do these days? e.g. could it be renamed AudioOutputStreamCacheAndAutoCloser?
https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/25001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:29: : AudioOutputDispatcher(audio_manager, On 2013/10/18 00:15:05, scherkus wrote: > On 2013/10/18 00:08:00, DaleCurtis wrote: > > On 2013/10/18 00:01:19, scherkus wrote: > > > OOC is AOD as an interface / base class providing much value? > > > > Kind of, definitely for unittests, by virtue of AudioOutputResampler and > > AudioOutputDispatcherImpl sharing an interface we get a lot of test reuse. We > > also switch between implementations inside of the AudioManager. > > Gotcha. > > Other Q's: > 1) What about making it not a base class? > 2) What does AODImpl actually do these days? e.g. could it be renamed > AudioOutputStreamCacheAndAutoCloser? 1) Sounds fine, AOD base only assigns some variables and does some message loop checks. 2. AudioOutputCache sounds fine and fits with AudioOutputResampler I can do these in follow up CLs.
https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:47: idle_proxies_++; Can we just increment idle_proxies_ after the if-statement, rather than increment it and decrement back on failure? https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:112: idle_proxies_--; It doesn't look like idle_proxies_ is used for anything. Can we get rid of it? Otherwise: 1. Is it enough to decrement by one here? What if more than one idle stream is closed below? 2. Is it ever true that idle_proxies_ is not exactly idle_streams_.size() after an operation? Can we just get rid of it? https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( My two cents on the use of std::for_each() here: IMHO, for_each() is not a good general replacement when you cannot use C++11 language features. Especially with Chromium style: You're forced to define the "helper code" far away from the rest of the functionality. Here, CloseStreamHelper() is defined at the top of the file, about 100 lines away from the CloseStream() method itself. That makes it harder to read code and understand code dependencies. Sean Parent's "raw loops" tech talk promotes the use of lambdas and auto typing to solve the "distant helper code" problem, but we cannot yet use these in Chromium code. :( So, it's up to you, but I think the old code was pretty clean and concise, modified to be: while (idle_streams_.size() > 1) { idle_streams_.back()->Close(); idle_streams_.pop_back(); } Also, consider Sean's major points on raw loops: That they are bad because it's dangerous to place non-O(1) operations within loops, that code is otherwise difficult to "prove" functionally, and that we should promote building a library for code reuse by others. Do any of these points apply here? https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:73: size_t idle_proxies_; C++ style: Should be of type 'int' (yes, even if it's always supposed to be a non-negative integer). http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege...
https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:47: idle_proxies_++; On 2013/10/21 23:44:36, Yuri wrote: > Can we just increment idle_proxies_ after the if-statement, rather than > increment it and decrement back on failure? Done. https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:112: idle_proxies_--; On 2013/10/21 23:44:36, Yuri wrote: > It doesn't look like idle_proxies_ is used for anything. Can we get rid of it? > Otherwise: > > 1. Is it enough to decrement by one here? What if more than one idle stream is > closed below? > > 2. Is it ever true that idle_proxies_ is not exactly idle_streams_.size() after > an operation? Can we just get rid of it? > idle_proxies_ will always be greater than idle_streams_. There may be only a single stream which is shared between any number of idle proxies. I think it's useful in terms of debugging. You're right though that the check is currently missing. I've added a DCHECK during destruction. https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:117: std::for_each( On 2013/10/21 23:44:36, Yuri wrote: > My two cents on the use of std::for_each() here: IMHO, for_each() is not a good > general replacement when you cannot use C++11 language features. Especially > with Chromium style: You're forced to define the "helper code" far away from the > rest of the functionality. Here, CloseStreamHelper() is defined at the top of > the file, about 100 lines away from the CloseStream() method itself. That makes > it harder to read code and understand code dependencies. > > Sean Parent's "raw loops" tech talk promotes the use of lambdas and auto typing > to solve the "distant helper code" problem, but we cannot yet use these in > Chromium code. :( > > So, it's up to you, but I think the old code was pretty clean and concise, > modified to be: > > while (idle_streams_.size() > 1) { > idle_streams_.back()->Close(); > idle_streams_.pop_back(); > } > > Also, consider Sean's major points on raw loops: That they are bad because it's > dangerous to place non-O(1) operations within loops, that code is otherwise > difficult to "prove" functionally, and that we should promote building a library > for code reuse by others. Do any of these points apply here? I played with this a few different ways and eventually concluded it is easier to read w/o std::for_each in this case. My main motivation for changing this was to avoid the repeated pop_back() in favor of a single erase(). So I've isolated that change instead. https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:73: size_t idle_proxies_; On 2013/10/21 23:44:36, Yuri wrote: > C++ style: Should be of type 'int' (yes, even if it's always supposed to be a > non-negative integer). > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... Superseded by http://www.chromium.org/developers/coding-style#TOC-Types since it's an object count.
lgtm Nice work! https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:112: idle_proxies_--; On 2013/10/22 20:51:40, DaleCurtis wrote: > On 2013/10/21 23:44:36, Yuri wrote: > > It doesn't look like idle_proxies_ is used for anything. Can we get rid of > it? > > Otherwise: > > > > 1. Is it enough to decrement by one here? What if more than one idle stream > is > > closed below? > > > > 2. Is it ever true that idle_proxies_ is not exactly idle_streams_.size() > after > > an operation? Can we just get rid of it? > > > > idle_proxies_ will always be greater than idle_streams_. There may be only a > single stream which is shared between any number of idle proxies. > > I think it's useful in terms of debugging. You're right though that the check > is currently missing. I've added a DCHECK during destruction. Okay, so it's just for debug builds. Just a thought, but don't waste the time unless you're bought into this: Everywhere idle_proxies_ is incremented/decremented, you could wrap it inside a DCHECK. That way, it would be entirely compiled out of release builds. Example: DCHECK_GT(idle_proxies_--, 0u); https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.h:73: size_t idle_proxies_; On 2013/10/22 20:51:40, DaleCurtis wrote: > On 2013/10/21 23:44:36, Yuri wrote: > > C++ style: Should be of type 'int' (yes, even if it's always supposed to be a > > non-negative integer). > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... > > Superseded by http://www.chromium.org/developers/coding-style#TOC-Types since > it's an object count. Ah! Never mind, then. I'll have to re-read the Chromium "style overrides" again so they'll sink in.
Thanks for reviews! https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/37001/media/audio/audio_output_... media/audio/audio_output_dispatcher_impl.cc:112: idle_proxies_--; On 2013/10/22 21:45:29, Yuri wrote: > On 2013/10/22 20:51:40, DaleCurtis wrote: > > On 2013/10/21 23:44:36, Yuri wrote: > > > It doesn't look like idle_proxies_ is used for anything. Can we get rid of > > it? > > > Otherwise: > > > > > > 1. Is it enough to decrement by one here? What if more than one idle stream > > is > > > closed below? > > > > > > 2. Is it ever true that idle_proxies_ is not exactly idle_streams_.size() > > after > > > an operation? Can we just get rid of it? > > > > > > > idle_proxies_ will always be greater than idle_streams_. There may be only a > > single stream which is shared between any number of idle proxies. > > > > I think it's useful in terms of debugging. You're right though that the check > > is currently missing. I've added a DCHECK during destruction. > > Okay, so it's just for debug builds. > > Just a thought, but don't waste the time unless you're bought into this: > Everywhere idle_proxies_ is incremented/decremented, you could wrap it inside a > DCHECK. That way, it would be entirely compiled out of release builds. > > Example: > > DCHECK_GT(idle_proxies_--, 0u); Hmm, a nice thought, but I think it makes things a bit more confusing and potentially awkward if later someone comes along and mucks with it outside of a DCHECK() or changes something to a CHECK().
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/27605002/101001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/27605002/101001
Message was sent while issue was closed.
Change committed as 230334
Reverted due to potentially exposing WASAPI issues, http://crbug.com/310838
Thinking about this a bit more: We may not want to save a stream open after Close() since that stream may have been shutdown due to an error. I'll see if its worth adding error detection to the AudioOutputProxy so only known-good streams are kept alive.
scherkus: PTAL. Trying to reland this now that I've fixed some WASAPI issues. (Patch set #5 was what was previously committed). New patch is rebased on top of the hang fix and a couple other patches, so it required some minor work. I've also simplified the ClosePendingStreams() path to make it easier for logging to be injected in a subsequent patch. As far as concerns with streams with errors, I think we should wait and see if they're worth worrying about as the UMA stat shows only 0.02% of HTML5 playbacks result in an error being triggered. Previously a stream which encountered an error ended up in pausing_streams_ when stopped. then closed with all pausing and idle streams in the immediate close call. Since all streams were closed without any loop pump in between, it wasn't possible for the stream to be reused. Now, the path is similar, but the keep alive aspect may result in a stream which resulted in error being reused which may lead to stream creation failure on Start(), which in turn triggers OnError() again... retrying after the close timer fired, would fix this, but it would janky if it occurs.
Actually, I just realized the existing dispatcher has the same problem because it also keeps streams alive if any stream is paused, so error'd streams aren't an issue; or at least not a new issue.
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... 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 at the code ... it seems any time idle_streams_.push_back() is called we increment idle_proxies_ and also looks like we try to decrement idle_proxies_ any time we close + idle_streams_.pop_back() https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... media/audio/audio_output_dispatcher_impl.h:57: // Closes all |idle_streams_| incomplete comment and also doesn't really line up with what the method is named (method name is singular operation yet comment hints at being a plural operation)
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... 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 difference between idle_streams_ and idle_proxies_? > > looking at the code ... it seems any time idle_streams_.push_back() is called we > increment idle_proxies_ and also looks like we try to decrement idle_proxies_ > any time we close + idle_streams_.pop_back() idle_streams_ may be empty when idle_proxies_ > 0. Consider when the close timer fires and kills all idle streams, it doesn't change the number of outstanding idle_proxies_. https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.h (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... media/audio/audio_output_dispatcher_impl.h:57: // Closes all |idle_streams_| On 2013/12/06 23:04:14, scherkus wrote: > incomplete comment and also doesn't really line up with what the method is named > (method name is singular operation yet comment hints at being a plural > operation) Whoops! Fixed.
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... 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 23:04:14, scherkus wrote: > > what's the difference between idle_streams_ and idle_proxies_? > > > > looking at the code ... it seems any time idle_streams_.push_back() is called > we > > increment idle_proxies_ and also looks like we try to decrement idle_proxies_ > > any time we close + idle_streams_.pop_back() > > idle_streams_ may be empty when idle_proxies_ > 0. Consider when the close > timer fires and kills all idle streams, it doesn't change the number of > outstanding idle_proxies_. I suppose I don't understand what an idle_proxy_ is supposed to represent or what it's tracking, which makes it hard to figure out what's going on. Is it supposed to refer to AudioOutputProxy or some other proxy-like object? For example we increment idle_proxies_ in OpenStream() but there's no proxy-like object in sight. Then in CloseStream() we pass in a proxy object but don't do anything with it, but we decrement idle_proxies_ by one, then close a bunch of idle_streams_. Can you provide some more details?
https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/531001/media/audio/audio_output... 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 00:33:33, DaleCurtis wrote: > > On 2013/12/06 23:04:14, scherkus wrote: > > > what's the difference between idle_streams_ and idle_proxies_? > > > > > > looking at the code ... it seems any time idle_streams_.push_back() is > called > > we > > > increment idle_proxies_ and also looks like we try to decrement > idle_proxies_ > > > any time we close + idle_streams_.pop_back() > > > > idle_streams_ may be empty when idle_proxies_ > 0. Consider when the close > > timer fires and kills all idle streams, it doesn't change the number of > > outstanding idle_proxies_. > > I suppose I don't understand what an idle_proxy_ is supposed to represent or > what it's tracking, which makes it hard to figure out what's going on. > > Is it supposed to refer to AudioOutputProxy or some other proxy-like object? > > For example we increment idle_proxies_ in OpenStream() but there's no proxy-like > object in sight. Then in CloseStream() we pass in a proxy object but don't do > anything with it, but we decrement idle_proxies_ by one, then close a bunch of > idle_streams_. > > Can you provide some more details? An idle proxy is an AudioOutputProxy object in the kOpened state (see AOP header); which correlates to immediately after AudioOutput(Proxy|Stream)::Open() or AudioOutput(Proxy|Stream)::Stop(). OpenStream() and CloseStream() are really only used as hints as to when the |idle_streams_| pool should be increased or decreased. Yes, it's bad that OpenStream() and CloseStream() are not symmetric in terms of parameters. I want to fix that; just not in this CL. The parameter should be removed from CloseStream(), but AudioOutputResampler is currently using it to share OnMoreDataConverter() allocations, so it's a bit more than a simple deletion.
lgtm w/ nit and thought cleaning up these APIs would be great. for example it still terrifies me that each AudioOutputStream implementation has to call audio_manager_->ReleaseOutputStream(this) inside of Close(), which really just deletes |this| it makes reading AudioOutputDispatcherImpl code a bit odd since we have containers of raw AOS* pointers that appear to be unowned https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output... media/audio/audio_output_dispatcher_impl.cc:111: CloseIdleStreams(std::max(idle_proxies_, static_cast<size_t>(1))); I forget ... does 1u not work here? https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output... media/audio/audio_output_dispatcher_impl.cc:144: void AudioOutputDispatcherImpl::CloseIdleStreams(size_t keep_alive) { food for thought ... this seems like a strange API because keep_alive can't be honoured all the time (e.g., I pass in 1 to keep alive but there's 0 idle streams), then on top of that we add CloseAllIdleStreams() perhaps CloseStream() could a bit more work so we can tighten up the Close{All}IdleStreams() API ... for example what if we only offered CloseAllIdleStreams()? then CloseStream() could inspect the relative sizes of idle_proxies_ and idle_streams_ and make a judgement call.
https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output... File media/audio/audio_output_dispatcher_impl.cc (right): https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output... 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 forget ... does 1u not work here? I seem to remember some of the bots (windows probably) freaking out about 1ul vs 1u when comparing size_t. https://codereview.chromium.org/27605002/diff/551001/media/audio/audio_output... media/audio/audio_output_dispatcher_impl.cc:144: void AudioOutputDispatcherImpl::CloseIdleStreams(size_t keep_alive) { On 2013/12/09 23:48:03, scherkus wrote: > food for thought ... this seems like a strange API because keep_alive can't be > honoured all the time (e.g., I pass in 1 to keep alive but there's 0 idle > streams), then on top of that we add CloseAllIdleStreams() > > perhaps CloseStream() could a bit more work so we can tighten up the > Close{All}IdleStreams() API ... for example what if we only offered > CloseAllIdleStreams()? then CloseStream() could inspect the relative sizes of > idle_proxies_ and idle_streams_ and make a judgement call. Just having CloseAllIdleStreams() isn't sufficient since the decision isn't whether or not to keep all idle streams. Instead the decision is how many of the idle streams should be kept alive. CloseStream() would have to call Close() explicitly rather than using a helper function, which is counter to one of my goals here. A goal here was to have all stream Close() calls happen in a single function so logging is easy (followup CL). Since they're private methods I don't think it's a big deal that the API is a little strange.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/27605002/551001
Message was sent while issue was closed.
Change committed as 239647 |