penfeizhou / APNG4Android

Android animation support for APNG & Animated WebP & Gif & Animated AVIF, High performance
Apache License 2.0
583 stars 77 forks source link

Gif memory leak #203

Closed terrysahaidak closed 1 year ago

terrysahaidak commented 1 year ago

New Issue Checklist

Issue Info

Info Value
Device Info Pixel 7 pro
System Version 13.0
APNG4Android Library Version 2.24.0
Repro rate all the time (100%)
Repro with our demo project not sure
Demo project link https://github.com/terrysahaidak/expo-image-gifs-memory-leak

Issue Description and Steps

I was building an app with React Native and was using expo-image package, that uses Glide and this library to animate gifs. After some investigation, I realized that the only thing causing my memory leaks was this library. Commenting out this library implementation in Gradle solves all the memory leaks.

The expo-image package uses pretty much the same code as another library called react-native-fast-image that also uses glide, but the only difference between both of them is the usage of this library - the fast-image one animates gifs using glide, but the performance of it is not great by any means.

Taking a heap dump I saw that byte[] are never released from the memory, and it happens only with this library enabled. They do clear the view the normal way you would do with glide: https://github.com/expo/expo/blob/main/packages/expo-image/android/src/main/java/expo/modules/image/ExpoImageViewWrapper.kt#L423-L429

After a quick investigation of the code, should this method be implemented somehow? https://github.com/penfeizhou/APNG4Android/blob/master/plugin_glide/src/main/java/com/github/penfeizhou/animation/glide/FrameDrawableTranscoder.java#L101

Here is the original issue: https://github.com/expo/expo/issues/24259 Here is the gradle file: https://github.com/expo/expo/blob/main/packages/expo-image/android/build.gradle#L103

penfeizhou commented 1 year ago

The memory will be released while the animation is being stopped. In glide the animation's stop api should be called automatically.So we didn't do things inside recycle. However,due to this issue,we will check this to see if something is wrong.

terrysahaidak commented 1 year ago

I just tried to use APNG and it seems that there is no memory leak anymore. Previously i tried WEBP and had the same problem. APNG is the only one that has implemented recycle method which stops the animation. Not sure why glide doesn't stop the animation in my case. https://github.com/penfeizhou/APNG4Android/blob/master/plugin_glide/src/main/java/com/github/penfeizhou/animation/glide/FrameDrawableTranscoder.java#L51

terrysahaidak commented 1 year ago

https://github.com/penfeizhou/APNG4Android/assets/7809008/938809a5-2ada-4652-8951-90c80eccfce5

jingpeng commented 1 year ago

Ok, we tried to detect this leak with your project and found that as below: 屏幕截图 2023-09-11 112216 Expo only start the animation and do not invoked the animation's stop method, so we implements the recylce method in glide as follows: https://github.com/penfeizhou/APNG4Android/commit/c3ec09fc8606bd1d3dabac1e2a3a7bb1ed0be10f

penfeizhou commented 1 year ago

Update to v2.28.0 will resolve this problem.

jingpeng commented 1 year ago

https://github.com/expo/expo/pull/24358 await the review and merge

terrysahaidak commented 1 year ago

That was quick! Thanks a lot! Appreciate it!

Somehow I missed the start call in the sources with the Animatable "if" block, was thinking about calling stop when they clear the view, but I had to do the if block there so the type would be Animatable to stop the animation too.

But I guess the recycle method is a more bullet proof way here.

terrysahaidak commented 1 year ago

By the way, can confirm 2.28.0 fixes all the issues I had.