Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11882010:
Clean-up and bug fixes in AudioOutputController: (Closed)
|
Created:
7 years, 11 months ago by miu Modified:
7 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClean-up and bug fixes in AudioOutputController:
1. In debug builds, explicitly enforce the contract with AudioOutputStream implementations: OnMoreIOData() may only be called after stream_->Start() and not after stream_->Stop().
2. Guarantee one call to stream_->Stop() for each call to stream_->Start(). Removed the complex kPausedWhenPlaying state handling since we can easily cancel the polling starts.
3. Remove blocking call to DoStopCloseAndClearStream() in destructor since the DCHECK is sufficient to guarantee Close() has been called.
4. Add const qualifier to members that never change (e.g., pointers to "injected" objects), and remove useless NULL DCHECKs.
BUG=120973, 168902
TEST=unittests; manual "trying to break it" in a browser; re-ran test.html from http://crbug.com/111272 to confirm no regression of a prior bug
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177051
Patch Set 1 #Patch Set 2 : Minor tweaks. #Patch Set 3 : Removed complex kPausedWhenStarting state, instead using weak ptr invalidation. #
Total comments: 19
Patch Set 4 : Moar clean-ups, addressing reviewers' comments. #Patch Set 5 : Better comments, simplify is_recreating bool; per crogers@. #
Total comments: 2
Patch Set 6 : Remove all the NDEBUG. #
Messages
Total messages: 17 (0 generated)
Chris and Tommi, I'd appreciate two eyes on this to make sure I got this right. Thanks, Yuri
lgtm where 'g' stands for great! https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:211: if (state_ == kPlaying) { nit: else? https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:212: DCHECK(stream_); no need for dcheck https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:320: if (state_ == kStarting) This looks like the same code block as in DoPause(). Is it worth pulling this out into a separate function that's called from here and DoPause()? https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller_unittest.cc (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller_unittest.cc:191: .Times(AtLeast(1)); Can we remove the AtLeast part now and just leave the 1?
lgtm assuming previous bugs are still fixed. https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (left): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:164: // the corresponding play event has already occurred. Can you backtrace to the bug this code fixed and make sure the issue is still fixed? https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:10: #include "base/synchronization/waitable_event.h" Cleanup header list.
Tommi/Dale, Thanks for the fast turnaround on this review. I've applied changes per all your suggestions. Dale mentioned an earlier bug (111272). I re-ran the test.html found there to confirm no regression of the "play after pause" bug. Thanks, Yuri https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (left): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:164: // the corresponding play event has already occurred. On 2013/01/15 19:24:47, DaleCurtis wrote: > Can you backtrace to the bug this code fixed and make sure the issue is still > fixed? Done. I re-ran the test.html and there's no regression on that bug. :) https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:10: #include "base/synchronization/waitable_event.h" On 2013/01/15 19:24:47, DaleCurtis wrote: > Cleanup header list. Done. I also noticed that DoStopCloseAndClearStream() no longer needs the "done signal" argument, so I cleaned that up. https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:211: if (state_ == kPlaying) { On 2013/01/15 11:30:54, tommi wrote: > nit: else? Done. https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:212: DCHECK(stream_); On 2013/01/15 11:30:54, tommi wrote: > no need for dcheck Done. https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:320: if (state_ == kStarting) On 2013/01/15 11:30:54, tommi wrote: > This looks like the same code block as in DoPause(). Is it worth pulling this > out into a separate function that's called from here and DoPause()? Done. Added a StopStream() function. https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.h:37: // .-----------------------> [ Closed / Error ] <------. Note: I updated the state diagram here. https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller_unittest.cc (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller_unittest.cc:191: .Times(AtLeast(1)); On 2013/01/15 11:30:54, tommi wrote: > Can we remove the AtLeast part now and just leave the 1? Done.
Thanks Yuri, looks good! I've added a couple of bike-shedding comments. https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:345: is_recreating_ = true; Managing |is_recreating_| as a member variable seems a bit awkward. Can't we instead simply add a single bool argument to DoCreate()? https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.h:211: #ifdef NDEBUG Not sure how the others feel, but it seems a bid fiddly to wrap this in NDEBUG since it makes the code harder to read and the extra sanity checking code is tiny in this case. I'd suggest always compiling it. ditto with other uses of NDEBUG https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.h:243: // Number of threads that are allowed inside the OnMoreIOData() callback. "Number of threads that are allowed inside the OnMoreIOData() callback" is a strange wording. Can we make the comment more precise and descriptive.
https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.h:211: #ifdef NDEBUG On 2013/01/15 20:38:42, Chris Rogers wrote: > Not sure how the others feel, but it seems a bid fiddly to wrap this in NDEBUG > since it makes the code harder to read and the extra sanity checking code is > tiny in this case. I'd suggest always compiling it. > > ditto with other uses of NDEBUG I'm not a fan of NDEBUG, but I suspect this will lead to some CHECK failures in the field => revert. So it might be best to remove NDEBUG in a secondary CL once the bots have absorbed this change successfully.
Thanks, Chris (comments below). Dale/Chris: Sounds like removing the NDEBUGs as a separate CL would be fine by me. I'm wondering, however, if this should always remain DCHECK-style (rather than CHECK-style)? Meaning, in release builds, do we really want to crash the browser process over this? Right now, *if* there are faulty platform implementations of AudioOutputStream, we're not seeing any harmful effects from them in the field (that I'm aware of). https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.cc:345: is_recreating_ = true; On 2013/01/15 20:38:42, Chris Rogers wrote: > Managing |is_recreating_| as a member variable seems a bit awkward. Can't we > instead simply add a single bool argument to DoCreate()? Done. Good call! :) https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... media/audio/audio_output_controller.h:243: // Number of threads that are allowed inside the OnMoreIOData() callback. On 2013/01/15 20:38:42, Chris Rogers wrote: > "Number of threads that are allowed inside the OnMoreIOData() callback" is a > strange wording. Can we make the comment more precise and descriptive. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11882010/15001
https://codereview.chromium.org/11882010/diff/15001/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11882010/diff/15001/media/audio/audio_output_... media/audio/audio_output_controller.h:231: // OnMoreIOData() method, and only when it is valid to do so. This is for Is this really totally accurate? Isn't it still possible for multiple threads to call OnMoreIOData() if the times do not overlap? I thought this was more of a check for re-entrancy (on the same thread)... Were we actually seeing a real-world case were this was happening? - just interested in what motivated you to add this particular check.
https://codereview.chromium.org/11882010/diff/15001/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11882010/diff/15001/media/audio/audio_output_... media/audio/audio_output_controller.h:231: // OnMoreIOData() method, and only when it is valid to do so. This is for On 2013/01/15 22:12:41, Chris Rogers wrote: > Is this really totally accurate? Isn't it still possible for multiple threads > to call OnMoreIOData() if the times do not overlap? Yes, it's possible. It doesn't matter, though. The method is invoked by platform-specific implementations of AudioOutputStream. They are allowed to do whatever they want, within the following contract: 1. OnMoreIOData() is not reentrant. 2. OnMoreIOData() is not to be called by more than one thread at a time. 3. OnMoreIOData() may not be called before Start() and not after Stop(). #3 was the main motivation for this change (bug 168902). > Were we actually seeing a real-world case were this was happening? - just > interested in what motivated you to add this particular check. I haven't seen a violation of this, but just last week I fixed a bug in code very closely related to this (in Android's impl of AudioOutputStream). Also, through multiple code reviews now, Dale has reminded me that AOC::OnMoreIOData() is only invoked under the circumstances listed above. Since I was cleaning up the controller, it seemed to make sense to add some code that enforces the contract, at least in Debug builds. This can be a non-obvious detail to those not intimately familiar with this part of the code base, especially those trying to launch a new platform in Chrome.
On 2013/01/15 22:27:10, Yuri wrote: > https://codereview.chromium.org/11882010/diff/15001/media/audio/audio_output_... > File media/audio/audio_output_controller.h (right): > > https://codereview.chromium.org/11882010/diff/15001/media/audio/audio_output_... > media/audio/audio_output_controller.h:231: // OnMoreIOData() method, and only > when it is valid to do so. This is for > On 2013/01/15 22:12:41, Chris Rogers wrote: > > Is this really totally accurate? Isn't it still possible for multiple threads > > to call OnMoreIOData() if the times do not overlap? > > Yes, it's possible. It doesn't matter, though. The method is invoked by > platform-specific implementations of AudioOutputStream. They are allowed to do > whatever they want, within the following contract: > > 1. OnMoreIOData() is not reentrant. > 2. OnMoreIOData() is not to be called by more than one thread at a time. > 3. OnMoreIOData() may not be called before Start() and not after Stop(). > > #3 was the main motivation for this change (bug 168902). > > > Were we actually seeing a real-world case were this was happening? - just > > interested in what motivated you to add this particular check. > > I haven't seen a violation of this, but just last week I fixed a bug in code > very closely related to this (in Android's impl of AudioOutputStream). > > Also, through multiple code reviews now, Dale has reminded me that > AOC::OnMoreIOData() is only invoked under the circumstances listed above. Since > I was cleaning up the controller, it seemed to make sense to add some code that > enforces the contract, at least in Debug builds. This can be a non-obvious > detail to those not intimately familiar with this part of the code base, > especially those trying to launch a new platform in Chrome. Yuri, thanks for the explanation. To really check that condition (2) is good, the current code isn't really sufficient since multiple threads can still call OnMoreIOData(). I just want to make sure we don't get too confident and overstate how much this protects us in that case. Maybe the comment can focus more on the (1) and (3) cases.
On 2013/01/15 21:10:29, DaleCurtis wrote: > https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... > File media/audio/audio_output_controller.h (right): > > https://codereview.chromium.org/11882010/diff/6001/media/audio/audio_output_c... > media/audio/audio_output_controller.h:211: #ifdef NDEBUG > On 2013/01/15 20:38:42, Chris Rogers wrote: > > Not sure how the others feel, but it seems a bid fiddly to wrap this in NDEBUG > > since it makes the code harder to read and the extra sanity checking code is > > tiny in this case. I'd suggest always compiling it. > > > > ditto with other uses of NDEBUG > > I'm not a fan of NDEBUG, but I suspect this will lead to some CHECK failures in > the field => revert. So it might be best to remove NDEBUG in a secondary CL once > the bots have absorbed this change successfully. Then I don't understand why is there a CHECK inside of a #ifndef NDEBUG? Can't we just change it to a DCHECK and remove the compile-time stuff safely now?
On 2013/01/15 22:45:36, Chris Rogers wrote: > Can't we just change it to a DCHECK and remove the compile-time stuff safely now? Done. LGTY?
On 2013/01/15 23:03:52, Yuri wrote: > On 2013/01/15 22:45:36, Chris Rogers wrote: > > Can't we just change it to a DCHECK and remove the compile-time stuff safely > now? > > Done. LGTY? Thanks Yuri, lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11882010/21002
Thanks for reviewing, Chris. I missed your previous comment about condition (2): On 2013/01/15 22:42:16, Chris Rogers wrote: > On 2013/01/15 22:27:10, Yuri wrote: > > 1. OnMoreIOData() is not reentrant. > > 2. OnMoreIOData() is not to be called by more than one thread at a time. > > 3. OnMoreIOData() may not be called before Start() and not after Stop(). > > Yuri, thanks for the explanation. To really check that condition (2) is good, > the current code isn't really sufficient since multiple threads can still call > OnMoreIOData(). To avoid confusion: Yes, multiple threads can call OnMoreIOData(). However, multiple threads cannot call OnMoreIOData() simultaneously. The calls to Disallow()/Allow() at the beginning/end of OnMoreIOData() ensure this. Really, we're splitting hairs here. ;-) It'd be awfully strange for a platform audio implementation to have multiple threads simultaneously pulling audio data via a single callback.
Message was sent while issue was closed.
Change committed as 177051 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
