From 492d0d7ad61312e0bf9cfeef1835f34185174b6a Mon Sep 17 00:00:00 2001 From: Wendy Date: Thu, 28 Apr 2016 14:52:31 -0700 Subject: [PATCH 1/8] [ASNetworkImageNode] Fix threading issue in current image quality Summary: Because we trampoline to main when setting _currentImageQuality, there would be situations where displayWillStart was called, we get a cached image and set currentImageQuality to 1.0, and then a background thread calling setDefaultImage would enqueue an operation that sets currentImageQuality to 0 and overwrites the correct value --- AsyncDisplayKit/ASNetworkImageNode.mm | 28 ++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/AsyncDisplayKit/ASNetworkImageNode.mm b/AsyncDisplayKit/ASNetworkImageNode.mm index fa42a3c3..8ff2be7c 100755 --- a/AsyncDisplayKit/ASNetworkImageNode.mm +++ b/AsyncDisplayKit/ASNetworkImageNode.mm @@ -124,9 +124,15 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; if (reset || _URL == nil) { self.image = _defaultImage; - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 1.0; - }); + if (_URL == nil) { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 1.0; + }); + } else { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 0.0; + }); + } } if (self.interfaceState & ASInterfaceStateFetchData) { @@ -151,9 +157,15 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _defaultImage = defaultImage; if (!_imageLoaded) { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 0.0; - }); + if (_URL == nil) { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 1.0; + }); + } else { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 0.0; + }); + } _lock.unlock(); // Locking: it is important to release _lock before entering setImage:, as it needs to release the lock before -invalidateCalculatedLayout. // If we continue to hold the lock here, it will still be locked until the next unlock() call, causing a possible deadlock with @@ -229,7 +241,9 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; if (result) { self.image = result; _imageLoaded = YES; - _currentImageQuality = 1.0; + dispatch_async(dispatch_get_main_queue(), ^{ + _currentImageQuality = 1.0; + }); } } } From 75b55974b53ebcc1b74170ff71129963a8fdea80 Mon Sep 17 00:00:00 2001 From: Robin Chou Date: Thu, 28 Apr 2016 20:02:15 -0400 Subject: [PATCH 2/8] Loads asset values with `-loadValuesAsync`. --- AsyncDisplayKit/ASVideoNode.mm | 53 ++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/AsyncDisplayKit/ASVideoNode.mm b/AsyncDisplayKit/ASVideoNode.mm index 3cf7b34c..2df9dcf0 100644 --- a/AsyncDisplayKit/ASVideoNode.mm +++ b/AsyncDisplayKit/ASVideoNode.mm @@ -100,16 +100,41 @@ static NSString * const kStatus = @"status"; ASDN::MutexLocker l(_videoLock); if (_asset != nil) { - if ([_asset isKindOfClass:[AVURLAsset class]]) { - return [[AVPlayerItem alloc] initWithURL:((AVURLAsset *)_asset).URL]; - } else { - return [[AVPlayerItem alloc] initWithAsset:_asset]; - } + return [[AVPlayerItem alloc] initWithAsset:_asset]; } return nil; } +- (void)prepareToPlayAsset:(AVAsset *)asset withKeys:(NSArray *)requestedKeys +{ + for (NSString *key in requestedKeys) { + NSError *error = nil; + AVKeyValueStatus keyStatus = [asset statusOfValueForKey:key error:&error]; + if (keyStatus == AVKeyValueStatusFailed) { + NSLog(@"Asset loading failed with error: %@", error); + } + } + + if (![asset isPlayable]) { + NSLog(@"Asset is not playable."); + return; + } + + AVPlayerItem *playerItem = [self constructPlayerItem]; + self.currentItem = playerItem; + + if (_player != nil) { + [_player replaceCurrentItemWithPlayerItem:playerItem]; + } else { + self.player = [AVPlayer playerWithPlayerItem:playerItem]; + } + + if (_placeholderImageNode.image == nil) { + [self generatePlaceholderImage]; + } +} + - (void)addPlayerItemObservers:(AVPlayerItem *)playerItem { [playerItem addObserver:self forKeyPath:kStatus options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew context:ASVideoNodeContext]; @@ -264,17 +289,13 @@ static NSString * const kStatus = @"status"; { ASDN::MutexLocker l(_videoLock); - AVPlayerItem *playerItem = [self constructPlayerItem]; - self.currentItem = playerItem; - - if (_player != nil) { - [_player replaceCurrentItemWithPlayerItem:playerItem]; - } else { - self.player = [AVPlayer playerWithPlayerItem:playerItem]; - } - - if (_placeholderImageNode.image == nil) { - [self generatePlaceholderImage]; + if (_asset != nil) { + NSArray *requestedKeys = @[ @"playable" ]; + [_asset loadValuesAsynchronouslyForKeys:requestedKeys completionHandler:^{ + dispatch_async(dispatch_get_main_queue(), ^{ + [self prepareToPlayAsset:_asset withKeys:requestedKeys]; + }); + }]; } } } From 9c2909a968b068cd188cfd2b403fe949263c8585 Mon Sep 17 00:00:00 2001 From: Robin Chou Date: Thu, 28 Apr 2016 21:04:04 -0400 Subject: [PATCH 3/8] Updating tests. --- AsyncDisplayKitTests/ASVideoNodeTests.m | 54 +++++++++++++++++++++---- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/AsyncDisplayKitTests/ASVideoNodeTests.m b/AsyncDisplayKitTests/ASVideoNodeTests.m index 3effa243..7464e9eb 100644 --- a/AsyncDisplayKitTests/ASVideoNodeTests.m +++ b/AsyncDisplayKitTests/ASVideoNodeTests.m @@ -6,6 +6,8 @@ * of patent rights can be found in the PATENTS file in the same directory. */ +#import + #import #import #import @@ -17,6 +19,7 @@ AVURLAsset *_firstAsset; AVAsset *_secondAsset; NSURL *_url; + NSArray *_requestedKeys; } @end @@ -32,6 +35,7 @@ @property (atomic, readwrite) BOOL shouldBePlaying; - (void)setVideoPlaceholderImage:(UIImage *)image; +- (void)prepareToPlayAsset:(AVAsset *)asset withKeys:(NSArray *)requestedKeys; @end @@ -43,6 +47,7 @@ _firstAsset = [AVURLAsset assetWithURL:[NSURL URLWithString:@"firstURL"]]; _secondAsset = [AVAsset assetWithURL:[NSURL URLWithString:@"secondURL"]]; _url = [NSURL URLWithString:@"testURL"]; + _requestedKeys = @[ @"playable" ]; } @@ -131,23 +136,42 @@ XCTAssertNil(_videoNode.player); } -- (void)testPlayerIsCreatedInFetchData +- (void)testPlayerIsCreatedAsynchronouslyInFetchData { - _videoNode.asset = _firstAsset; + AVAsset *asset = _firstAsset; + + id assetMock = [OCMockObject partialMockForObject:asset]; + id videoNodeMock = [OCMockObject partialMockForObject:_videoNode]; + + [[[assetMock stub] andReturnValue:@YES] isPlayable]; + [[[videoNodeMock expect] andForwardToRealObject] prepareToPlayAsset:assetMock withKeys:_requestedKeys]; + + _videoNode.asset = assetMock; _videoNode.interfaceState = ASInterfaceStateFetchData; + [videoNodeMock verifyWithDelay:1.0f]; + XCTAssertNotNil(_videoNode.player); } -- (void)testPlayerIsCreatedInFetchDataWithURL +- (void)testPlayerIsCreatedAsynchronouslyInFetchDataWithURL { - _videoNode.asset = [AVAsset assetWithURL:_url]; + AVAsset *asset = [AVAsset assetWithURL:_url]; + + id assetMock = [OCMockObject partialMockForObject:asset]; + id videoNodeMock = [OCMockObject partialMockForObject:_videoNode]; + + [[[assetMock stub] andReturnValue:@YES] isPlayable]; + [[[videoNodeMock expect] andForwardToRealObject] prepareToPlayAsset:assetMock withKeys:_requestedKeys]; + + _videoNode.asset = assetMock; _videoNode.interfaceState = ASInterfaceStateFetchData; + [videoNodeMock verifyWithDelay:1.0f]; + XCTAssertNotNil(_videoNode.player); } - - (void)testPlayerLayerNodeIsAddedOnDidLoadIfVisibleAndAutoPlaying { _videoNode.asset = _firstAsset; @@ -284,13 +308,18 @@ - (void)testVideoThatDoesNotAutorepeatsShouldPauseOnPlaybackEnd { - _videoNode.asset = _firstAsset; + id assetMock = [OCMockObject partialMockForObject:_firstAsset]; + [[[assetMock stub] andReturnValue:@YES] isPlayable]; + + _videoNode.asset = assetMock; _videoNode.shouldAutorepeat = NO; [_videoNode didLoad]; [_videoNode setInterfaceState:ASInterfaceStateVisible | ASInterfaceStateDisplay | ASInterfaceStateFetchData]; [_videoNode play]; + [_videoNode prepareToPlayAsset:assetMock withKeys:_requestedKeys]; + XCTAssertTrue(_videoNode.isPlaying); [[NSNotificationCenter defaultCenter] postNotificationName:AVPlayerItemDidPlayToEndTimeNotification object:_videoNode.currentItem]; @@ -372,9 +401,20 @@ - (void)testClearingFetchedContentShouldClearAssetData { - _videoNode.asset = _firstAsset; + AVAsset *asset = _firstAsset; + + id assetMock = [OCMockObject partialMockForObject:asset]; + id videoNodeMock = [OCMockObject partialMockForObject:_videoNode]; + + [[[assetMock stub] andReturnValue:@YES] isPlayable]; + [[[videoNodeMock expect] andForwardToRealObject] prepareToPlayAsset:assetMock withKeys:_requestedKeys]; + + _videoNode.asset = assetMock; [_videoNode fetchData]; [_videoNode setVideoPlaceholderImage:[[UIImage alloc] init]]; + + [videoNodeMock verifyWithDelay:1.0f]; + XCTAssertNotNil(_videoNode.player); XCTAssertNotNil(_videoNode.currentItem); XCTAssertNotNil(_videoNode.placeholderImageNode.image); From 39ee33f21ac8f01db07f8506916b09fb86982257 Mon Sep 17 00:00:00 2001 From: Robin Chou Date: Thu, 28 Apr 2016 21:13:34 -0400 Subject: [PATCH 4/8] Update remaining tests. --- AsyncDisplayKitTests/ASVideoNodeTests.m | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/AsyncDisplayKitTests/ASVideoNodeTests.m b/AsyncDisplayKitTests/ASVideoNodeTests.m index 7464e9eb..6530edd4 100644 --- a/AsyncDisplayKitTests/ASVideoNodeTests.m +++ b/AsyncDisplayKitTests/ASVideoNodeTests.m @@ -316,9 +316,8 @@ [_videoNode didLoad]; [_videoNode setInterfaceState:ASInterfaceStateVisible | ASInterfaceStateDisplay | ASInterfaceStateFetchData]; - [_videoNode play]; - [_videoNode prepareToPlayAsset:assetMock withKeys:_requestedKeys]; + [_videoNode play]; XCTAssertTrue(_videoNode.isPlaying); @@ -330,11 +329,15 @@ - (void)testVideoThatAutorepeatsShouldRepeatOnPlaybackEnd { - _videoNode.asset = _firstAsset; + id assetMock = [OCMockObject partialMockForObject:_firstAsset]; + [[[assetMock stub] andReturnValue:@YES] isPlayable]; + + _videoNode.asset = assetMock; _videoNode.shouldAutorepeat = YES; [_videoNode didLoad]; [_videoNode setInterfaceState:ASInterfaceStateVisible | ASInterfaceStateDisplay | ASInterfaceStateFetchData]; + [_videoNode prepareToPlayAsset:assetMock withKeys:_requestedKeys]; [_videoNode play]; [[NSNotificationCenter defaultCenter] postNotificationName:AVPlayerItemDidPlayToEndTimeNotification object:_videoNode.currentItem]; @@ -344,9 +347,13 @@ - (void)testVideoResumedWhenBufferIsLikelyToKeepUp { - _videoNode.asset = _firstAsset; + id assetMock = [OCMockObject partialMockForObject:_firstAsset]; + [[[assetMock stub] andReturnValue:@YES] isPlayable]; + + _videoNode.asset = assetMock; [_videoNode setInterfaceState:ASInterfaceStateVisible | ASInterfaceStateDisplay | ASInterfaceStateFetchData]; + [_videoNode prepareToPlayAsset:assetMock withKeys:_requestedKeys]; [_videoNode pause]; _videoNode.shouldBePlaying = YES; From 5b2aa9d7392d4f3f2eefba9ca132fc7d83c44aaa Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Tue, 26 Apr 2016 16:39:44 -0700 Subject: [PATCH 5/8] Add .editorconfig --- .editorconfig | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 .editorconfig diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 00000000..0605feef --- /dev/null +++ b/.editorconfig @@ -0,0 +1,19 @@ +# http://editorconfig.org +root = true + +[*] +charset = utf-8 +end_of_line = lf +insert_final_newline = true +trim_trailing_whitespace = true + +[**.{h,cc,mm,m}] +indent_style = space +indent_size = 2 + +[*.{md,markdown}] +trim_trailing_whitespace = false + +# Makefiles always use tabs for indentation +[Makefile] +indent_style = tab \ No newline at end of file From 2032cffd907d1740ebfc320f4e784b91d9938b66 Mon Sep 17 00:00:00 2001 From: Scott Goodson Date: Wed, 27 Apr 2016 23:27:30 -0700 Subject: [PATCH 6/8] [Cocoapods, Build] Disable tvOS in podspec for now due to Cocoapods support problems. --- AsyncDisplayKit.podspec | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/AsyncDisplayKit.podspec b/AsyncDisplayKit.podspec index 72eaa2a9..cc4eff21 100644 --- a/AsyncDisplayKit.podspec +++ b/AsyncDisplayKit.podspec @@ -5,7 +5,7 @@ Pod::Spec.new do |spec| spec.homepage = 'http://asyncdisplaykit.org' spec.authors = { 'Scott Goodson' => 'scottgoodson@gmail.com', 'Ryan Nystrom' => 'rnystrom@fb.com' } spec.summary = 'Smooth asynchronous user interfaces for iOS apps.' - spec.source = { :git => 'https://github.com/facebook/AsyncDisplayKit.git', :tag => '1.9.7.3' } + spec.source = { :git => 'https://github.com/facebook/AsyncDisplayKit.git', :tag => '1.9.73' } spec.documentation_url = 'http://asyncdisplaykit.org/appledoc/' @@ -69,5 +69,6 @@ Pod::Spec.new do |spec| } spec.ios.deployment_target = '7.0' - spec.tvos.deployment_target = '9.0' +# Uncomment when fixed: The platform of the target `Pods` (tvOS 9.0) is not compatible with `PINRemoteImage/iOS (2.1.4)`, which does not support `tvos`.) during validation +# spec.tvos.deployment_target = '9.0' end From fd4d18259ea55f0c07d5acd17b7a60b12ae9efa8 Mon Sep 17 00:00:00 2001 From: Wendy Date: Fri, 29 Apr 2016 22:30:56 -0700 Subject: [PATCH 7/8] Use dispatch_async for setting currentImageQuality to ensure current order --- AsyncDisplayKit/ASNetworkImageNode.mm | 51 ++++++++++++++------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/AsyncDisplayKit/ASNetworkImageNode.mm b/AsyncDisplayKit/ASNetworkImageNode.mm index 8ff2be7c..27a677fe 100755 --- a/AsyncDisplayKit/ASNetworkImageNode.mm +++ b/AsyncDisplayKit/ASNetworkImageNode.mm @@ -122,17 +122,15 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _URL = URL; - if (reset || _URL == nil) { + BOOL hasURL = _URL == nil; + if (reset || hasURL) { self.image = _defaultImage; - if (_URL == nil) { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 1.0; - }); - } else { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 0.0; - }); - } + /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, + so always use a dispatch_async to ensure that we queue the operations in the correct order. + (see comment in displayDidFinish) */ + dispatch_async(dispatch_get_main_queue(), ^{ + self.currentImageQuality = hasURL ? 0.0 : 1.0; + }); } if (self.interfaceState & ASInterfaceStateFetchData) { @@ -157,15 +155,13 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _defaultImage = defaultImage; if (!_imageLoaded) { - if (_URL == nil) { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 1.0; - }); - } else { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 0.0; - }); - } + BOOL hasURL = _URL == nil; + /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, + so always use a dispatch_async to ensure that we queue the operations in the correct order. + (see comment in displayDidFinish) */ + dispatch_async(dispatch_get_main_queue(), ^{ + self.currentImageQuality = hasURL ? 0.0 : 1.0; + }); _lock.unlock(); // Locking: it is important to release _lock before entering setImage:, as it needs to release the lock before -invalidateCalculatedLayout. // If we continue to hold the lock here, it will still be locked until the next unlock() call, causing a possible deadlock with @@ -336,7 +332,7 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; return; } strongSelf.image = progressImage; - ASPerformBlockOnMainThread(^{ + dispatch_async(dispatch_get_main_queue(), ^{ strongSelf->_currentImageQuality = progress; }); }; @@ -361,7 +357,7 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; self.animatedImage = nil; self.image = _defaultImage; _imageLoaded = NO; - ASPerformBlockOnMainThread(^{ + dispatch_async(dispatch_get_main_queue(), ^{ self.currentImageQuality = 0.0; }); } @@ -445,9 +441,14 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; } } } - + _imageLoaded = YES; - self.currentImageQuality = 1.0; + /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, + so always use a dispatch_async to ensure that we queue the operations in the correct order. + (see comment in displayDidFinish) */ + dispatch_async(dispatch_get_main_queue(), ^{ + self.currentImageQuality = 1.0; + }); [_delegate imageNode:self didLoadImage:self.image]; }); } @@ -473,7 +474,9 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; } else { strongSelf.image = [imageContainer asdk_image]; } - strongSelf->_currentImageQuality = 1.0; + dispatch_async(dispatch_get_main_queue(), ^{ + strongSelf->_currentImageQuality = 1.0; + }); } strongSelf->_downloadIdentifier = nil; From b700eeb32cd0a84233b7a270bdf4b04cc2d12bc5 Mon Sep 17 00:00:00 2001 From: Robin Chou Date: Sat, 30 Apr 2016 17:22:11 -0400 Subject: [PATCH 8/8] Use `ASPerformOnMainThread` helper. `-fetchData` improvements. --- AsyncDisplayKit/ASVideoNode.mm | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/AsyncDisplayKit/ASVideoNode.mm b/AsyncDisplayKit/ASVideoNode.mm index 2df9dcf0..8db2d1d1 100644 --- a/AsyncDisplayKit/ASVideoNode.mm +++ b/AsyncDisplayKit/ASVideoNode.mm @@ -106,7 +106,7 @@ static NSString * const kStatus = @"status"; return nil; } -- (void)prepareToPlayAsset:(AVAsset *)asset withKeys:(NSArray *)requestedKeys +- (void)prepareToPlayAsset:(AVAsset *)asset withKeys:(NSArray *)requestedKeys { for (NSString *key in requestedKeys) { NSError *error = nil; @@ -122,7 +122,7 @@ static NSString * const kStatus = @"status"; } AVPlayerItem *playerItem = [self constructPlayerItem]; - self.currentItem = playerItem; + [self setCurrentItem:playerItem]; if (_player != nil) { [_player replaceCurrentItemWithPlayerItem:playerItem]; @@ -289,14 +289,13 @@ static NSString * const kStatus = @"status"; { ASDN::MutexLocker l(_videoLock); - if (_asset != nil) { - NSArray *requestedKeys = @[ @"playable" ]; - [_asset loadValuesAsynchronouslyForKeys:requestedKeys completionHandler:^{ - dispatch_async(dispatch_get_main_queue(), ^{ - [self prepareToPlayAsset:_asset withKeys:requestedKeys]; - }); - }]; - } + AVAsset *asset = self.asset; + NSArray *requestedKeys = @[ @"playable" ]; + [asset loadValuesAsynchronouslyForKeys:requestedKeys completionHandler:^{ + ASPerformBlockOnMainThread(^{ + [self prepareToPlayAsset:asset withKeys:requestedKeys]; + }); + }]; } }