Improve locking situation in ASVideoPlayerNode (#1042)

* Improve locking in ASVideoPlayerNode * Address comments
TextureGroup · Jul 20, 2018 · d9d5b12 · d9d5b12
1 parent 5fef6a1
commit d9d5b12
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,7 +23,9 @@
 - Split MapKit, Photos, and AssetsLibrary dependent code into separate subspecs to improve binary size and start time when they're not needed. The default subspec includes all three for backwards compatibility, but this **will change in 3.0**. When using non-Cocoapods build environments, define `AS_USE_PHOTOS, AS_USE_MAPKIT, AS_USE_ASSETS_LIBRARY` to 1 respectively to signal their use. [Adlai Holler](https://github.com/Adlai-Holler)
 - Optimization: Removed an NSMutableArray in flattened layouts. [Adlai Holler](https://github.com/Adlai-Holler)
 - Reduced binary size by disabling exception support (which we don't use.) [Adlai Holler](https://github.com/Adlai-Holler)
-- Create and set delegate for clip corner layers within ASDisplayNode [Michael Schneider](https://github.com/maicki) [#1024](https://github.com/TextureGroup/Texture/pull/1029)
+- Create and set delegate for clip corner layers within ASDisplayNode [Michael Schneider](https://github.com/maicki) [#1029](https://github.com/TextureGroup/Texture/pull/1029)
+- Improve locking situation in ASVideoPlayerNode [Michael Schneider](https://github.com/maicki) [#1042](https://github.com/TextureGroup/Texture/pull/1042)
+
 
 ## 2.7
 - Fix pager node for interface coalescing. [Max Wang](https://github.com/wsdwsd0829) [#877](https://github.com/TextureGroup/Texture/pull/877)

diff --git a/Source/ASVideoPlayerNode.mm b/Source/ASVideoPlayerNode.mm
@@ -209,10 +209,8 @@ - (AVAsset *)asset
 - (void)didLoad
 {
   [super didLoad];
-  {
-    ASLockScopeSelf();
-    [self createControls];
-  }
+
+  [self createControls];
 }
 
 - (void)didEnterPreloadState
@@ -284,15 +282,23 @@ - (void)createControls
 
     if (_delegateFlags.delegateCustomControls && _delegateFlags.delegateLayoutSpecForControls) {
       NSDictionary *customControls = [_delegate videoPlayerNodeCustomControls:self];
+      std::vector<ASDisplayNode *> subnodes;
       for (id key in customControls) {
         id node = customControls[key];
         if (![node isKindOfClass:[ASDisplayNode class]]) {
           continue;
         }
 
-        [self addSubnode:node];
+        subnodes.push_back(node);
         [_cachedControls setObject:node forKey:key];
       }
+
+      {
+        ASUnlockScope(self);
+        for (var subnode : subnodes) {
+          [self addSubnode:subnode];
+        }
+      }
     }
   }
 
@@ -315,14 +321,21 @@ - (NSArray *)createDefaultControlElementArray
 
 - (void)removeControls
 {
-  for (ASDisplayNode *node in [_cachedControls objectEnumerator]) {
-    [node removeFromSupernode];
+  NSMutableDictionary *cachedControls = nil;
+  {
+    ASLockScope(self);
+
+    // Grab the cached controls for removing it
+    cachedControls = [_cachedControls copy];
+    [self _locked_cleanCachedControls];
   }
 
-  [self cleanCachedControls];
+  for (ASDisplayNode *node in [cachedControls objectEnumerator]) {
+    [node removeFromSupernode];
+  }
 }
 
-- (void)cleanCachedControls
+- (void)_locked_cleanCachedControls
 {
   [_cachedControls removeAllObjects];
 
@@ -355,7 +368,10 @@ - (void)_locked_createPlaybackButton
     [_cachedControls setObject:_playbackButtonNode forKey:@(ASVideoPlayerNodeControlTypePlaybackButton)];
   }
 
-  [self addSubnode:_playbackButtonNode];
+  {
+    ASUnlockScope(self);
+    [self addSubnode:_playbackButtonNode];
+  }
 }
 
 - (void)_locked_createFullScreenButton
@@ -374,7 +390,10 @@ - (void)_locked_createFullScreenButton
     [_cachedControls setObject:_fullScreenButtonNode forKey:@(ASVideoPlayerNodeControlTypeFullScreenButton)];
   }
 
-  [self addSubnode:_fullScreenButtonNode];
+  {
+    ASUnlockScope(self);
+    [self addSubnode:_fullScreenButtonNode];
+  }
 }
 
 - (void)_locked_createElapsedTextField
@@ -389,7 +408,10 @@ - (void)_locked_createElapsedTextField
 
     [_cachedControls setObject:_elapsedTextNode forKey:@(ASVideoPlayerNodeControlTypeElapsedText)];
   }
-  [self addSubnode:_elapsedTextNode];
+  {
+    ASUnlockScope(self);
+    [self addSubnode:_elapsedTextNode];
+  }
 }
 
 - (void)_locked_createDurationTextField
@@ -405,7 +427,10 @@ - (void)_locked_createDurationTextField
     [_cachedControls setObject:_durationTextNode forKey:@(ASVideoPlayerNodeControlTypeDurationText)];
   }
   [self updateDurationTimeLabel];
-  [self addSubnode:_durationTextNode];
+  {
+    ASUnlockScope(self);
+    [self addSubnode:_durationTextNode];
+  }
 }
 
 - (void)_locked_createScrubber
@@ -450,8 +475,10 @@ - (void)_locked_createScrubber
 
     [_cachedControls setObject:_scrubberNode forKey:@(ASVideoPlayerNodeControlTypeScrubber)];
   }
-
-  [self addSubnode:_scrubberNode];
+  {
+    ASUnlockScope(self);
+    [self addSubnode:_scrubberNode];
+  }
 }
 
 - (void)_locked_createControlFlexGrowSpacer
@@ -623,7 +650,6 @@ - (void)showSpinner
   ASLockScopeSelf();
 
   if (!_spinnerNode) {
-
     __weak __typeof__(self) weakSelf = self;
     _spinnerNode = [[ASDisplayNode alloc] initWithViewBlock:^UIView *{
       __typeof__(self) strongSelf = weakSelf;
@@ -642,24 +668,32 @@ - (void)showSpinner
 
       return spinnnerView;
     }];
-
     _spinnerNode.style.preferredSize = CGSizeMake(44.0, 44.0);
-
-    [self addSubnode:_spinnerNode];
-    [self setNeedsLayout];
+
+    let spinnerNode = _spinnerNode;
+    {
+      ASUnlockScope(self);
+      [self addSubnode:spinnerNode];
+      [self setNeedsLayout];
+    }
   }
   [(UIActivityIndicatorView *)_spinnerNode.view startAnimating];
 }
 
 - (void)removeSpinner
 {
-  ASLockScopeSelf();
-
-  if (!_spinnerNode) {
-    return;
+  ASDisplayNode *spinnerNode = nil;
+  {
+    ASLockScopeSelf();
+    if (!_spinnerNode) {
+      return;
+    }
+
+    spinnerNode = _spinnerNode;
+    _spinnerNode = nil;
   }
-  [_spinnerNode removeFromSupernode];
-  _spinnerNode = nil;
+
+  [spinnerNode removeFromSupernode];
 }
 
 - (void)didTapPlaybackButton:(ASControlNode*)node