Reduce GIF buffer limits

[kean]

By default, Gifu preloads and stores 50 frames in memory, which is more than enough to support smooth playback.

The limits set by the application, especially for large images, are excessive, to say the least. If you take a 1000x1000 pixels GIF with a typical 32 bits per pixel, it'll be 4 MB per single frame/bitmap. Multiple that by 300, and that's 1.2 GB for a single GIF without taking into account how this memory is allocated – it'll most likely take even more memory.

You can't fit many large GIFs fully in memory, so the buffer is typically there to preload just some of the frames to avoid choppy playback. I'm not sure it's even necessary on modern CPUs. GIF playback has been notoriously hard to do efficiently for as long as it existed.

To test:

I'm not sure if there is a good way to test it because even without the buffer, the system will still keep the bitmaps in memory but will clear them on memory warnings. But Gifu won't clear its frame store on memory warnings. If you set it to 300, it'll force these bitmaps to stay in memory.

I'll try to set up a good post with GIFs of different sizes to facilitate further testing and improvements in this area. But this change is a quick hotfix before Monday just to address the most egregious issues. If we switch to FLAnimatedImage, it won't really be necessary because they've already done a lot of performance testing and have an excellent sample for simulating memory pressure scenarios.

Note
Update: I uploaded a few open images from Wikimedia to my test site for testing purposes. I'll upload more different variants tomorrow; done for today.

Regression Notes

1. Potential unintended areas of impact: GIF playback
2. What I did to test those areas of impact (or what existing automated tests I relied on): manual testing
3. What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21156-7a1d70b
	Version	22.8
	Bundle ID	com.jetpack.alpha
	Commit	7a1d70b
	App Center Build	jetpack-installable-builds #5479

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[kean]

These kinds of changes are a bit tricky to measure. I tested it on the test site that I also linked in the description. Before, it used 1.3 GB of memory after viewing all 8 posts with GIFs, and now it's using just 750 MB. The CPU usage is the same because most of the images I uploaded have more than 150 frames, and they don't fully fit in memory anyway, and Gifu keeps recomputing them.

Update: I'm able to reproduce these results consistently. It seems to be a big improvement in terms of memory usage.

The CPU usage will be a bit higher, but it's irrelevant because if you are not rewatching the same GIF multiple times, which you most likely aren't, there is no point keeping it entirely in memory.

Before	After

The power of the library defaults 😁

[kean]

One of the unintended consequences of this change is that it increases the likelihood of this crash happening. So it has to be merged together with #21161.

[staskus]

One of the unintended consequences of this change is that it increases the likelihood of #21160 happening. So it has to be merged together with #21161.

Yes, I encountered the crash myself, good that it can be addressed with the pod update.

Update: I'm able to reproduce these results consistently. It seems to be a big improvement in terms of memory usage.

At least on the test sites I used before, the memory decrease is pretty significant. In cases where I reached 2GB+ now I stay below 1GB.

/// Sets the number of frames that should be buffered. Default is 50. A high number will result in more memory usage and less CPU load, and vice versa.
///
/// - parameter frames: The number of frames to buffer.
public func setFrameBufferCount(_ frames: Int) {
animator?.frameBufferCount = frames
}

I tried to observe changed in CPU load as well, it does seem higher for some of the posts but the wins I observed in terms of memory usage looked greater. On some of the posts with GIFs the difference was pretty dramatic:

Before	After

[staskus]

The changes looked good to me and make sense, good catch! I agree with your assessment that:

The limits set by the application, especially for large images, are excessive, to say the least

I was interested in why those limits had these values set. It looks like it was done in Fixes some issues with GIF images in the reader., for the issue reported here: p4a5px-2xT-p2 when observing this post: pbMoDN-gz-p2. This was the conclusion of investigation:

From what I could gather from running the profiler, the issue is caused by the GIF animation in the last image using a lot of CPU time.

I noticed that one of the GIFs in that post is a 8.4 MB file with 275 frames (3rd image) and the other GIF is a 3 MB file with 141 frames. There are quite large, although the 8.4 MB one is not even animated due to how large it is.

A partial solution for this issue is to increase the frame buffer size in CachedAnimatedImageView.swift to a number like 300. This will shift the problem from CPU to Memory, although I believe it’s acceptable. The downside of this approach is that we can always come across larger GIFs with more frames that will still require constant loading.

So it looks like this was a solution to the opposite problem, reducing the CPU load in exchange of more memory load. This is how it looks on iPhone 14 Pro when scrolling this post: pbMoDN-gz-p2 on the Reader, we can clearly see the shift from memory to CPU, I'll check on a lower-end device as well:

Before Changes	After Changes

What is your take on this? My inclination is to accept the default frameBufferCount which is 50. However, it seems that large GIFs are bound to create challenges, in one way or another.

[kean]

What is your take on this? My inclination is to accept the default frameBufferCount which is 50. However, it seems that large GIFs are bound to create challenges, in one way or another.

The main reason Gifu and other frameworks exist is to reduce the amount of memory used when rendering GIFs. For example, if you take FLAnimatedImage, they use a buffer with 5 frames by default, which I think is an even better default. FLAnimatedImage also reacts to memory warnings and automatically reduces the buffer size when under memory pressure.

There is a way to configure FLAnimatedImage to store all frames in memory, which is great when you want to render small GIFs you have control over as part of your app's user interface. The assumption is that these perform multiple loops, so the memory isn't wasted.

If you are scrolling a feed of images, just like in the Reader, you don't need a buffer. It's also impossible to fit large GIFs entirely in memory.

The CPU usage will be high during the initial loop regardless of whether you store the pre-rendered frames or not. It settles only after you sit watching the same GIF over and over again. A brief spike in CPU usage is OK, but a quick burst of allocations that need hundreds of megabytes of memory is what will likely trigger a memory warning and may even result in an app termination.

[kean]

I completely forgot I'm a Gifu collaborator 😅
If we need to do some performance tuning for the app, I can help with that.

[staskus]

The CPU usage is will be high during the initial loop regardless of whether you store the pre-rendered frames or not. It settles only after you sit watching the same GIF over and over again. A brief spike in CPU usage is OK, but a quick burst of allocations that need hundreds of megabytes of memory is what may trigger an app termination.

Okay, thanks for the answer 👍 I think it makes sense. Lets merge it with the #21161