Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1435293003:
Improved error handling in iOS ADM to avoid race during init (Closed)
|
Created:
5 years, 1 month ago by henrika_webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImproved error handling in iOS ADM to avoid race during init
BUG=webrtc:5166
R=pbos@webrtc.org, tkchin@webrtc.org
Committed: https://crrev.com/34911ad55c4c4c549fe60e1b4cc127420b15666b
Cr-Commit-Position: refs/heads/master@{#10728}
Patch Set 1 #Patch Set 2 : Fine tuned audio session deactivation scheme #
Total comments: 10
Patch Set 3 : Feedback from tkchin@ #Patch Set 4 : Adding rtc::GlobalLockPod and counts number of active audio sessions at Start/Stop #
Total comments: 14
Patch Set 5 : Feedback from pbos@ #
Total comments: 25
Patch Set 6 : More feedback from pbos #Patch Set 7 : Stability improvements #
Total comments: 8
Patch Set 8 : nits #
Messages
Total messages: 35 (6 generated)
Description was changed from ========== Improved error handling in iOS ADM to avoid race during init BUG= ========== to ========== Improved error handling in iOS ADM to avoid race during init BUG=webrtc:5166 ==========
henrika@webrtc.org changed reviewers: + tkchin@webrtc.org
PTAL
lgtm https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.h (right): https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:217: static volatile int object_count_; nit: Add comment for what this is used for and why it has to be static https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:85: static bool ActivateAudioSession(AVAudioSession* session, bool activate) { Would this be suitable for the new rtc::Optional thing? Just a question, not really related to this CL. https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:218: const int count = rtc::AtomicOps::Increment(&object_count_); Is AudioDeviceIOS always created from the same thread? Otherwise wouldn't you need to lock? https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:901: if ((result = AudioComponentInstanceDispose(vpio_unit_)) != noErr) { Why not just: OSStatus result = udioComponentInstanceDispose(vpio_unit_); if (result != noError) { }
Thanks! https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.h (right): https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:217: static volatile int object_count_; On 2015/11/16 21:30:08, tkchin_webrtc wrote: > nit: Add comment for what this is used for and why it has to be static Done. https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:85: static bool ActivateAudioSession(AVAudioSession* session, bool activate) { Not sure if I understand. How would it improve the existing implementation? It is a three-state bool, right? https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:218: const int count = rtc::AtomicOps::Increment(&object_count_); If I make an AppRTC call, we see: (audio_device_ios.mm:227): AudioDeviceIOS::ctor[1]<NSThread: 0x124d5d570>{number = 3, name = (null)} [009:727] [26115] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[1] [042:415] [25375] (audio_device_ios.mm:227): AudioDeviceIOS::ctor[1]<NSThread: 0x124d38dd0>{number = 6, name = (null)} [043:831] [61191] (audio_device_ios.mm:227): AudioDeviceIOS::ctor[2]<NSThread: 0x128b2ebc0>{number = 7, name = (null)} [128:947] [61191] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[2] [129:053] [25375] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[1] hence, it can be created on more than one thread but it is always destructed on same as created. Anyow, it does not feel OK with a lock in ctor/dtor since the only usage of this counter is to disable the audio session when last object dies. Let's open up a separate discussion if there are potential improvements in this area? https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:901: if ((result = AudioComponentInstanceDispose(vpio_unit_)) != noErr) { On 2015/11/16 21:30:08, tkchin_webrtc wrote: > Why not just: > OSStatus result = udioComponentInstanceDispose(vpio_unit_); > if (result != noError) { > } Done.
I would like to make more improvements here related to how and when to activate the audio session and make it thread safe. Let me come back with one more round on this one.
https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:218: const int count = rtc::AtomicOps::Increment(&object_count_); On 2015/11/17 14:46:11, henrika_webrtc wrote: > If I make an AppRTC call, we see: > > (audio_device_ios.mm:227): AudioDeviceIOS::ctor[1]<NSThread: 0x124d5d570>{number > = 3, name = (null)} > [009:727] [26115] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[1] > [042:415] [25375] (audio_device_ios.mm:227): AudioDeviceIOS::ctor[1]<NSThread: > 0x124d38dd0>{number = 6, name = (null)} > [043:831] [61191] (audio_device_ios.mm:227): AudioDeviceIOS::ctor[2]<NSThread: > 0x128b2ebc0>{number = 7, name = (null)} > [128:947] [61191] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[2] > [129:053] [25375] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[1] > > hence, it can be created on more than one thread but it is always destructed on > same as created. > > Anyow, it does not feel OK with a lock in ctor/dtor since the only usage of this > counter is to disable the audio session when last object dies. > > Let's open up a separate discussion if there are potential improvements in this > area? So this count can be potentially inaccurate then. Because it's possible to read stale value. Unless I'm misunderstanding something? You can potentially use main queue to guarantee correct order. Do dispatch_async(dispatch_get_main_queue(), ^{ <incr/decr> <log> }); Since it's a serial execution queue you won't even need to atomic or volatile bits.
henrika@webrtc.org changed reviewers: + pbos@webrtc.org
Adding pbos@ for an upcoming thread safe implementation using global locks. https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:218: const int count = rtc::AtomicOps::Increment(&object_count_); Thanks, I am learning something new every day and dispatch queues was news to me ;-) I was actually very close to an alternative solution (after discussions with pbos@) using a combo of global lock and avoid counting objetcs but instead keep track of the actual number of activations. Can most likely use your idea as well but the pattern is new to me. Let me see if I can come up with two alternative solutions.
pbos@: I have added a solution that we have discussed off-line. Please check it out. tkchin@ has suggested an iOS/ObjC-specific method to serialize access to shared objects that might actually simplify the code. Would still appreciate your view on this round. Thanks.
tkchin@: given the latest proposal where we add counters in combination with activate/deactivate of the audio session (instead of in the ctor/dtor), I find it difficult to use tasks and async dispatch queues. I tried it but it is messy to use an async approach and I actually find the existing proposal more readable. If you feel strongly about using dispatch queues, please comment in the latest patch (and if possible) add proposal on how to use it. I can't come up with a clean way of using queues here. pbos@: PTAL
https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.h (right): https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:223: static int audio_session_activation_count_; Put GUARDED_BY(lock_) between "count_" and ";" https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:28: // Initialization of static members. See audio_device_ios.h for details. Feel free to remove this comment (or put why it's there). I think the info in here is implied. https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:87: static bool ActivateAudioSession(AVAudioSession* session, bool activate) { EXCLUSIVE_LOCKS_REQUIRED(lock_) (before {) https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:862: return false; LOG with LS_ERROR here? https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:883: bool AudioDeviceIOS::ShutdownPlayOrRecord() { Should this one be bool? There's no sensible way to handle a shutdown error here is there? https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:909: if (audio_session_activation_count_ == 1) { pref: --audio_session_activation_count_ == 0 (and remove the -- below), leaving the state in count=1 when there's no user doesn't make much sense either right? https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:911: if (!ActivateAudioSession(session, false)) { I'm wondering whether this should be a CHECK, or at least log a LS_ERROR.
On 2015/11/18 12:54:22, pbos-webrtc wrote: > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/ios/audio_device_ios.h (right): > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.h:223: static int > audio_session_activation_count_; > Put GUARDED_BY(lock_) between "count_" and ";" > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.mm:28: // Initialization of > static members. See audio_device_ios.h for details. > Feel free to remove this comment (or put why it's there). I think the info in > here is implied. > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.mm:87: static bool > ActivateAudioSession(AVAudioSession* session, bool activate) { > EXCLUSIVE_LOCKS_REQUIRED(lock_) (before {) > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.mm:862: return false; > LOG with LS_ERROR here? > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.mm:883: bool > AudioDeviceIOS::ShutdownPlayOrRecord() { > Should this one be bool? There's no sensible way to handle a shutdown error here > is there? > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.mm:909: if > (audio_session_activation_count_ == 1) { > pref: --audio_session_activation_count_ == 0 (and remove the -- below), leaving > the state in count=1 when there's no user doesn't make much sense either right? > > https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.mm:911: if > (!ActivateAudioSession(session, false)) { > I'm wondering whether this should be a CHECK, or at least log a LS_ERROR. These are in webrtc/base/thread_annotations.h btw.
Thanks Peter. Done changes as proposed and fixed some other issues as well. PTAL https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.h (right): https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:223: static int audio_session_activation_count_; On 2015/11/18 12:54:22, pbos-webrtc wrote: > Put GUARDED_BY(lock_) between "count_" and ";" Done. https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:28: // Initialization of static members. See audio_device_ios.h for details. Removed. https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:87: static bool ActivateAudioSession(AVAudioSession* session, bool activate) { Can't do that actually since this is not a member method. https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:862: return false; On 2015/11/18 12:54:22, pbos-webrtc wrote: > LOG with LS_ERROR here? Done. https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:883: bool AudioDeviceIOS::ShutdownPlayOrRecord() { No good way to handle false result here. Tried it but void is better IMHO. https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:911: if (!ActivateAudioSession(session, false)) { We have strived to avoid crashing the app in these cases for this class. Will add LS_ERROR.
https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:909: if (audio_session_activation_count_ == 1) { Solved in a different manner. Hope you are OK with it.
Executed media_unittests --gtest_filter=AudioDevice* --gtest_repeat=50 and also did 10 manual calls using AppRTCDemo (P2P, only trying to connect and in loopback). Worked well.
https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.h (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:16: #include "webrtc/base/criticalsection.h" Move to .cc file https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:18: #include "webrtc/base/thread_annotations.h" Move to .cc file https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:463: bool AudioDeviceIOS::ActivateAudioSession() { These aren't accessing any members right? This could be static and put outside the class along with the other stuff, correct? https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:495: if (!webrtc::ActivateAudioSession(session, false)) { You don't need the webrtc:: prefix here or above. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:496: LOG(LS_ERROR) << "Failed to deactivate the audio session"; + ", this shouldn't happen, the audio session will now leak." https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:910: // Activate our own audio session if not already activated. -our own, it makes it sound like we own it. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:922: // TODO(henrika): remove CHECK when we are sure that we no longer see Are you sure? Seems like a good CHECK to keep. Maybe move the CHECK here into the function where it can actually fail, then make the function void? https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:933: if (nullptr != vpio_unit_) { Did you wanna abort early if this wasn't the case? Right now if activation has failed you still call DeactivateAudioSession if anyone calls this, right?
https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:474: AVAudioSession* session = [AVAudioSession sharedInstance]; I don't know if there are consequences to managing AVAudioSession from non-main thread. If it works then all well and good. But there are no docs related to thread-safety that I can find. Application devs may be calling AVAudioSession methods in the application from the main thread. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:496: LOG(LS_ERROR) << "Failed to deactivate the audio session"; On 2015/11/18 18:51:48, pbos-webrtc wrote: > + ", this shouldn't happen, the audio session will now leak." It will "leak" in the sense that there is mismatched activate/deactivate, but activate/deactivate don't deal with object lifetime to my knowledge. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:922: // TODO(henrika): remove CHECK when we are sure that we no longer see On 2015/11/18 18:51:48, pbos-webrtc wrote: > Are you sure? Seems like a good CHECK to keep. Maybe move the CHECK here into > the function where it can actually fail, then make the function void? Any of the iOS audio APIs can fail. Crashing due to system API failure is undesirable.
Comments only. Will make final changes tomorrow. Feels like we are in phase and that only minor changes remains. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:463: bool AudioDeviceIOS::ActivateAudioSession() { Missed that. Will fix. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:474: AVAudioSession* session = [AVAudioSession sharedInstance]; We have done so for a long time now, also done in other clients using the old code base for several years without issues. I would be very surprised if it was only safe to call on main thread. Have not found any reason why that should be the case. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:495: if (!webrtc::ActivateAudioSession(session, false)) { On 2015/11/18 18:51:48, pbos-webrtc wrote: > You don't need the webrtc:: prefix here or above. Acknowledged. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:496: LOG(LS_ERROR) << "Failed to deactivate the audio session"; Will make comment more clear. It is not a real leak but only a deviation from an recommended state. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:910: // Activate our own audio session if not already activated. Will fix. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:922: // TODO(henrika): remove CHECK when we are sure that we no longer see Will try to avoid CHECK. I know that we have discussed it before. Using a CHECK here feels too "dramatic" IMHO. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:933: if (nullptr != vpio_unit_) { No, if ActivateAudiSession has failed, we will never try to create the audio unit. We can only enter this scope if the pointer is non-null. If we bailed out early, we would miss calling Deactive even if Activate was successful.
pbos: let's discuss the final details off-line tomorrow.
Fixed. Uploading new version. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.h (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:16: #include "webrtc/base/criticalsection.h" On 2015/11/18 18:51:48, pbos-webrtc wrote: > Move to .cc file Done. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.h:18: #include "webrtc/base/thread_annotations.h" On 2015/11/18 18:51:48, pbos-webrtc wrote: > Move to .cc file Done. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:463: bool AudioDeviceIOS::ActivateAudioSession() { On 2015/11/18 19:48:21, henrika_webrtc wrote: > Missed that. Will fix. Done.
pbos: PTAL
pbos: done more tests and found some issues, please hold thanks.
On 2015/11/19 14:26:31, henrika_webrtc wrote: > pbos: done more tests and found some issues, please hold thanks. yessir, let me know!
Patchset #7 (id:120001) has been deleted
pbos: many, many tests later. Please check the (final) version. Thanks!
https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:922: // TODO(henrika): remove CHECK when we are sure that we no longer see On 2015/11/18 19:48:21, henrika_webrtc wrote: > Will try to avoid CHECK. I know that we have discussed it before. Using a CHECK > here feels too "dramatic" IMHO. Maybe a DCHECK? https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:950: DeactivateAudioSession(); But this can be called even if vpio_unit_ == nullptr? Is that cool? https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:210: static bool ActivateAudioSessionWithLock() { Don't need WithLock in the name here. https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:235: static bool DeactivateAudioSessionWithLock() { Same here https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:341: { mismatched {, this can't compile https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:428: LOG(LS_INFO) << "Voice-Processing I/O audio unit is now started"; stopped?
Thanks! https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:922: // TODO(henrika): remove CHECK when we are sure that we no longer see Would like to avoid DCHECK as well since the current design will allow to attempts to succeed (one for play and one for rec) and I don't want to break at dirt failure. It feels better to just log an error message here. https://codereview.webrtc.org/1435293003/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:950: DeactivateAudioSession(); Yep. That is perfectly OK and the preferred way. https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:210: static bool ActivateAudioSessionWithLock() { On 2015/11/20 12:29:50, pbos-webrtc wrote: > Don't need WithLock in the name here. Acknowledged. https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:235: static bool DeactivateAudioSessionWithLock() { On 2015/11/20 12:29:50, pbos-webrtc wrote: > Same here Acknowledged. https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:341: { Ooops. Fixed. https://codereview.webrtc.org/1435293003/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_ios.mm:428: LOG(LS_INFO) << "Voice-Processing I/O audio unit is now started"; No, started. Maybe I am missing something but the comment is correct.
PTAL
lgtm
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1435293003/#ps160001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1435293003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1435293003/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001) manually as 34911ad55c4c4c549fe60e1b4cc127420b15666b (presubmit successful).
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/34911ad55c4c4c549fe60e1b4cc127420b15666b Cr-Commit-Position: refs/heads/master@{#10728} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
