Repository navigation
[CP-stable][Android] Close FlutterRenderer texture image SyncFence objects immediately after waiting on them - #189272
Conversation
…iately after waiting on them (flutter#188313) Each SyncFence may use a file descriptor. But the SyncFences were not being closed promptly and their file descriptors were not being deleted until the SyncFences were garbage collected. If the process is rendering video and creating a SyncFence for each frame, then it may run out of file descriptor capacity before Java GC happens. Fixes flutter#188161
|
@jason-simmons please fill out the PR description above, afterwards the release team will review this request. |
github-actions
Bot
added
platform-android
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors FlutterRenderer to use try-with-resources for managing SyncFence resources, ensuring they are closed, and adds a unit test for this behavior. The review feedback recommends adding defensive null checks to prevent potential NullPointerExceptions when retrieving the fence, and restricting the new test to API level 33 and above to avoid compatibility issues on older SDKs.
| try (SyncFence fence = image.getFence()) { | ||
| fence.awaitForever(); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
To prevent potential NullPointerExceptions on certain OEM implementations or in test environments where image.getFence() might return null, add a defensive null check before calling fence.awaitForever().
try (SyncFence fence = image.getFence()) {
if (fence != null) {
fence.awaitForever();
}
} catch (IOException e) {| try (SyncFence fence = image.getFence()) { | ||
| fence.awaitForever(); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
To prevent potential NullPointerExceptions on certain OEM implementations or in test environments where image.getFence() might return null, add a defensive null check before calling fence.awaitForever().
try (SyncFence fence = image.getFence()) {
if (fence != null) {
fence.awaitForever();
}
} catch (IOException e) {| @Test | ||
| public void waitOnFence_closesFence() throws Exception { |
There was a problem hiding this comment.
Since SyncFence and ImageReaderSurfaceProducer are only available on API level 33 and above, running this test on older SDKs will result in ClassCastException or NoClassDefFoundError. Annotate the test with @Config(sdk = 33) to ensure it only runs on supported SDK versions.
@Test
@org.robolectric.annotation.Config(sdk = 33)
public void waitOnFence_closesFence() throws Exception {
justinmc
added
the
autosubmit
69c8c61
into
flutter:flutter-3.44-candidate.0
…jects immediately after waiting on them (flutter#189272) ### Issue Link: What is the link to the issue this cherry-pick is addressing? flutter#188161 ### Impact Description: This issue can causes crashes in apps that use external textures (often used for rendering video). On Arm Mali GPUs each frame may leak a file descriptor, and the app will crash when the process reaches the file descriptor limit. ### Changelog Description: flutter#188161 When apps that use external textures run on some GPUs (such as Arm Mali), it can cause a resource leak that eventually crashes the app. ### Workaround: Is there a workaround for this issue? No ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? * Run the example app from the `flutter-webrtc` package on a Mali-based Android device * Go to the "GetUserMedia" screen and turn on the camera. * Watch the count of file descriptors in `/proc/[pid]/fd` for the app process and check that it does not increase as the captured video is displayed
3 participants
Issue Link:
What is the link to the issue this cherry-pick is addressing?
#188161
Impact Description:
This issue can causes crashes in apps that use external textures (often used for rendering video). On Arm Mali GPUs each frame may leak a file descriptor, and the app will crash when the process reaches the file descriptor limit.
Changelog Description:
#188161 When apps that use external textures run on some GPUs (such as Arm Mali), it can cause a resource leak that eventually crashes the app.
Workaround:
Is there a workaround for this issue?
No
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
What are the steps to validate that this fix works?
flutter-webrtcpackage on a Mali-based Android device/proc/[pid]/fdfor the app process and check that it does not increase as the captured video is displayed