5.18.4+ SDAnimatedImage static image support performance issue

[LeoTheCatMeow]

New Issue Checklist

• I have read and understood the CONTRIBUTING guide
• I have read the Documentation
• I have searched for a similar issue in the project and found none

Issue Info

Info	Value
Platform Name	ios
Platform Version	15.0 +
SDWebImage Version	5.18.4 +
Integration Method	SPM
Xcode Version	Xcode 15
Repro rate	all the time (100%)
Repro with our demo prj	-
Demo project link	sorry unable to provide

Issue Description and Steps

Hi, I have particularly strange issue encountered which I narrowed down to this particular change of SDAnimatedImage:

5.18.4 - 5.18 Fix, on Oct 27th, 2023
SDAnimatedImage now supports static image like JPEG data #3626 #3625
Previous version the initializer will return nil and has to use UIImage/NSImage instead

When downloading images from the web, we sometimes don't know for sure beforehand if this image is animated or not. Although we can guess from the url extension, we'd prefer the process to be more automatic. So in some occasions, we would pass
context[.animatedImageClass] = SDAnimatedImage.self
to ask SDWebImage to try use animated image if possible. In previous versions, if this image is not an animated image, it would fallback to a regular UIImage, which we have no issue with.

After upgrading to the 5.18.4+ however, the newly added mechanism would create a SDAnimatedImage instead of returning nil, this seems to bring noticeable performance overhead when scrolling in a UICollectionView. We use a subclass of SDAnimatedImageView as a general purpose image view that can show either regular or animated images. Scrolling in this case becomes quite choppy, I tried to look into the source code but was unable to understand the full picture. And sorry I do not have any reliable way to benchmark this except from purely observations. I tried 5.18.3 or try not pass context[.animatedImageClass] = SDAnimatedImage.self, the scrolling becomes very smooth again.

I am wondering if it is possible to add an additional SDImageCoderOptions to configure this behavior? Namely, could we add an option so that SDAnimatedImage would fail to create like in the past and we would fallback to a simple UIImage?

[dreampiggy]

The issue may because the default behavior of "Force Decoding" is OFF, for SDAnimatedImage

The comment is showing in the "SDImageForceDecodePolicy" and "SDImageDecodeUseLazyDecoding"

If you don't know what's this, TLDR, the Apple's decoder return a lazy image which is a wrapper, and will decode during UIImageView rendering on screen. This block main queue and drop FPS.
We provide the policy to decode this in advance, called force decode but since this consume RAM, the default behavior for AnimatedImage does not apply force decode

Maybe we need to fix this by check for "frame == 1" instead of "isKindOfClass"

[LeoTheCatMeow]

@dreampiggy
Ok, I see I can enable force decode, or make it not lazy decode, in order to not have FPS drop. But the issue is, SDAnimatedImageView and our custom subclass relies heavily on the logic of if the image conforms to SDAnimatedImage protocol. I see in SDAnimatedImageView, there are a lot of extra logic, such as installing the player and handling animation related calls, checking play etc, if the image should be animated. For our custom subclass, we also have some additional logic to perform if the image is animated image. So even if we can optimize the image rendering itself, all these extra add-ons still bring unnecessary performance overhead. If the image cannot be animated, I feel like it should not pretend to be an animated image, or at least we can have an option to make it not to.

Of course checking frame == 1 is an alternative, but this might have a lot more impact to existing code? Since any place that used to check if the image is animated, now needs to double check the frame count as well. But you know better than me XD, so this is just a suggestion.

[dreampiggy]

The SDAnimatedImage naming is designed by me for animated image from the beginning, in 5 years ago.

But things changed during these years:

1. Like that SwiftUI user who want the easy way to use both animated and static image without thinking what's it.

2. And, image format becomes complicated. Many new format (WebP/HEIF/AVIF) can both be animated or static. JPEG image can not be animated, this will no longer be true. Because we have JPEG-XL: https://jpeg.org/jpegxl/ which supports animated image.

3. Some people says that the cache for the same url, but returns different image class (UIImage or SDAnimatedImage) which may cause mass. I have no good solution currently (That SDWebImageMatchAnimatedImageClass is just a hack). See: Proposal: Introduce global and per-image-request policy control about whether to use UIKit animated image or SDAnimatedImage, based on frame count and bytes size #3669

I think, the better way is to rename this class into SDImage not SDAnimatedImage, so it can init with any image format.

For current quick hack, the old check for conformsToProtocol: @protocol(SDAnimatedImage) can be changes in SDWebImage core repo with sd_isAnimated. This API already considerate the frame count > 1

[dreampiggy]

Can #3708 fix your issue ?

If not, actually, it's OK to totally revert the #3625

These are just two choice:

1. Always pin the user to use UIImage as backup when SDAnimatedImage init failed (Previous 5.18 version)
2. Handle the complicated case ourselves, just all using SDAnimatedImage and ignore the parent class

[LeoTheCatMeow]

The SDAnimatedImage naming is designed by me for animated image from the beginning, in 5 years ago.

But things changed during these years:

1. Like that SwiftUI user who want the easy way to use both animated and static image without thinking what's it.
2. And, image format becomes complicated. Many new format (WebP/HEIF/AVIF) can both be animated or static. JPEG image can not be animated, this will no longer be true. Because we have JPEG-XL: https://jpeg.org/jpegxl/ which supports animated image.
3. Some people says that the cache for the same url, but returns different image class (UIImage or SDAnimatedImage) which may cause mass. I have no good solution currently (That SDWebImageMatchAnimatedImageClass is just a hack). See: Proposal: Introduce global and per-image-request policy control about whether to use UIKit animated image or SDAnimatedImage, based on frame count and bytes size #3669

I think, the better way is to rename this class into SDImage not SDAnimatedImage, so it can init with any image format.

For current quick hack, the old check for conformsToProtocol: @protocol(SDAnimatedImage) can be changes in SDWebImage core repo with sd_isAnimated. This API already considerate the frame count > 1

I see, thanks for educating me on the details of animated images. I think then it is very reasonable to use a single image class and rename to "SDImage". It does feel more intuitive from a user's perspective to not have to worry too much about animated or not and just form a single image for all use cases. As long as sd_isAnimated is a reliable check, and SDAnimatedImageView handles things properly, then i think it should be good.

[LeoTheCatMeow]

Can #3708 fix your issue ?

If not, actually, it's OK to totally revert the #3625

These are just two choice:

1. Always pin the user to use UIImage as backup when SDAnimatedImage init failed (Previous 5.18 version)
2. Handle the complicated case ourselves, just all using SDAnimatedImage and ignore the parent class

I checked, #3708, looks good to me. Thanks a lot! Here are a couple questions & suggestions I have:

1. In my subclass, i should just use image.sd_isAnimated to check and then setup some additional logics right? I have some custom player loop observation code in place. I see in your source code you checked with conformance + frame count, both way should be fine right?
2. Now that animated and non-animated images can be both handled by SDAnimatedImage(SDImage), should the context option animatedImageClass to renamed to maybe just "imageClass"? This is minor tho.
3. I am bit confused about the SDImageCoderDecodeUseLazyDecoding and SDImageForceDecodePolicy. So if we force decode images on background, it will provide better performance when scrolling because it is ready to use directly. But documentation of SDImageCoderDecodeUseLazyDecoding says static images use lazy decode, so they are not forced decoded before hand? There's some confusion here and I don't quite understand the full picture, sorry XD

[dreampiggy]

says static images use lazy decode, so they are not forced decoded before hand

Static Image (UIImage, trigged by decodedImageWithData:)

1. Apple's ImageIO coder return a lazy CGImage
2. We check some format (like JPEG/HEIF) to do force decoding, based on the option SDWebImageAvoidDecodeImage(deprecated and replaced with SDWebImageContextForceDecodePolicy). The default behavior is to force decode JPEG/HEIF, and ignore on other format (This is Apple's recommendation, using https://developer.apple.com/documentation/uikit/uiimage/3750834-preparingfordisplay)
3. This result the final CGImage to become non-lazy (JPEG/HEIF only)

Animated Image (SDAnimatedImage, triggered by animatedImageFrameAtIndex:)

1. Apple's ImageIO coder return a lazy CGImage
2. The SDWebImageAvoidDecodeImage is useless and does nothing. You have to use SDImageCoderDecodeUseLazyDecoding, the default behavior is to still use the lazy CGImage (because huge animated image force decode may have OOM, you allocate memory in advance)

[dreampiggy]

image.sd_isAnimated to check and then setup some additional logics right

Is enough at least for most cases. Because sd_isAnimated actually matches the UIAnimatedImage and SDAnimatedImage whose frame count > 1.

These two case is hard compared to static image (like transformer) or re-encoding.

If you really want to distinguish the UIAnimatedImage and SDAnimatedImage whose frame count > 1, check the protocol (not class, because, actually when I develop this feature, I want to support YYImage-like custom object to be used in SDAnimatedImageView)

[LeoTheCatMeow]

@dreampiggy Ok thanks, I see. I think once the new version comes out, and the performance issue is taken care of, then we can close this issue.

[dreampiggy]

I think once the new version comes out, and the performance issue is taken care of

Can you just test using that branch dependency to see whether the performance regression is solved ?

I'll wait for your tesing result and then merge that

[LeoTheCatMeow]

I think once the new version comes out, and the performance issue is taken care of

Can you just test using that branch dependency to see whether the performance regression is solved ?

I'll wait for your tesing result and then merge that

I left a comment on the PR. I verified with that branch and the scrolling performance is smooth. I think you can go ahead 👍 Things look good and thanks for the fast response and help!