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

fix(MM-62375): OOM on gif #8573

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions android/settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ include ':reactnativenotifications'
project(':reactnativenotifications').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-notifications/lib/android/app')
include ':watermelondb-jsi'
project(':watermelondb-jsi').projectDir = new File(rootProject.projectDir, '../node_modules/@nozbe/watermelondb/native/android-jsi')
include ':frameanimation'
project(':frameanimation').projectDir = new File('../node_modules/APNG4Android/frameanimation')
include ':gif'
project(':gif').projectDir = new File('../node_modules/APNG4Android/gif')
Comment on lines +11 to +14
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to get this as project in expo-image/android/build.gradle, need to declare frameanimation and gif here.

includeBuild('../node_modules/@react-native/gradle-plugin')
apply from: new File(["node", "--print", "require.resolve('expo/package.json')"].execute(null, rootDir).text.trim(), "../scripts/autolinking.gradle")
useExpoModules()
4 changes: 2 additions & 2 deletions ios/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ PODS:
- ExpoModulesCore
- ExpoFileSystem (18.0.6):
- ExpoModulesCore
- ExpoImage (2.0.3):
- ExpoImage (2.0.4):
- ExpoModulesCore
- SDWebImage (~> 5.19.1)
- SDWebImageSVGCoder (~> 1.7.0)
Expand Down Expand Up @@ -2466,7 +2466,7 @@ SPEC CHECKSUMS:
ExpoCrypto: 483fc758365923f89ddaab4327c21cae9534841d
ExpoDevice: 449822f2c45660c1ce83283dd51c67fe5404996c
ExpoFileSystem: a38e1bb58d77f41717293a7b73ebd4014d8cb8dd
ExpoImage: 2c1f4adee95d033894c53346c721361736583728
ExpoImage: 18ec1a3e5cd96ee6162d988be3085e18113e143f
ExpoLinearGradient: 18148bd38f98fa0229c1f9df23393b32ab6d2d32
ExpoModulesCore: c9cb309323aee3c048099bb40ad0226e3fcf0c56
ExpoStoreReview: f38ed71ee04dfc02dde63a05f925f001cebba5d8
Expand Down
9 changes: 4 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
"expo-application": "6.0.1",
"expo-crypto": "14.0.1",
"expo-device": "7.0.1",
"expo-image": "2.0.3",
"expo-image": "2.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to bump this? Just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a mistake when reading the changelog. And looking at the changes, I thought the addition of try..catch was going to help, but it didn't. Looking deeper, I noticed that it was already implemented in 2.0.3, not a new change in 2.0.4.

Changelog for 2.0.4:

## 2.0.4 — 2025-01-10

_This version does not introduce any user-facing changes._

So it's pretty moot.

"expo-linear-gradient": "14.0.1",
"expo-store-review": "8.0.0",
"expo-video-thumbnails": "9.0.2",
Expand Down
18 changes: 18 additions & 0 deletions patches/expo-image+2.0.3.patch → patches/expo-image+2.0.4.patch
Original file line number Diff line number Diff line change
@@ -1,3 +1,21 @@
diff --git a/node_modules/expo-image/android/build.gradle b/node_modules/expo-image/android/build.gradle
index a85fb5e..ac568c1 100644
--- a/node_modules/expo-image/android/build.gradle
+++ b/node_modules/expo-image/android/build.gradle
@@ -44,7 +44,12 @@ dependencies {
kapt "com.github.bumptech.glide:compiler:${GLIDE_VERSION}"
api 'com.caverock:androidsvg-aar:1.4'

- implementation "com.github.penfeizhou.android.animation:glide-plugin:3.0.2"
+ implementation project(':frameanimation')
+ implementation project(':gif')
+ implementation("com.github.penfeizhou.android.animation:glide-plugin:3.0.2") {
+ exclude group: 'com.github.penfeizhou.android.animation', module: 'frameanimation'
+ exclude group: 'com.github.penfeizhou.android.animation', module: 'gif'
Comment on lines +13 to +14
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without exclusions, it will cause duplicate symbols error. Since glide-plugin will have frameanimation (required module) in other modules (apng, webp, etc).

+ }
Comment on lines +10 to +15
Copy link
Contributor Author

@rahimrahman rahimrahman Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixes are in frameanimation & gif.

Now in this PR (that will not get merged).

implementation "com.github.bumptech.glide:avif-integration:${GLIDE_VERSION}"

api 'com.github.bumptech.glide:okhttp3-integration:4.11.0'
diff --git a/node_modules/expo-image/android/src/main/java/expo/modules/image/okhttp/ExpoImageOkHttpClientGlideModule.kt b/node_modules/expo-image/android/src/main/java/expo/modules/image/okhttp/ExpoImageOkHttpClientGlideModule.kt
index 071907c..edf10c1 100644
--- a/node_modules/expo-image/android/src/main/java/expo/modules/image/okhttp/ExpoImageOkHttpClientGlideModule.kt
Expand Down
5 changes: 5 additions & 0 deletions scripts/postinstall.sh
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,8 @@ else
mkdir -p "android/app/src/main/res/raw/"
cp $SOUNDS/* "android/app/src/main/res/raw/"
fi

APNG4Android="node_modules/APNG4Android"
if [ ! -z "$APNG4Android" ]; then
git clone -b "fix/MM-62375-oom-on-gif" --single-branch https://github.com/rahimrahman/APNG4Android.git "$APNG4Android";
fi
Copy link
Contributor Author

@rahimrahman rahimrahman Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't do npm i since APNG4Android doesn't have package.json.

  • TODO: will replace with mattermost/APNG4Android (as soon as permission to write is granted)
  • TODO: figure out if I can use SHA instead of branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you cannot clone a specific commit, but you can clone the branch and then do a checkout

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use --depth=1 so that it only clones the latest one

Loading