Skip to content

[camera_android_camerax] Fixes premature garbage collection of native objects when app is under memory pressure #9287

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

Merged
merged 6 commits into from
Jun 5, 2025

Conversation

bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented May 19, 2025

Note: Waiting on #9264 to land and to regenerate baseline file with.

This bumps pigeon to 25.3.2 and regenerates the code to fix a bug where native objects were getting garbage collected prematurely. See flutter/flutter#168531

Fixes flutter/flutter#152763

Pre-Review Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

@bparrishMines bparrishMines changed the title regen pigeon [camera_android_camerax] Fixes premature garbage collection of native objects when app is under memory pressure May 19, 2025
@@ -60,7 +60,7 @@ android {
lintOptions {
checkAllWarnings true
warningsAsErrors true
disable 'AndroidGradlePluginVersion', 'GradleDependency', 'InvalidPackage', 'NewerVersionAvailable'
disable 'AndroidGradlePluginVersion', 'GradleDependency', 'InvalidPackage', 'NewerVersionAvailable', 'UnsafeOptInUsageError'
Copy link
Contributor Author

@bparrishMines bparrishMines May 19, 2025

Choose a reason for hiding this comment

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

In #8618, I manually added this to the generated pigeon file CameraXLibrary.g.kt. To prevent the need to manually change generated code, I moved the disable here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Working on another camera PR (#9264) and I created a baseline lint file for that same UnsafeOptInUsageError lint. I think that might be better just in case the lint is valid for non-generated files, but let me know what you think!

If you agree and these other lints are from CameraXLibrary.g.kt, maybe we should remove them and just regenerate the baseline file.

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'll wait for #9264 to land and then regen the baseline file for this PR

@bparrishMines bparrishMines marked this pull request as ready for review May 19, 2025 21:11
@bparrishMines bparrishMines requested a review from camsim99 as a code owner May 19, 2025 21:11
@@ -60,7 +60,7 @@ android {
lintOptions {
checkAllWarnings true
warningsAsErrors true
disable 'AndroidGradlePluginVersion', 'GradleDependency', 'InvalidPackage', 'NewerVersionAvailable'
disable 'AndroidGradlePluginVersion', 'GradleDependency', 'InvalidPackage', 'NewerVersionAvailable', 'UnsafeOptInUsageError'
Copy link
Contributor

Choose a reason for hiding this comment

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

Working on another camera PR (#9264) and I created a baseline lint file for that same UnsafeOptInUsageError lint. I think that might be better just in case the lint is valid for non-generated files, but let me know what you think!

If you agree and these other lints are from CameraXLibrary.g.kt, maybe we should remove them and just regenerate the baseline file.

@@ -31,7 +31,7 @@ dev_dependencies:
sdk: flutter
leak_tracker_flutter_testing: any
mockito: ^5.4.4
pigeon: ^25.3.1
pigeon: ^25.3.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify: this is a guess at the fix for flutter/flutter#152763 or have you determined this is the cause?

Copy link
Contributor Author

@bparrishMines bparrishMines May 21, 2025

Choose a reason for hiding this comment

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

It is a guess, but a very educated guess.

I haven't been able to reproduce the exact error from the camerax_issue, but I have been able to reproduce the issues from flutter/flutter#168531 and flutter/flutter#168306 on iOS. Both of these are similar to the camerax_issue because they are NullPointerExceptions caused by an instance that originates from the native side (e.g. ImageProxy). Both of these were solved by bumping pigeon to 25.3.2.

I think it is harder to reproduce the error on Android because garbage collection for Java is nondeterministic and it requires the ImageProxy to be garbage collected within a small time frame. But I did use a few debug logs to verify that the scenario described here does occur within the plugin. And that this change most likely prevents it. Although the scenario is pretty rare, so its not impossible that I was just lucky.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha makes sense! SGTM :)

@bparrishMines bparrishMines marked this pull request as draft May 21, 2025 20:08
@@ -60,7 +60,7 @@ void onFailure(@NonNull String methodName, @NonNull Throwable throwable) {
}

// PreviewProxyApi maintains a state to track SurfaceProducers provided by the Flutter engine.
@NonNull private final PreviewProxyApi previewProxyApi = new PreviewProxyApi(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm what version of Java are you using with locally?

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 tried building again on flutter stable and I don't get the lint error. Could it be caused by the flutter.compileSdkVersion in the example app build.gradle? I was building it on flutter main before. Regardless, the lint seems to be valid.

@bparrishMines bparrishMines marked this pull request as ready for review June 4, 2025 15:41
@bparrishMines bparrishMines requested a review from camsim99 June 4, 2025 15:41
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

Change LGTM :) curious about the lint error you got locally but non-blocking

@@ -60,7 +60,7 @@ void onFailure(@NonNull String methodName, @NonNull Throwable throwable) {
}

// PreviewProxyApi maintains a state to track SurfaceProducers provided by the Flutter engine.
@NonNull private final PreviewProxyApi previewProxyApi = new PreviewProxyApi(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm what version of Java are you using with locally?

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2025
@auto-submit auto-submit bot merged commit 610dcb7 into flutter:main Jun 5, 2025
80 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2025
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Jun 6, 2025
flutter/packages@ecba2db...974f152

2025-06-06 [email protected] [camera_avfoundation]
Implementation swift migration - part 4 (flutter/packages#9219)
2025-06-05 [email protected] [go_router_builder] Use a mixin
instead of an extension (flutter/packages#9275)
2025-06-05 [email protected] Roll Flutter master to 8b18dde
(flutter/packages#9387)
2025-06-05 [email protected]
[camera_android_camerax] Fixes premature garbage collection of native
objects when app is under memory pressure (flutter/packages#9287)
2025-06-05 [email protected]
[google_maps_flutter_platform_interface] Add a new zIndexInt param to
marker and deprecate zIndex (flutter/packages#9372)
2025-06-05 [email protected]
[camera_android_camerax] Java style improvements (flutter/packages#9234)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[camera_android_camerax] Camera Image Stream is randomly stopping
2 participants