From 64c5ff59a31ea054b15878464630c7454012b661 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 8 Jan 2024 19:33:16 +0800 Subject: [PATCH 1/3] Fix the behavior that query thumbnail from full size data does not sync back the thumbnail image into memory cache The caller should not passes the `thumbnail` related context when querying the actual thumbnail key (calculate twice) Now we always do sync to match previous behavior before 5.13.0 --- SDWebImage/Core/SDImageCache.m | 75 +++++++++++++++++------------ SDWebImage/Core/SDWebImageManager.m | 12 ++++- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/SDWebImage/Core/SDImageCache.m b/SDWebImage/Core/SDImageCache.m index ac6097b2..edab8f0a 100644 --- a/SDWebImage/Core/SDImageCache.m +++ b/SDWebImage/Core/SDImageCache.m @@ -15,6 +15,7 @@ #import "UIImage+Metadata.h" #import "UIImage+ExtendedCacheData.h" #import "SDCallbackQueue.h" +#import "SDImageTransformer.h" // TODO, remove this @interface SDImageCacheToken () @@ -430,22 +431,9 @@ static NSString * _defaultDiskCacheDirectory; SDImageCacheType cacheType = [context[SDWebImageContextStoreCacheType] integerValue]; shouldCacheToMomery = (cacheType == SDImageCacheTypeAll || cacheType == SDImageCacheTypeMemory); } - CGSize thumbnailSize = CGSizeZero; - NSValue *thumbnailSizeValue = context[SDWebImageContextImageThumbnailPixelSize]; - if (thumbnailSizeValue != nil) { -#if SD_MAC - thumbnailSize = thumbnailSizeValue.sizeValue; -#else - thumbnailSize = thumbnailSizeValue.CGSizeValue; -#endif - } - if (thumbnailSize.width > 0 && thumbnailSize.height > 0) { - // Query full size cache key which generate a thumbnail, should not write back to full size memory cache - shouldCacheToMomery = NO; - } - if (shouldCacheToMomery && diskImage && self.config.shouldCacheImagesInMemory) { - NSUInteger cost = diskImage.sd_memoryCost; - [self.memoryCache setObject:diskImage forKey:key cost:cost]; + if (shouldCacheToMomery) { + // check if we need sync logic + [self _syncDiskToMemoryWithImage:diskImage forKey:key]; } return diskImage; @@ -526,6 +514,42 @@ static NSString * _defaultDiskCacheDirectory; return image; } +- (void)_syncDiskToMemoryWithImage:(UIImage *)diskImage forKey:(NSString *)key { + // earily check + if (!self.config.shouldCacheImagesInMemory) { + return; + } + if (!diskImage) { + return; + } + // The disk -> memory sync logic, which should only store thumbnail image with thumbnail key + // However, caller (like SDWebImageManager) will query full key, with thumbnail size, and get thubmnail image + // We should add a check here, currently it's a hack + if (diskImage.sd_isThumbnail) { + SDImageCoderOptions *options = diskImage.sd_decodeOptions; + CGSize thumbnailSize = CGSizeZero; + NSValue *thumbnailSizeValue = options[SDImageCoderDecodeThumbnailPixelSize]; + if (thumbnailSizeValue != nil) { + #if SD_MAC + thumbnailSize = thumbnailSizeValue.sizeValue; + #else + thumbnailSize = thumbnailSizeValue.CGSizeValue; + #endif + } + BOOL preserveAspectRatio = YES; + NSNumber *preserveAspectRatioValue = options[SDImageCoderDecodePreserveAspectRatio]; + if (preserveAspectRatioValue != nil) { + preserveAspectRatio = preserveAspectRatioValue.boolValue; + } + // Calculate the actual thumbnail key + NSString *thumbnailKey = SDThumbnailedKeyForKey(key, thumbnailSize, preserveAspectRatio); + // Override the sync key + key = thumbnailKey; + } + NSUInteger cost = diskImage.sd_memoryCost; + [self.memoryCache setObject:diskImage forKey:key cost:cost]; +} + - (void)_unarchiveObjectWithImage:(UIImage *)image forKey:(NSString *)key { if (!image || !key) { return; @@ -655,19 +679,6 @@ static NSString * _defaultDiskCacheDirectory; SDImageCacheType cacheType = [context[SDWebImageContextStoreCacheType] integerValue]; shouldCacheToMomery = (cacheType == SDImageCacheTypeAll || cacheType == SDImageCacheTypeMemory); } - CGSize thumbnailSize = CGSizeZero; - NSValue *thumbnailSizeValue = context[SDWebImageContextImageThumbnailPixelSize]; - if (thumbnailSizeValue != nil) { - #if SD_MAC - thumbnailSize = thumbnailSizeValue.sizeValue; - #else - thumbnailSize = thumbnailSizeValue.CGSizeValue; - #endif - } - if (thumbnailSize.width > 0 && thumbnailSize.height > 0) { - // Query full size cache key which generate a thumbnail, should not write back to full size memory cache - shouldCacheToMomery = NO; - } // Special case: If user query image in list for the same URL, to avoid decode and write **same** image object into disk cache multiple times, we query and check memory cache here again. if (shouldCacheToMomery && self.config.shouldCacheImagesInMemory) { diskImage = [self.memoryCache objectForKey:key]; @@ -675,9 +686,9 @@ static NSString * _defaultDiskCacheDirectory; // decode image data only if in-memory cache missed if (!diskImage) { diskImage = [self diskImageForKey:key data:diskData options:options context:context]; - if (shouldCacheToMomery && diskImage && self.config.shouldCacheImagesInMemory) { - NSUInteger cost = diskImage.sd_memoryCost; - [self.memoryCache setObject:diskImage forKey:key cost:cost]; + // check if we need sync logic + if (shouldCacheToMomery) { + [self _syncDiskToMemoryWithImage:diskImage forKey:key]; } } } diff --git a/SDWebImage/Core/SDWebImageManager.m b/SDWebImage/Core/SDWebImageManager.m index d2ba07a9..d533c79e 100644 --- a/SDWebImage/Core/SDWebImageManager.m +++ b/SDWebImage/Core/SDWebImageManager.m @@ -302,8 +302,18 @@ static id _defaultImageLoader; if (shouldQueryCache) { // transformed cache key NSString *key = [self cacheKeyForURL:url context:context]; + // to avoid the SDImageCache's sync logic use the mismatched cache key + // we should strip the `thumbnail` related context + SDWebImageMutableContext *mutableContext; + if (context) { + mutableContext = [context mutableCopy]; + } else { + mutableContext = [NSMutableDictionary dictionary]; + } + mutableContext[SDWebImageContextImageThumbnailPixelSize] = nil; + mutableContext[SDWebImageContextImagePreserveAspectRatio] = nil; @weakify(operation); - operation.cacheOperation = [imageCache queryImageForKey:key options:options context:context cacheType:queryCacheType completion:^(UIImage * _Nullable cachedImage, NSData * _Nullable cachedData, SDImageCacheType cacheType) { + operation.cacheOperation = [imageCache queryImageForKey:key options:options context:mutableContext cacheType:queryCacheType completion:^(UIImage * _Nullable cachedImage, NSData * _Nullable cachedData, SDImageCacheType cacheType) { @strongify(operation); if (!operation || operation.isCancelled) { // Image combined operation cancelled by user From 0092f74b7fdacb479f89bde518e690516981ede4 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 8 Jan 2024 19:58:15 +0800 Subject: [PATCH 2/3] Added the assert to check the current behavior that thumbnail key should not passed twice --- SDWebImage/Core/SDImageCache.m | 9 +++++++++ SDWebImage/Core/SDWebImageManager.m | 7 +------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/SDWebImage/Core/SDImageCache.m b/SDWebImage/Core/SDImageCache.m index edab8f0a..c188adc3 100644 --- a/SDWebImage/Core/SDImageCache.m +++ b/SDWebImage/Core/SDImageCache.m @@ -17,6 +17,14 @@ #import "SDCallbackQueue.h" #import "SDImageTransformer.h" // TODO, remove this +// TODO, remove this +static BOOL SDIsThumbnailKey(NSString *key) { + if ([key rangeOfString:@"-Thumbnail("].location != NSNotFound) { + return YES; + } + return NO; +} + @interface SDImageCacheToken () @property (nonatomic, strong, nullable, readwrite) NSString *key; @@ -526,6 +534,7 @@ static NSString * _defaultDiskCacheDirectory; // However, caller (like SDWebImageManager) will query full key, with thumbnail size, and get thubmnail image // We should add a check here, currently it's a hack if (diskImage.sd_isThumbnail) { + NSAssert(!SDIsThumbnailKey(key), @"The input cache key %@ should not be thumbnail key", key); SDImageCoderOptions *options = diskImage.sd_decodeOptions; CGSize thumbnailSize = CGSizeZero; NSValue *thumbnailSizeValue = options[SDImageCoderDecodeThumbnailPixelSize]; diff --git a/SDWebImage/Core/SDWebImageManager.m b/SDWebImage/Core/SDWebImageManager.m index d533c79e..2ddceabb 100644 --- a/SDWebImage/Core/SDWebImageManager.m +++ b/SDWebImage/Core/SDWebImageManager.m @@ -304,12 +304,7 @@ static id _defaultImageLoader; NSString *key = [self cacheKeyForURL:url context:context]; // to avoid the SDImageCache's sync logic use the mismatched cache key // we should strip the `thumbnail` related context - SDWebImageMutableContext *mutableContext; - if (context) { - mutableContext = [context mutableCopy]; - } else { - mutableContext = [NSMutableDictionary dictionary]; - } + SDWebImageMutableContext *mutableContext = [context mutableCopy]; mutableContext[SDWebImageContextImageThumbnailPixelSize] = nil; mutableContext[SDWebImageContextImagePreserveAspectRatio] = nil; @weakify(operation); From 9ae93e54568e989ac289010d4a8455112f29ee87 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Tue, 9 Jan 2024 17:04:55 +0800 Subject: [PATCH 3/3] Update the test case for new behavior --- Tests/Tests/SDWebImageManagerTests.m | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Tests/Tests/SDWebImageManagerTests.m b/Tests/Tests/SDWebImageManagerTests.m index a35231f1..c57285a3 100644 --- a/Tests/Tests/SDWebImageManagerTests.m +++ b/Tests/Tests/SDWebImageManagerTests.m @@ -482,11 +482,15 @@ [SDImageCache.sharedImageCache queryCacheOperationForKey:fullSizeKey options:0 context:@{SDWebImageContextImageThumbnailPixelSize : @(thumbnailSize)} done:^(UIImage * _Nullable image, NSData * _Nullable data, SDImageCacheType cacheType) { expect(image.size).equal(thumbnailSize); expect(cacheType).equal(SDImageCacheTypeDisk); - // Currently, thumbnail decoding does not write back to the original key's memory cache - // But this may change in the future once I change the API for `SDImageCacheProtocol` - expect([SDImageCache.sharedImageCache imageFromMemoryCacheForKey:fullSizeKey]).beNil(); - expect([SDImageCache.sharedImageCache imageFromMemoryCacheForKey:thumbnailKey]).beNil(); + // Check the full image should not be in memory cache (because we have only full data + thumbnail image) + // Check the thumbnail image should be in memory cache (because we have only full data + thumbnail image) + expect([SDImageCache.sharedImageCache imageFromMemoryCacheForKey:fullSizeKey].size).equal(CGSizeZero); + expect([SDImageCache.sharedImageCache imageFromDiskCacheForKey:fullSizeKey]).notTo.beNil(); + expect([SDImageCache.sharedImageCache imageFromMemoryCacheForKey:thumbnailKey].size).equal(thumbnailSize); + expect([SDImageCache.sharedImageCache imageFromDiskCacheForKey:thumbnailKey]).beNil(); + [SDImageCache.sharedImageCache removeImageFromDiskForKey:fullSizeKey]; + [SDImageCache.sharedImageCache removeImageFromMemoryForKey:thumbnailKey]; [expectation fulfill]; }];