Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2822233002:
Don't call unconfigureWebRTCSession if setConfiguration fails. webrtc:7471 (Closed)
|
Created:
3 years, 8 months ago by jtt_webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, audio-team_agora.io, sdk-team_agora.io, peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't call unconfigureWebRTCSession if configureWebRTCSession fails.
Otherwise, the activeCount will become negative.
BUG=webrtc:7471
Review-Url: https://codereview.webrtc.org/2822233002
Cr-Commit-Position: refs/heads/master@{#17816}
Committed: https://chromium.googlesource.com/external/webrtc/+/f84c1d6644923251ee9ca2ed6723df86c3baa999
Patch Set 1 #
Total comments: 21
Patch Set 2 : Handle configure failure in audio_device_ios.mm #
Total comments: 6
Patch Set 3 : updated comment #
Total comments: 2
Patch Set 4 : updated #Patch Set 5 : updated' #
Total comments: 1
Messages
Total messages: 26 (10 generated)
Description was changed from ========== Don't call unconfigureWebRTCSession if setConfiguration fails. webrtc:7471 ========== to ========== Don't call unconfigureWebRTCSession if configureWebRTCSession fails. Otherwise, the activeCount will become negative. BUG=webrtc:7471 ==========
jtteh@webrtc.org changed reviewers: + henrika@webrtc.org, tkchin@webrtc.org
https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... File webrtc/modules/audio_device/ios/objc/RTCAudioSession.mm (right): https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSession.mm:691: // Do not call setActive:NO if setActive:YES failed. Can you also check audio_device_ios.mm and make sure that it is balancing configure/unconfigure correctly? https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:190: // Hack - fixes OCMVerify link error what is the link error? https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:192: return [OCMLocation locationWithTestCase:testCase file:[NSString align : https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:197: NSError *error; nit: init to nil https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:200: __autoreleasing NSError **retError; Do we really need to specify autoreleasing? https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:208: OCMExpect([[mockAVAudioSession ignoringNonObjectArgs] nit: stub everything you need first, and then place expectations before callsites https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:223: // Force failure, so activationCount should remain 0 nit: this isn't technically forcing a failure - the failure is forced by the setActive block above. Clarify the comment. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:229: EXPECT_EQ(FALSE, [mockAVAudioSession setActive:YES withOptions:0 error:&error]); don't use FALSE -> NO
Looks like great work. I have no experience from OCMock and can't provide detailed feedback on those parts. lgtm
https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:202: *retError = [NSError errorWithDomain:@"AVAudioSession" code:561017449 userInfo:nil]; Where does these details come from?
On 2017/04/18 18:14:11, henrika_webrtc wrote: > https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... > File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): > > https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... > webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:202: *retError = > [NSError errorWithDomain:@"AVAudioSession" code:561017449 userInfo:nil]; > Where does these details come from? Updated to use iOS error AVAudioSessionErrorInsufficientPriority.
Handle configure failure in audio_device_ios.mm also. Ran audio tests on device (doesn't work on sim). https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... File webrtc/modules/audio_device/ios/objc/RTCAudioSession.mm (right): https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSession.mm:691: // Do not call setActive:NO if setActive:YES failed. On 2017/04/18 17:53:28, tkchin_webrtc wrote: > Can you also check audio_device_ios.mm and make sure that it is balancing > configure/unconfigure correctly? Acknowledged. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:190: // Hack - fixes OCMVerify link error On 2017/04/18 17:53:28, tkchin_webrtc wrote: > what is the link error? // Link error is: Undefined symbols for architecture i386: // "OCMMakeLocation(objc_object*, char const*, int)", referenced from: // -[RTCAudioSessionTest testConfigureWebRTCSession] in RTCAudioSessionTest.o // ld: symbol(s) not found for architecture i386 // REASON: https://github.com/erikdoe/ocmock/issues/238 https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:192: return [OCMLocation locationWithTestCase:testCase file:[NSString On 2017/04/18 17:53:28, tkchin_webrtc wrote: > align : Done. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:197: NSError *error; On 2017/04/18 17:53:28, tkchin_webrtc wrote: > nit: init to nil Defaults to nil. Will add it anyway. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:200: __autoreleasing NSError **retError; On 2017/04/18 17:53:28, tkchin_webrtc wrote: > Do we really need to specify autoreleasing? Yes, otherwise it'll throw a build error. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:202: *retError = [NSError errorWithDomain:@"AVAudioSession" code:561017449 userInfo:nil]; On 2017/04/18 18:14:11, henrika_webrtc wrote: > Where does these details come from? Acknowledged. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:208: OCMExpect([[mockAVAudioSession ignoringNonObjectArgs] On 2017/04/18 17:53:28, tkchin_webrtc wrote: > nit: stub everything you need first, and then place expectations before > callsites > Done. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:223: // Force failure, so activationCount should remain 0 On 2017/04/18 17:53:28, tkchin_webrtc wrote: > nit: this isn't technically forcing a failure - the failure is forced by the > setActive block above. Clarify the comment. Done. https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:229: EXPECT_EQ(FALSE, [mockAVAudioSession setActive:YES withOptions:0 error:&error]); On 2017/04/18 17:53:28, tkchin_webrtc wrote: > don't use FALSE -> NO Done.
https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:190: // Hack - fixes OCMVerify link error On 2017/04/18 18:53:54, jtt wrote: > On 2017/04/18 17:53:28, tkchin_webrtc wrote: > > what is the link error? > > // Link error is: Undefined symbols for architecture i386: > // "OCMMakeLocation(objc_object*, char const*, int)", referenced from: > // -[RTCAudioSessionTest testConfigureWebRTCSession] in RTCAudioSessionTest.o > // ld: symbol(s) not found for architecture i386 > // REASON: https://github.com/erikdoe/ocmock/issues/238 Was that fixed upstream? Can we update ocmock? https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:200: __autoreleasing NSError **retError; On 2017/04/18 18:53:54, jtt wrote: > On 2017/04/18 17:53:28, tkchin_webrtc wrote: > > Do we really need to specify autoreleasing? > > Yes, otherwise it'll throw a build error. what's the build error? (for my education) https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:285: RTCAudioSessionTest *test = [[RTCAudioSessionTest alloc] init]; Do all these need to be in an autoreleasepool? I don't remember if I made a change so that all gUnit unit tests automatically have one - I do remember considering it at one point. https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:727: RTCLogError(@"Failed to configure audio unit."); misleading - the audio unit is never configured here. Failed to configure audio session? will this cause problems though? What if the error is transient? When would audio unit initialization be attempted again? Is it better to attempt to setup the audio unit anyway?
On 2017/04/18 21:06:34, tkchin_webrtc wrote: > https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... > File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): > > https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... > webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:190: // Hack - fixes > OCMVerify link error > On 2017/04/18 18:53:54, jtt wrote: > > On 2017/04/18 17:53:28, tkchin_webrtc wrote: > > > what is the link error? > > > > // Link error is: Undefined symbols for architecture i386: > > // "OCMMakeLocation(objc_object*, char const*, int)", referenced from: > > // -[RTCAudioSessionTest testConfigureWebRTCSession] in RTCAudioSessionTest.o > > // ld: symbol(s) not found for architecture i386 > > // REASON: https://github.com/erikdoe/ocmock/issues/238 > > Was that fixed upstream? Can we update ocmock? > > https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... > webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:200: __autoreleasing > NSError **retError; > On 2017/04/18 18:53:54, jtt wrote: > > On 2017/04/18 17:53:28, tkchin_webrtc wrote: > > > Do we really need to specify autoreleasing? > > > > Yes, otherwise it'll throw a build error. > > what's the build error? (for my education) Undefined symbols for architecture i386: "OCMMakeLocation(objc_object*, char const*, int)", referenced from: -[RTCAudioSessionTest testConfigureWebRTCSession] in RTCAudioSessionTest.o ld: symbol(s) not found for architecture i386 > > https://codereview.webrtc.org/2822233002/diff/1/webrtc/modules/audio_device/i... > webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:285: > RTCAudioSessionTest *test = [[RTCAudioSessionTest alloc] init]; > Do all these need to be in an autoreleasepool? I don't remember if I made a > change so that all gUnit unit tests automatically have one - I do remember > considering it at one point. The only things that are explicitly allocated would be the RTCAudioSession, that's a singleton. So may not be needed. > > https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): > > https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_ios.mm:727: RTCLogError(@"Failed to > configure audio unit."); > misleading - the audio unit is never configured here. Failed to configure audio > session? > > will this cause problems though? What if the error is transient? When would > audio unit initialization be attempted again? > > Is it better to attempt to setup the audio unit anyway?
OCMMock webrtc uses is v3.1 The OCMVerify bug was fixed in 3.2 https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:727: RTCLogError(@"Failed to configure audio unit."); On 2017/04/18 21:06:33, tkchin_webrtc wrote: > misleading - the audio unit is never configured here. Failed to configure audio > session? > > will this cause problems though? What if the error is transient? When would > audio unit initialization be attempted again? > > Is it better to attempt to setup the audio unit anyway? Callers may assume that the audio session has been configured. Should it be better to fail? There may be unseen consequences of having the audio unit initialized without the audio session activated.
The CQ bit was checked by jtteh@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
I've filed webrtc:7496 bug to update OCMMock from v3.1 to v3.2 to fix the OCMVerify link error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Bump - >24hrs
https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:727: RTCLogError(@"Failed to configure audio unit."); On 2017/04/18 21:28:05, jtt wrote: > On 2017/04/18 21:06:33, tkchin_webrtc wrote: > > misleading - the audio unit is never configured here. Failed to configure > audio > > session? > > > > will this cause problems though? What if the error is transient? When would > > audio unit initialization be attempted again? > > > > Is it better to attempt to setup the audio unit anyway? > > Callers may assume that the audio session has been configured. Should it be > better to fail? > There may be unseen consequences of having the audio unit initialized without > the audio session activated. I'm worried that this will break existing situations where we attempt to initialize anyway and it works. If we don't try to start here - when will we actually retry? We can try it if you think the risk is low. Please update the text to "Failed to configure audio session" rather than "Failed to configure audio unit". Maybe do it in ConfigureAudioSession. https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:769: RTCLog(@"Configured audio session."); This isn't true if the session configuration failed. Use bool. https://codereview.webrtc.org/2822233002/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2822233002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:209: code:AVAudioSessionErrorInsufficientPriority userInfo:nil]; align : vertically
Addressed comments. https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:727: RTCLogError(@"Failed to configure audio unit."); On 2017/04/21 19:52:21, tkchin_webrtc wrote: > On 2017/04/18 21:28:05, jtt wrote: > > On 2017/04/18 21:06:33, tkchin_webrtc wrote: > > > misleading - the audio unit is never configured here. Failed to configure > > audio > > > session? > > > > > > will this cause problems though? What if the error is transient? When would > > > audio unit initialization be attempted again? > > > > > > Is it better to attempt to setup the audio unit anyway? > > > > Callers may assume that the audio session has been configured. Should it be > > better to fail? > > There may be unseen consequences of having the audio unit initialized without > > the audio session activated. > > I'm worried that this will break existing situations where we attempt to > initialize anyway and it works. > If we don't try to start here - when will we actually retry? We can try it if > you think the risk is low. > > Please update the text to "Failed to configure audio session" rather than > "Failed to configure audio unit". Maybe do it in ConfigureAudioSession. I'll change it so that SetupAudioBuffersForActiveAudioSession(); is called anyway. https://codereview.webrtc.org/2822233002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:769: RTCLog(@"Configured audio session."); On 2017/04/21 19:52:21, tkchin_webrtc wrote: > This isn't true if the session configuration failed. Use bool. Done. https://codereview.webrtc.org/2822233002/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2822233002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:209: code:AVAudioSessionErrorInsufficientPriority userInfo:nil]; On 2017/04/21 19:52:21, tkchin_webrtc wrote: > align : vertically Done.
lgtm https://codereview.webrtc.org/2822233002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_ios.mm (right): https://codereview.webrtc.org/2822233002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_ios.mm:765: if (success) { nit: can just set to has_configured_session_ above to avoid unneeded local var
The CQ bit was checked by jtteh@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/2822233002/#ps80001 (title: "updated'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492807080669210,
"parent_rev": "99cdcb6dea4d1cb35c83f49ad94c90bd00db31e2", "commit_rev":
"f84c1d6644923251ee9ca2ed6723df86c3baa999"}
Message was sent while issue was closed.
Description was changed from ========== Don't call unconfigureWebRTCSession if configureWebRTCSession fails. Otherwise, the activeCount will become negative. BUG=webrtc:7471 ========== to ========== Don't call unconfigureWebRTCSession if configureWebRTCSession fails. Otherwise, the activeCount will become negative. BUG=webrtc:7471 Review-Url: https://codereview.webrtc.org/2822233002 Cr-Commit-Position: refs/heads/master@{#17816} Committed: https://chromium.googlesource.com/external/webrtc/+/f84c1d6644923251ee9ca2ed6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/f84c1d6644923251ee9ca2ed6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
