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.

Sign up for GitHub

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
+45 −8
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12b3757
fix(MM-62375): OOM on gif
rahimrahman Feb 8, 2025
ea591bf
add APNG4Android into github action script to download
rahimrahman Feb 8, 2025
24cf5bd
change to mattermost/APNG4Android
rahimrahman Feb 10, 2025
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
11 changes: 11 additions & 0 deletions .github/actions/prepare-node-deps/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,14 @@ runs:
cp "$COMPASS_ICONS" "assets/fonts/"
cp "$COMPASS_ICONS" "android/app/src/main/assets/fonts"
echo "::endgroup::"
- name: ci/clone-APNG4Android
shell: bash
env:
APNG4Android: "node_modules/APNG4Android"
run: |
echo "::group::clone-APNG4Android"
if [ ! -d "$APNG4Android" ]; then
git clone -b "fix/MM-62375-oom-on-gif" --single-branch https://github.com/rahimrahman/APNG4Android.git "$APNG4Android";
Copy link
Contributor Author

@rahimrahman rahimrahman Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any benefit of putting it into node_modules instead of a custom folder on the root path of the app?

  • TODO: Will replace this with the correct repo (mattermost/APNG4Android)

Copy link
Contributor

Choose a reason for hiding this comment

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

Irrelevant in my opinion, but of it works here no need to change

Copy link
Contributor Author

@rahimrahman rahimrahman Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I like node_modules because it's easier to do the npm run clean and it'll auto-clean this library as well. But I could also have added an extra line to clean the folder wherever it might be in scripts/clean.sh as well.

I'll stick with node_modules.

fi
echo "::endgroup::"
4 changes: 4 additions & 0 deletions android/settings.gradle
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop

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
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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 •
edited
Loading

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