Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDWebImage 5.17.0 iOS 16 SDImageIOAnimatedCoder createFrameAtIndex: still crashes EXC_BAD_ACCESS #3612

Open
2 of 3 tasks
LeoTheCatMeow opened this issue Sep 27, 2023 · 22 comments
Labels
Milestone

Comments

@LeoTheCatMeow
Copy link

LeoTheCatMeow commented Sep 27, 2023

New Issue Checklist

Issue Info

Info Value
Platform Name iOS
Platform Version 16.5.1
SDWebImage Version 5.17.0
Integration Method SwiftPM
Xcode Version Xcode 15
Repro rate sometimes (found in Xcode Organizer crashlogs)
Repro with our demo prj sorry haven't tried
Demo project link -

Issue Description and Steps

Hi we have an iOS app that uses SDWebImage for animated images. We use SDAnimatedImageView + SDAnimatedImage to display. Project is written in Swift and uses a mix of SwiftUI and UIKit. For loading, .scaleDownLargeImages is always passed. But context option "SDImageCoderDecodeUseLazyDecoding" is not passed. I find that setting SDImageCoderDecodeUseLazyDecoding = true causes noticeable lag when scrolling, so I prefer to not set it to true. However, in Xcode Organizer, we see several crash logs like so:

0   libsystem_platform.dylib      	0x000000020a0fbe14 _platform_memmove + 84
1   ImageIO                       	0x00000001c51591e0 GIFBufferInfo::GIFBufferInfo(unsigned char*, bool, unsigned int, unsigned int, unsigned int) + 88 (GIF_common.cpp:27)
2   ImageIO                       	0x00000001c5169f50 std::__1::__shared_ptr_emplace<GIFBufferInfo, std::__1::allocator<GIFBufferInfo> >::__shared_ptr_emplace[abi:v15006]<unsigned char*&, bool, unsigned int&, unsigned int&, unsigned int>(std::__1::all... + 56 (shared_ptr.h:294)
3   ImageIO                       	0x00000001c5169ee4 std::__1::shared_ptr<GIFBufferInfo> std::__1::allocate_shared[abi:v15006]<GIFBufferInfo, std::__1::allocator<GIFBufferInfo>, unsigned char*&, bool, unsigned int&, unsigned int&, unsigned int, void>... + 84 (shared_ptr.h:953)
4   ImageIO                       	0x00000001c51696c0 GIFReadPlugin::copyImageBlockSet(InfoRec*, CGImageProvider*, CGRect, CGSize, __CFDictionary const*) + 872 (GIF_readPlugin.cpp:1520)
5   ImageIO                       	0x00000001c4fa4b70 IIO_Reader::CopyImageBlockSetProc(void*, CGImageProvider*, CGRect, CGSize, __CFDictionary const*) + 228 (IIOReader.cpp:1454)
6   ImageIO                       	0x00000001c4fa08d4 IIOImageProviderInfo::copyImageBlockSetWithOptions(CGImageProvider*, CGRect, CGSize, __CFDictionary const*) + 740 (CGImagePlus.cpp:2326)
7   ImageIO                       	0x00000001c4fa8430 IIOImageProviderInfo::CopyImageBlockSetWithOptions(void*, CGImageProvider*, CGRect, CGSize, __CFDictionary const*) + 840 (CGImagePlus.cpp:2638)
8   CoreGraphics                  	0x00000001c1caafe0 imageProvider_retain_data + 88 (CGDataProviderImageProvider.c:91)
9   CoreGraphics                  	0x00000001c1cc8530 CGDataProviderRetainData + 84 (CGDataProvider.c:922)
10  CoreGraphics                  	0x00000001c1ce4b98 provider_for_destination_retain_data + 24 (CGDataProviderForDestination.c:705)
11  CoreGraphics                  	0x00000001c1cc8530 CGDataProviderRetainData + 84 (CGDataProvider.c:922)
12  CoreGraphics                  	0x00000001c1ca7ed8 CGAccessSessionCreate + 104 (CGAccessSession.c:71)
13  CoreGraphics                  	0x00000001c1ca28d0 get_access_session + 48 (CGSImage.c:488)
14  CoreGraphics                  	0x00000001c1cc9bf4 img_raw_read + 944 (CGSImage.c:627)
15  CoreGraphics                  	0x00000001c1cb841c img_interpolate_read + 588 (CGSImage.c:2547)
16  CoreGraphics                  	0x00000001c1c76a80 img_data_lock + 8748 (CGSImage.c:5553)
17  CoreGraphics                  	0x00000001c1cb99ec CGSImageDataLock + 1324 (CGSImage.c:5847)
18  CoreGraphics                  	0x00000001c1c8d12c ripc_AcquireRIPImageData + 708 (RIPImage.c:337)
19  CoreGraphics                  	0x00000001c1caa230 ripc_DrawImage + 820 (RIPContext.c:1432)
20  CoreGraphics                  	0x00000001c1c811dc CGContextDrawImageWithOptions + 1112 (CGContextImage.c:303)
21  ImageIO                       	0x00000001c4fbfff0 CGImageCreateCopyWithParametersNew(CGImage*, CGColor*, CGAffineTransform, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, CGColorSpace*, unsigned int, bool, CGColorRender... + 1640 (CGImageCopy.cpp:784)
22  ImageIO                       	0x00000001c4fac560 IIOImageSource::createThumbnailAtIndex(unsigned long, IIODictionary*) + 2692 (CGImageSource.cpp:2192)
23  ImageIO                       	0x00000001c4f98844 CGImageSourceCreateThumbnailAtIndex + 388 (CGImageSource.cpp:4269)
24  <App Name>                    	0x0000000102973428 +[SDImageIOAnimatedCoder createFrameAtIndex:source:scale:preserveAspectRatio:thumbnailSize:lazyDecode:animatedImage:] + 820 (SDImageIOAnimatedCoder.m:269)
25  <App Name>                    	0x0000000102973a9c -[SDImageIOAnimatedCoder decodedImageWithData:options:] + 1032 (SDImageIOAnimatedCoder.m:449)
26  <App Name>                    	0x0000000102972004 -[SDImageCodersManager decodedImageWithData:options:] + 276 (SDImageCodersManager.m:109)
27  <App Name>                    	0x000000010296ce4c SDImageCacheDecodeImageData + 396 (SDImageCacheDefine.m:123)
28  <App Name>                    	0x000000010296a2ac -[SDImageCache diskImageForKey:data:options:context:] + 112 (SDImageCache.m:524)
29  <App Name>                    	0x000000010296abe0 __73-[SDImageCache queryCacheOperationForKey:options:context:cacheType:done:]_block_invoke.161 + 420 (SDImageCache.m:677)
30  <App Name>                    	0x000000010296ae74 __73-[SDImageCache queryCacheOperationForKey:options:context:cacheType:done:]_block_invoke.164 + 68 (SDImageCache.m:701)

There are not many threads created, only 9 threads are found in the crash log. This crash is EXC_BAD_ACCESS (SIGSEGV), and after looking around for similar issues, this looks like some kind of out of memory crash? My question is what is the best approach right now to avoid this kind of problem, besides trying SDImageCoderDecodeUseLazyDecoding = true ? We are using a fairly new version of SDWebImage as well (5.17.0), and I see several issues in the past attempting to address or fix similar crash issues, yet it still seems to happen. The images are loaded in either a SwiftUI List or UIKit UICollectionView, it is common for several animated images to appear together (but they are not the same image), these animated images are user uploaded, so we have no size guarantee. My guess is that some of the animated images are very big, causing out of memory when decoding. Is there some approach I can take to better limit the image size, in order to avoid crashes like this? Thanks in advance!

@dreampiggy
Copy link
Contributor

dreampiggy commented Sep 28, 2023

AnimatedImage do still support thumbnail decoding, so you can use "thumbnailPixelSize" and pass the image view or render size

For example, your image list is 1000x1000, even a user uploaded huge animted image is 8K, it still only occupy 4MB each frame (1000x1000x4 bytesPerPixel)

@LeoTheCatMeow
Copy link
Author

@dreampiggy Sorry I haven't fully understood everything. By "thumbnailPixelSize" you mean the "SDImageCoderDecodeThumbnailPixelSize" option under SDImageCoderOption? I see it accepts a CGSize, how would I pass the image view to it? How does thumbnail decoding work? If I pass a valid size for thumbnailPixelSize, does SDWebImage only return the image as thumbnail and never the full image? I use SDWebImageManager.loadImageWithURL to load most of my images (animated and static) and then manually set the image with animation (SwiftUI) or transition in UIImageView / SDAnimatedImageView. I searched other issues and someone also mentioned the "maxBufferSize" property on SDAnimatedImagePlayer. If I limited the bufferSize here to something small, such as 2MB, would this help alleviate potential crashes? Thanks again

@dreampiggy
Copy link
Contributor

dreampiggy commented Sep 28, 2023

I see it accepts a CGSize, how would I pass the image view to it?

From newer version, you can always pass decode options to coder and do customization, don't need the same naming context arg (There are one SDWebImageContextImageThumbnailPixelSize, same name)

let decodeOptions: [SDImageCoderOption: Any] = [.decodeThumbnailPixelSize: CGSize(width: 1000, height: 1000)]
let context: [SDWebImageContext: Any] = [.imageDecodeOptions: decodeOptions]
imageView.sd_setImage(with: url, placeholderImage: nil, options: [], context: context, progress:nil, completed: nil)

This design is better, since actually decoder is plugin, which means, not written by me or SDWebImage, it can written by you and provide new option wo don't know. Pass that raw dictionary directly to decoder is correct design.

or

let context: [SDWebImageContext: Any] = [.imageThumbnailPixelSize: CGSize(width: 1000, height: 1000)]
imageView.sd_setImage(with: url, placeholderImage: nil, options: [], context: context, progress:nil, completed: nil)

This only supports some pre-defined context option which passthrough to decoder options (but not extensible)

@dreampiggy
Copy link
Contributor

does SDWebImage only return the image as thumbnail and never the full image?

It will return thumbnail image, and nil data. But by defaults, the data is stored into disk cache, thumbnail image is stored into memory cache. This behavior can also be controlled by advanced context arg .storeCacheType, .originalStoreCacheType two, see documentation there

@dreampiggy
Copy link
Contributor

dreampiggy commented Sep 28, 2023

If I limited the bufferSize here to something small, such as 2MB, would this help alleviate potential crashes? Thanks again

Maybe, but this limit is per-image-view-level, right ? If you have 100 image view (which means, you use SwiftUI.List/UIScrollView, or some non-lazy no re-using cell), you keep all image view into memory and consume RAM even that image view is not visible.

For this case, we have a special option called clearBufferWhenStopped (defaults to NO, I doubt this is a wrong design), because when the image view is not visible, the image view will be sent a stopAnimating call and get stopped. Then it will clear temp buffer.

Another solution, it's strongly recommaned to use the LazyVStack or UITableView/UICollectionView instead, which is lazy and cell-reusing. So actually even you have a list of 100 GIF, in the RAM there are only SDAnimatedImageView living, the temp buffer is guranteed to < 5 GIF images all frames

@LeoTheCatMeow
Copy link
Author

LeoTheCatMeow commented Sep 28, 2023

@dreampiggy Thanks for the super detailed replies! They are incredibly helpful!

Yes we are not using a ScrollView + VStack, we don't even use LazyVStack, we always use List (which is backed by UICollectionView on iOS 16 and UITableView on iOS 15, we support min iOS15) or just UIKit UICollectionView, so we have plenty of cell re-use. I think in SwiftUI's case they don't re-use the cell, they just destroy and remake. Anyways the memory should be cleared and limited to maybe less than 10 GIF images at all times. I personally have never encountered the crash so I am surprised to see them from the organizer. If I don't have that many animated images, yet it still crashes, I can only assume it is because of some really big or really long GIF images? In that case if I take your suggestion of using thumbnail decoding, with a limit of maybe 1000 x 1000, it should help with "Big images" i think? And then by using the maxBufferSize, it could help with "Long images" then? The buffer is kept on the SDAnimatedImageView right? If SwiftUI deallocates the view, the buffer should be released I think? For UIKit's case, when I reuse a SDAnimatedImageView and load in a new GIF, will the previous buffer be cleared? Do I still need to pass "clearBufferWhenStopped" just in case? And lastly sorry for so many questions XD, just out of curiosity, if i need to get the full size image, not the thumbnail, I just need to make another load image call without the thumbnail size context option right?

@dreampiggy
Copy link
Contributor

dreampiggy commented Sep 28, 2023

with a limit of maybe 1000 x 1000, it should help with "Big images" i think

Yes, animted image = frame count * bytes per frame (= bytesPerPixel * pixel count)

And then by using the maxBufferSize, it could help with "Long images" then

Yes, but pay attention this is not global control but for current image view

If SwiftUI deallocates the view, the buffer should be released I think

Yes, then buffer is just a NSMutableDictionary hosted in SDAnimatedPlayer which retained by SDAnimatedImageView

will the previous buffer be cleared

Yes, a new setImage: call cause that SDAnimatedImagePlayer been dealloced, so as buffer NSMutableDictionary

Do I still need to pass "clearBufferWhenStopped" just in case

Maybe. A OOM may not always been that case "total RAM usage is high", a rapid memory peak may still cause OOM. This can help for that

if i need to get the full size image, not the thumbnail, I just need to make another load image call without the thumbnail size context option right?

Yes. Because I told that thumnail by defaults store full image data into disk cache(actually no thumnail data is stored into disk, only thumnail image object into memory). When you query without thumnail it will hit disk cache async less than 1 second

@LeoTheCatMeow
Copy link
Author

Great. Thanks a lot. I have a clear picture now. I will try all these techniques. I think i am supposed to close the ticket now? I'll go do that.

@LeoTheCatMeow
Copy link
Author

@dreampiggy Sorry to bother again. After some detail checking, I found that most of the GIFs we encounter are really long, hundreds of frames, their sizes however are way below 1000 x 1000. I have applied clearBufferWhenStopped = true and a max buffer size of 10 MB for all animated image views. But I notice something strange. When I open a navigation page containing a lot of these animated images, the memory usage goes from 100 MB to somewhere around 200 MB. As I scroll i can confirm that memory is being released and allocated back and forth, and under good control. However when I push another navigation page onto the stack, triggering window to become nil for all the animated images (can confirm window becomes nil and animation stops), the memory usage displayed by Xcode is still around 200 MB, CPU usage drops drastically as all the animation stops and the new page has little UI elements on it. I tried different settings, including setting max buffer size to 1, manually clearing frame buffer, disabling storing cache to memory. Nothing helps. Not sure what is holding onto that memory? When i finally dismiss the initial navigation page that contains all the animated images, Xcode displays memory usage drops back down to around 100 MB. Any clue as to why it is like this? Does the SDAnimatedImage itself consume a lot of memory? Or the loaded image & data is held somewhere that i am not aware of? I checked my code, and I don't think i am strongly holding onto any of these (besides setting the image of course)

@dreampiggy
Copy link
Contributor

dreampiggy commented Sep 29, 2023

Yes, then buffer is just a NSMutableDictionary hosted in SDAnimatedPlayer which retained by SDAnimatedImageView

SD is open source project. If you think that clearBufferWhenStopped behaves incorrectly, you can have a debug to check actual reason. PR is welcomed.

Does the SDAnimatedImage itself consume a lot of memory

SDAnimatedImage retain original image data, but it should not be large compared to all frame buffers. But pay attention if you use UIImageView without "decodeFirstFrameOnly", then it will create "_UIAnimatedImage" which retain all frame into memory. Use Xcode's memory graph tool to see all the object living in the memory

@LeoTheCatMeow
Copy link
Author

LeoTheCatMeow commented Sep 29, 2023

@dreampiggy I see. I experimented more and is able to confirm that "clearBufferWhenStopped" works correctly. However there is not much buffer to discard in the first place. When clearing all the buffers, it merely shreds off ~20MB of memory, the majority of the memory is used by SDAnimatedImage itself. I am not sure if this is an issue with the animated image format or the animated image coder. I found an alternative solution for now: I simply de-allocate the SDAnimatedImage when a SDAnimatedImageView loses its window, by setting image = nil. And then query for it again from disk later when I need it. This successfully drops the 200 MB usage back to 100 MB when i am not on that page, and then it grows back to 200 MB when i come to the page again. Another thing I found is that "thumbnailPixelSize" option does not work for the animated images we encounter. It works on static images only. For animated images, setting a small value does not affect the resulting size, until I set something really small, such as (50,50), which then simply makes the animated image not animate, but its resolution is still never affected. I suspect we are not dealing with GIF here, but APNG instead? I have little knowledge of all the image formats and decoding, pretty clueless. But at least the solution i came up kinda fixes the problem. Thanks again for all the suggestions and help!

Edit: Ah, I realized some more. I have set a global memory cache limit of 50MB, so there wouldn't be much cache at all, setting maxBufferSize does not help, since there's already a global limit of 50. Most of the memory usage only comes from the image data, i simply have too many images on screen, and CPU usage is also super high as many frames are decoded again and again. There's not much to do here, we simply need to not push things to the limit.

@hnny09
Copy link

hnny09 commented Nov 17, 2023

Great. Thanks a lot. I have a clear picture now. I will try all these techniques. I think i am supposed to close the ticket now? I'll go do that.

Problem solved? What is the ultimate solution?

@dreampiggy
Copy link
Contributor

dreampiggy commented Nov 17, 2023

I think

The ultimate solution, is to refactor the current SDAnimatedImageView/SDAnimatedImagePlayer frame buffer control

Previous it use the individual control, like calculation of current RAM pressure, inside each SDAnimatedImagePlayer instance level (no-global)

But since, RAM is a resource Globally, this design is totally wrong.

We should move the calculation of current RAM pressure into a global manager, which blocking any decoding thread.

@hnny09
Copy link

hnny09 commented Nov 17, 2023

have you made any modifications to the plan? this bug has been around for a long time

@dreampiggy
Copy link
Contributor

This is not bug...

It's actually OOM

To reduce memory usage, whatever in SDWebImage's loading system, or your App code, need to consider more.

@LeoTheCatMeow
Copy link
Author

have you made any modifications to the plan? this bug has been around for a long time

@hnny09 I also feel that it can't be quite considered a bug, it does not happen normally, but only happens when you have a ton of animated images and the memory they occupy exceed the amount the app could handle. The ultimate solution is to just handle memory better. What I came up in the end is simple:

  1. When an animated image view's window becomes nil (via didMoveToWindow), you just replace the image (yes, the entire image) with nil image. With a combination of clearing the buffer and de-allocating the image, all used memory is released.
  2. Another strategy I also applied is to only store animated image on disk, never in memory. Because each animated image can take up a lot of memory, so ensure we only store and retrieve from disk.
  3. When the animated image is added to window again (appears), just go fetch it again, because it should already be cached on disk, so it is pretty quick to get. You can slap on some transition and honestly it looks natural, doesn't feel out of place.
  4. Remember to never straight up use a ScrollView / UIScrollView to display animated images without any lazy loading mechanism. You can use lazy stacks in SwiftUI or just UITableView / UICollectionView to re-use cells

Of course any improvement or safety that can be implemented from SDWebImage's end would also be appreciated..

@dreampiggy
Copy link
Contributor

dreampiggy commented Nov 20, 2023

you just replace the image (yes, the entire image) with nil image

This can be easily changed because in SDWebImage 5.x

Because SDAnimatedImageView.image is a UIImage is public API, which is used to handle the setter of animated image. You can not have two properities , one called image, another called animatedImage

If we set the SDAnimatedImageView.image = nil, actually, we can not get that SDAnimatedImage back from anywhere. It's disappear totally.

There are already something handled during viewDidMoveToWindow and UIApplicationDidEnterBackground, check the code there. Which clear the frame buffer (not SDAnimatedImage, but CGImage)

@dreampiggy
Copy link
Contributor

dreampiggy commented Nov 20, 2023

Another strategy I also applied is to only store animated image on disk, never in memory.

You can configurate it in SD. Using the options processor to filter only the gif/webp url, and override SDWebImageContextStoreCacheType = .disk

@dreampiggy
Copy link
Contributor

dreampiggy commented Nov 20, 2023

Of course any improvement or safety that can be implemented from SDWebImage's end would also be appreciated..

Many of them Can not implemented by SD, because it's not controlled by us. It's controlled by UIKit and App developer. We can only provide the SDK and utils

The only or better things we can do , it's to provide the default options (behavior) which match most of users. However, An App contains large amount of GIF or AWebP actually is not the most popular user we meet. At least at the day of SDWebImage 5.x development (in 2019, I remember)...

SD is never designed to provide any hook or magic for system SDK or user, which may surprise the exists user.

@dreampiggy
Copy link
Contributor

Anyway, I think the global control for decoding thread and process is a better idea.

We can not simply be stupid and create a dispatch queue then submit all ImageIO decoding work into that queue. Which is wrong. All the decoding process should be controlled and scheduled, or blocking, or cancelling when system resource is limited

@dreampiggy dreampiggy added this to the Future milestone Nov 20, 2023
@dreampiggy dreampiggy added the OOM label Nov 20, 2023
@dreampiggy dreampiggy reopened this Nov 20, 2023
@hnny09
Copy link

hnny09 commented Nov 20, 2023

ok, I check my code.

@LeoTheCatMeow
Copy link
Author

This can be easily changed because in SDWebImage 5.x

Because SDAnimatedImageView.image is a UIImage is public API, which is used to handle the setter of animated image. You can not have two properities , one called image, another called animatedImage

If we set the SDAnimatedImageView.image = nil, actually, we can not get that SDAnimatedImage back from anywhere. It's disappear totally.

Yes, I understand this. I did it this way because the animated images themselves, even when not decoded, was taking about 10MB each. It was just the extreme case I encountered, not a common issue. If we have 20 of these images on screen, it would be 200MB + memory already, not even considering the buffer. So I only tried to de-allocate the image completely due to the extreme scenario I encountered. It's not something others should follow. Because the image disappears totally, I just keep the url around and query again from disk when I need it again.

Anyway, I think the global control for decoding thread and process is a better idea.

We can not simply be stupid and create a dispatch queue then submit all ImageIO decoding work into that queue. Which is wrong. All the decoding process should be controlled and scheduled, or blocking, or cancelling when system resource is limited

Yeah sounds like a good idea! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants