Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3009383002:
Enhance RTCUIApplicationStatusObserver thread safety. (Closed)
DescriptionEnhance RTCUIApplicationStatusObserver thread safety.
Add locking around waiting for initialization to finish, since calling
dispatch_block_wait from multiple threads leads to undefined behavior.
Initialize RTCUIApplicationStatusObserver earlier to give the
initialization block more time to run on the main thread before
starting to query the application state.
http://www.dailymotion.com/video/x2mckmh
BUG=b/65558688
Review-Url: https://codereview.webrtc.org/3009383002
Cr-Commit-Position: refs/heads/master@{#19822}
Committed: https://chromium.googlesource.com/external/webrtc/+/9a85f0782ea4647d77515b48b4bd2e1b0826210a
Patch Set 1 #
Total comments: 8
Patch Set 2 : Better name for the method to prepare the singleton for use. #Patch Set 3 : Add note to explain the waiting logic in `isApplicationActive`. #Patch Set 4 : Rebase #
Total comments: 6
Messages
Total messages: 22 (9 generated)
Description was changed from ========== Enchance RTCUIApplicationStatusObserver thread safety. Add locking around waiting for initialization to finish, since calling dispatch_block_wait from multiple threads leads to undefined behavior. Initialize RTCUIApplicationStatusObserver earlier to give the initialization block more time to run on the main thread before starting to query the application state. http://www.dailymotion.com/video/x2mckmh BUG=b/65558688 ========== to ========== Enchance RTCUIApplicationStatusObserver thread safety. Add locking around waiting for initialization to finish, since calling dispatch_block_wait from multiple threads leads to undefined behavior. Initialize RTCUIApplicationStatusObserver earlier to give the initialization block more time to run on the main thread before starting to query the application state. http://www.dailymotion.com/video/x2mckmh BUG=b/65558688 ==========
andersc@webrtc.org changed reviewers: + haysc@webrtc.org
andersc@webrtc.org changed reviewers: + denicija@webrtc.org, kthelgason@webrtc.org
Description was changed from ========== Enchance RTCUIApplicationStatusObserver thread safety. Add locking around waiting for initialization to finish, since calling dispatch_block_wait from multiple threads leads to undefined behavior. Initialize RTCUIApplicationStatusObserver earlier to give the initialization block more time to run on the main thread before starting to query the application state. http://www.dailymotion.com/video/x2mckmh BUG=b/65558688 ========== to ========== Enhance RTCUIApplicationStatusObserver thread safety. Add locking around waiting for initialization to finish, since calling dispatch_block_wait from multiple threads leads to undefined behavior. Initialize RTCUIApplicationStatusObserver earlier to give the initialization block more time to run on the main thread before starting to query the application state. http://www.dailymotion.com/video/x2mckmh BUG=b/65558688 ==========
Please take a look
On 2017/09/13 08:03:52, andersc wrote: > Please take a look There is more information about the thinking behind these fixes in the bug b/65558688
https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/VideoToolbox/RTCVideoEncoderH264.mm (right): https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/VideoToolbox/RTCVideoEncoderH264.mm:308: #endif I think this needs a comment to explain to future generations what's going on. We could also add a class method with a clearer name like `initialize`. Then it may be obvious what's going on without a comment.
https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/VideoToolbox/RTCVideoEncoderH264.mm (right): https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/VideoToolbox/RTCVideoEncoderH264.mm:308: #endif On 2017/09/13 08:20:09, kthelgason wrote: > I think this needs a comment to explain to future generations what's going on. > > We could also add a class method with a clearer name like `initialize`. Then it > may be obvious what's going on without a comment. Good idea. I named the method `prepareForUse` since `initialize` clashes with the method https://developer.apple.com/documentation/objectivec/nsobject/1418639-initial...
lgtm with two comments to be addressed https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:68: dispatch_semaphore_wait(_waitForInitializeSemaphore, DISPATCH_TIME_FOREVER); This code is becoming slightly complex now. Would you mind adding comments to describe why it was done like it is? https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:69: if (!_initialized) { I guess we want to synchronize only if the observation wasn't initialized? Wouldn't moving the semaphore in the if statement make more sense?
lgtm
https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:68: dispatch_semaphore_wait(_waitForInitializeSemaphore, DISPATCH_TIME_FOREVER); On 2017/09/13 09:02:50, daniela-webrtc wrote: > This code is becoming slightly complex now. Would you mind adding comments to > describe why it was done like it is? Acknowledged. https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:69: if (!_initialized) { On 2017/09/13 09:02:50, daniela-webrtc wrote: > I guess we want to synchronize only if the observation wasn't initialized? > Wouldn't moving the semaphore in the if statement make more sense? Not sure what you mean. Since the `dispatch_block_wait` function can only legally be called once, this if statement basically checks if we were stuck in the semaphore waiting for the other thread. If we were, and the other thread has now finished waiting for the initialization block to run and exited the critical section, then we are already initialized and should not call `dispatch_block_wait` but just proceed and read the _state variable. If _initialized is still false after we enter the critical section, we are the first thread to come here and should call `dispatch_block_wait`. I clarified this in a comment.
https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:69: if (!_initialized) { On 2017/09/13 09:19:51, andersc wrote: > On 2017/09/13 09:02:50, daniela-webrtc wrote: > > I guess we want to synchronize only if the observation wasn't initialized? > > Wouldn't moving the semaphore in the if statement make more sense? > > Not sure what you mean. Since the `dispatch_block_wait` function can only > legally be called once, this if statement basically checks if we were stuck in > the semaphore waiting for the other thread. If we were, and the other thread has > now finished waiting for the initialization block to run and exited the critical > section, then we are already initialized and should not call > `dispatch_block_wait` but just proceed and read the _state variable. If > _initialized is still false after we enter the critical section, we are the > first thread to come here and should call `dispatch_block_wait`. > > I clarified this in a comment. Missed that there were two if statements. The comments make it clearer. Thanks! https://codereview.webrtc.org/3009383002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:69: if (!_initialized) { On 2017/09/13 09:19:51, andersc wrote: > On 2017/09/13 09:02:50, daniela-webrtc wrote: > > I guess we want to synchronize only if the observation wasn't initialized? > > Wouldn't moving the semaphore in the if statement make more sense? > > Not sure what you mean. Since the `dispatch_block_wait` function can only > legally be called once, this if statement basically checks if we were stuck in > the semaphore waiting for the other thread. If we were, and the other thread has > now finished waiting for the initialization block to run and exited the critical > section, then we are already initialized and should not call > `dispatch_block_wait` but just proceed and read the _state variable. If > _initialized is still false after we enter the critical section, we are the > first thread to come here and should call `dispatch_block_wait`. > > I clarified this in a comment. Acknowledged.
lgtm
The CQ bit was checked by andersc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/3009383002/#ps60001 (title: "Rebase")
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": 60001, "attempt_start_ts": 1505311726509380,
"parent_rev": "f54573bd3b9774db6782af8de0f22242598cb974", "commit_rev":
"9a85f0782ea4647d77515b48b4bd2e1b0826210a"}
Message was sent while issue was closed.
Description was changed from ========== Enhance RTCUIApplicationStatusObserver thread safety. Add locking around waiting for initialization to finish, since calling dispatch_block_wait from multiple threads leads to undefined behavior. Initialize RTCUIApplicationStatusObserver earlier to give the initialization block more time to run on the main thread before starting to query the application state. http://www.dailymotion.com/video/x2mckmh BUG=b/65558688 ========== to ========== Enhance RTCUIApplicationStatusObserver thread safety. Add locking around waiting for initialization to finish, since calling dispatch_block_wait from multiple threads leads to undefined behavior. Initialize RTCUIApplicationStatusObserver earlier to give the initialization block more time to run on the main thread before starting to query the application state. http://www.dailymotion.com/video/x2mckmh BUG=b/65558688 Review-Url: https://codereview.webrtc.org/3009383002 Cr-Commit-Position: refs/heads/master@{#19822} Committed: https://chromium.googlesource.com/external/webrtc/+/9a85f0782ea4647d77515b48b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/9a85f0782ea4647d77515b48b...
Message was sent while issue was closed.
tkchin@webrtc.org changed reviewers: + tkchin@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:22: @property(nonatomic, assign) UIApplicationState state; State is read and written to from multiple threads. It should be atomic, and only accessed via property accessor instead of directly through _state. https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:51: + (void)prepareForUse { This class feels more complex than it needs to be. I suspect that if we constructed multiple instances instead of a singleton, created the instance in encoder / decoder ctors, provided an isReady method, ignored frames until main queue initialization is complete, we'd have the same thing working. I'd guess there'd be sufficient time between encoder / decoder ctor and the first encode/decode method call to complete init in most cases because it will take time for capture to begin. https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:104: dispatch_time(DISPATCH_TIME_NOW, 10.0 * NSEC_PER_SEC)); What's the benefit of setting a short timeout here? It seems to introduce bad behavior if the timeout isn't met, and it seems hard to guarantee that this number works for all iOS hardware. DISPATCH_TIME_FOREVER seems more appropriate.
Message was sent while issue was closed.
https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:22: @property(nonatomic, assign) UIApplicationState state; On 2017/09/13 18:32:02, tkchin_webrtc wrote: > State is read and written to from multiple threads. It should be atomic, and > only accessed via property accessor instead of directly through _state. Acknowledged. https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:51: + (void)prepareForUse { On 2017/09/13 18:32:02, tkchin_webrtc wrote: > This class feels more complex than it needs to be. > I suspect that if we constructed multiple instances instead of a singleton, > created the instance in encoder / decoder ctors, provided an isReady method, > ignored frames until main queue initialization is complete, we'd have the same > thing working. I'd guess there'd be sufficient time between encoder / decoder > ctor and the first encode/decode method call to complete init in most cases > because it will take time for capture to begin. > I agree, and I mentioned a solution similar to that in b/65558688#comment4 The reason I did it this way was that it is a logically smaller change than from what we had before, and I'm not confident that returning OK from the encoder/decoder without performing the work doesn't impact any other record keeping made by WebRTC. But it should be easy to try out and see if it works as expected. Let me know if you think we should spend time on trying such a solution out instead. https://codereview.webrtc.org/3009383002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:104: dispatch_time(DISPATCH_TIME_NOW, 10.0 * NSEC_PER_SEC)); On 2017/09/13 18:32:02, tkchin_webrtc wrote: > What's the benefit of setting a short timeout here? It seems to introduce bad > behavior if the timeout isn't met, and it seems hard to guarantee that this > number works for all iOS hardware. DISPATCH_TIME_FOREVER seems more appropriate. 10 seconds was picked to be a long timeout. A test deadlocked (because the main queue was blocked on something else), so added the DCHECK to make it easier to debug similar issues in the future, rather than the test just hanging. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
