From 1487f6762f64054d9de75606e07e3e8a6d555085 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Sat, 31 Aug 2019 13:09:53 +0800 Subject: [PATCH 1/5] Try to fix the SDAnimatedImageView's imageScaling and imageAligning behavior compared to the built-in NSImageView --- SDWebImage/Core/SDAnimatedImageView.m | 64 ++++++++++++--------------- 1 file changed, 29 insertions(+), 35 deletions(-) diff --git a/SDWebImage/Core/SDAnimatedImageView.m b/SDWebImage/Core/SDAnimatedImageView.m index 0adcfd61..374d2931 100644 --- a/SDWebImage/Core/SDAnimatedImageView.m +++ b/SDWebImage/Core/SDAnimatedImageView.m @@ -65,6 +65,7 @@ static NSUInteger SDDeviceFreeMemory() { #else @property (nonatomic, strong) CADisplayLink *displayLink; #endif +@property (nonatomic) CALayer *imageViewLayer; // The actual rendering layer. @end @@ -132,10 +133,6 @@ static NSUInteger SDDeviceFreeMemory() { self.shouldIncrementalLoad = YES; #if SD_MAC self.wantsLayer = YES; - // Default value from `NSImageView` - self.layerContentsRedrawPolicy = NSViewLayerContentsRedrawOnSetNeedsDisplay; - self.imageScaling = NSImageScaleProportionallyDown; - self.imageAlignment = NSImageAlignCenter; #endif #if SD_UIKIT [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(didReceiveMemoryWarning:) name:UIApplicationDidReceiveMemoryWarningNotification object:nil]; @@ -194,7 +191,7 @@ static NSUInteger SDDeviceFreeMemory() { - (void)setImage:(UIImage *)image { - if (self.image == image) { + if (super.image == image) { return; } @@ -246,11 +243,8 @@ static NSUInteger SDDeviceFreeMemory() { if (self.shouldAnimate) { [self startAnimating]; } - - [self.layer setNeedsDisplay]; -#if SD_MAC - [self.layer displayIfNeeded]; // macOS's imageViewLayer may not equal to self.layer. But `[super setImage:]` will impliedly mark it needsDisplay. We call `[self.layer displayIfNeeded]` to immediately refresh the imageViewLayer to avoid flashing -#endif + + [self.imageViewLayer setNeedsDisplay]; } } @@ -555,7 +549,7 @@ static NSUInteger SDDeviceFreeMemory() { // We must use `image.class conformsToProtocol:` instead of `image conformsToProtocol:` here // Because UIKit on macOS, using internal hard-coded override method, which returns NO if ([image.class conformsToProtocol:@protocol(SDAnimatedImage)] && image.sd_isIncremental) { - UIImage *previousImage = self.image; + UIImage *previousImage = super.image; if ([previousImage.class conformsToProtocol:@protocol(SDAnimatedImage)] && previousImage.sd_isIncremental) { NSData *previousData = [((UIImage *)previousImage) animatedImageData]; NSData *currentData = [((UIImage *)image) animatedImageData]; @@ -643,7 +637,7 @@ static NSUInteger SDDeviceFreeMemory() { self.currentFrame = currentFrame; self.currentFrameIndex = nextFrameIndex; self.bufferMiss = NO; - [self.layer setNeedsDisplay]; + [self.imageViewLayer setNeedsDisplay]; } else { self.bufferMiss = YES; } @@ -714,42 +708,42 @@ static NSUInteger SDDeviceFreeMemory() { - (void)displayLayer:(CALayer *)layer { - if (_currentFrame) { + if (self.currentFrame) { layer.contentsScale = self.animatedImageScale; - layer.contents = (__bridge id)_currentFrame.CGImage; + layer.contents = (__bridge id)self.currentFrame.CGImage; } } #if SD_MAC -// Layer-backed NSImageView optionally optimize to use a subview to do actual layer rendering. -// When the optimization is turned on, it calls `updateLayer` instead of `displayLayer:` to update subview's layer. -// When the optimization it turned off, this return nil and calls `displayLayer:` directly. -- (CALayer *)imageViewLayer { - NSView *imageView = imageView = objc_getAssociatedObject(self, NSSelectorFromString(@"_imageView")); +// NSImageView use a subview. We need this subview's layer for actual rendering. +// Why using this design may because of properties like `imageAlignment` and `imageScaling`, which it's not available for UIImageView.contentMode (it's impossible to align left and keep aspect ratio at the same time) +- (NSView *)imageView { + NSImageView *imageView = imageView = objc_getAssociatedObject(self, NSSelectorFromString(@"_imageView")); if (!imageView) { // macOS 10.14 imageView = objc_getAssociatedObject(self, NSSelectorFromString(@"_imageSubview")); } - return imageView.layer; + return imageView; } -- (void)updateLayer -{ - if (_currentFrame) { - [self displayLayer:self.imageViewLayer]; - } else { - [super updateLayer]; +// on macOS, it's the imageView subview's layer (we use layer-hosting view to let CALayerDelegate works) +- (CALayer *)imageViewLayer { + NSView *imageView = self.imageView; + if (!imageView) { + return nil; } + if (!_imageViewLayer) { + _imageViewLayer = [CALayer new]; + _imageViewLayer.delegate = self; + imageView.layer = _imageViewLayer; + imageView.wantsLayer = YES; + } + return _imageViewLayer; } - -- (BOOL)wantsUpdateLayer { - // AppKit is different from UIKit, it need extra check before the layer is updated - // When we use the custom animation, the layer.setNeedsDisplay is directly called from display link (See `displayDidRefresh:`). However, for normal image rendering, we must implements and return YES to mark it need display - if (_currentFrame) { - return NO; - } else { - return YES; - } +#else +// on iOS, it's the imageView itself's layer +- (CALayer *)imageViewLayer { + return self.layer; } #endif From 9c28ce40a9baf8e5142f14693460bb2170fde4be Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Sat, 31 Aug 2019 23:23:47 +0800 Subject: [PATCH 2/5] Update the Example for Appkit using the aligning and scaling layout --- Examples/SDWebImage OSX Demo/ViewController.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Examples/SDWebImage OSX Demo/ViewController.m b/Examples/SDWebImage OSX Demo/ViewController.m index 9463d81d..44a76766 100644 --- a/Examples/SDWebImage OSX Demo/ViewController.m +++ b/Examples/SDWebImage OSX Demo/ViewController.m @@ -41,6 +41,8 @@ [self.imageView3 sd_setImageWithURL:[NSURL URLWithString:@"https://nr-platform.s3.amazonaws.com/uploads/platform/published_extension/branding_icon/275/AmazonS3.png"]]; // SDAnimatedImageView + Animated Image self.imageView4.sd_imageTransition = SDWebImageTransition.fadeTransition; + self.imageView4.imageScaling = NSImageScaleProportionallyUpOrDown; + self.imageView4.imageAlignment = NSImageAlignLeft; // supports NSImageView's layout properties [self.imageView4 sd_setImageWithURL:[NSURL URLWithString:@"http://littlesvr.ca/apng/images/SteamEngine.webp"] placeholderImage:nil options:SDWebImageForceTransition]; self.clearCacheButton.target = self; From 031f21a18d83f26c67139eb3e7efcbe2430e0db3 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 2 Sep 2019 13:27:17 +0800 Subject: [PATCH 3/5] Fix the test case by using the same lock in SDAnimatedImageView to avoid thread-safe issue --- Tests/Tests/SDAnimatedImageTest.m | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Tests/Tests/SDAnimatedImageTest.m b/Tests/Tests/SDAnimatedImageTest.m index cc04cd0d..3134df5c 100644 --- a/Tests/Tests/SDAnimatedImageTest.m +++ b/Tests/Tests/SDAnimatedImageTest.m @@ -8,6 +8,7 @@ */ #import "SDTestCase.h" +#import "SDInternalMacros.h" #import static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop count @@ -17,6 +18,8 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun @property (nonatomic, assign) BOOL isProgressive; @property (nonatomic, strong) NSMutableDictionary *frameBuffer; +@property (nonatomic, strong) dispatch_semaphore_t lock; + @end @@ -316,8 +319,10 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.5 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ // 0.5s is not finished, frame index should not be 0 + SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).beGreaterThan(0); expect(imageView.currentFrameIndex).beGreaterThan(0); + SD_UNLOCK(imageView.lock); }); dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ @@ -326,8 +331,10 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun #else imageView.animates = NO; #endif + SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).beGreaterThan(0); expect(imageView.currentFrameIndex).beGreaterThan(0); + SD_UNLOCK(imageView.lock); [imageView removeFromSuperview]; [expectation fulfill]; @@ -354,8 +361,10 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.5 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ // 0.5s is not finished, frame index should not be 0 + SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).beGreaterThan(0); expect(imageView.currentFrameIndex).beGreaterThan(0); + SD_UNLOCK(imageView.lock); }); dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ @@ -364,8 +373,10 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun #else imageView.animates = NO; #endif + SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).equal(0); expect(imageView.currentFrameIndex).equal(0); + SD_UNLOCK(imageView.lock); [imageView removeFromSuperview]; [expectation fulfill]; From eb05b9f4fc0cf98b142869b17ce0d5b87691b0d4 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Wed, 4 Sep 2019 12:01:37 +0800 Subject: [PATCH 4/5] Revert "Fix the test case by using the same lock in SDAnimatedImageView to avoid thread-safe issue" This reverts commit 031f21a18d83f26c67139eb3e7efcbe2430e0db3. --- Tests/Tests/SDAnimatedImageTest.m | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/Tests/Tests/SDAnimatedImageTest.m b/Tests/Tests/SDAnimatedImageTest.m index 3134df5c..cc04cd0d 100644 --- a/Tests/Tests/SDAnimatedImageTest.m +++ b/Tests/Tests/SDAnimatedImageTest.m @@ -8,7 +8,6 @@ */ #import "SDTestCase.h" -#import "SDInternalMacros.h" #import static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop count @@ -18,8 +17,6 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun @property (nonatomic, assign) BOOL isProgressive; @property (nonatomic, strong) NSMutableDictionary *frameBuffer; -@property (nonatomic, strong) dispatch_semaphore_t lock; - @end @@ -319,10 +316,8 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.5 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ // 0.5s is not finished, frame index should not be 0 - SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).beGreaterThan(0); expect(imageView.currentFrameIndex).beGreaterThan(0); - SD_UNLOCK(imageView.lock); }); dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ @@ -331,10 +326,8 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun #else imageView.animates = NO; #endif - SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).beGreaterThan(0); expect(imageView.currentFrameIndex).beGreaterThan(0); - SD_UNLOCK(imageView.lock); [imageView removeFromSuperview]; [expectation fulfill]; @@ -361,10 +354,8 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.5 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ // 0.5s is not finished, frame index should not be 0 - SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).beGreaterThan(0); expect(imageView.currentFrameIndex).beGreaterThan(0); - SD_UNLOCK(imageView.lock); }); dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ @@ -373,10 +364,8 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun #else imageView.animates = NO; #endif - SD_LOCK(imageView.lock); expect(imageView.frameBuffer.count).equal(0); expect(imageView.currentFrameIndex).equal(0); - SD_UNLOCK(imageView.lock); [imageView removeFromSuperview]; [expectation fulfill]; From bba155421a4e9cb619c2b8aa59211b9a25df69d1 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Wed, 4 Sep 2019 17:55:09 +0800 Subject: [PATCH 5/5] Revert the changes to use super.image, make it still possible for subclassing and override --- SDWebImage/Core/SDAnimatedImageView.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SDWebImage/Core/SDAnimatedImageView.m b/SDWebImage/Core/SDAnimatedImageView.m index a4c59c09..0d3ea034 100644 --- a/SDWebImage/Core/SDAnimatedImageView.m +++ b/SDWebImage/Core/SDAnimatedImageView.m @@ -191,7 +191,7 @@ static NSUInteger SDDeviceFreeMemory() { - (void)setImage:(UIImage *)image { - if (super.image == image) { + if (self.image == image) { return; } @@ -549,7 +549,7 @@ static NSUInteger SDDeviceFreeMemory() { // We must use `image.class conformsToProtocol:` instead of `image conformsToProtocol:` here // Because UIKit on macOS, using internal hard-coded override method, which returns NO if ([image.class conformsToProtocol:@protocol(SDAnimatedImage)] && image.sd_isIncremental) { - UIImage *previousImage = super.image; + UIImage *previousImage = self.image; if ([previousImage.class conformsToProtocol:@protocol(SDAnimatedImage)] && previousImage.sd_isIncremental) { NSData *previousData = [((UIImage *)previousImage) animatedImageData]; NSData *currentData = [((UIImage *)image) animatedImageData];