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 clip sharing memory leak #3223

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Fix clip sharing memory leak #3223

merged 4 commits into from
Nov 14, 2024

Conversation

MiSikora
Copy link
Contributor

@MiSikora MiSikora commented Nov 13, 2024

Description

FFmpegKitConfig retains session history. Sessions hold references to callbacks, which, in turn, hold references to view models and fragments. There is no mechanism in the FFMpegKit library to clear a single session or to retain no history. For this reason, I clear the entire history after a session completes its task.

Issue reported here: arthenica/ffmpeg-kit#1066

Testing Instructions

  1. Share an audio or a video clip.
  2. Close the sharing flow.
  3. Using Leak Canary or Android Studio's memory profiler verify that ShareClipFragment and ShareClipViewModel do not leak.

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

@MiSikora MiSikora added [Type] Bug Not functioning as intended. [Type] Tech Debt Involving upgrades or refactoring to maintain or enhance the codebase. [Project] Reimagine Sharing labels Nov 13, 2024
@MiSikora MiSikora added this to the 7.78 milestone Nov 13, 2024
@MiSikora MiSikora requested a review from a team as a code owner November 13, 2024 12:07
@MiSikora MiSikora requested review from geekygecko and removed request for a team November 13, 2024 12:07
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 13, 2024

📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
App Name 📱 Mobile
Build TypedebugProd
Commitdf1b672
Direct Downloadpocketcasts-app-prototype-build-pr3223-df1b672.apk
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
App Name 🚗 Automotive
Build TypedebugProd
Commitdf1b672
Direct Downloadpocketcasts-automotive-prototype-build-pr3223-df1b672.apk
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
App Name ⌚ Wear
Build TypedebugProd
Commitdf1b672
Direct Downloadpocketcasts-wear-prototype-build-pr3223-df1b672.apk

@CookieyedCodes
Copy link

I would recommend that dismissing also clears the cache to prevent another potential memory link 😉

@MiSikora MiSikora removed the [Type] Tech Debt Involving upgrades or refactoring to maintain or enhance the codebase. label Nov 13, 2024
Copy link
Member

@geekygecko geekygecko left a comment

Choose a reason for hiding this comment

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

Leak Canary seemed happy.

build.gradle.kts Outdated Show resolved Hide resolved
Co-authored-by: Philip Simpson <philz.the.gecko@gmail.com>
@MiSikora MiSikora enabled auto-merge (squash) November 14, 2024 06:41
@MiSikora MiSikora merged commit cbc8f9d into main Nov 14, 2024
16 checks passed
@MiSikora MiSikora deleted the task/sharing-leak branch November 14, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Project] Reimagine Sharing [Type] Bug Not functioning as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants