Fix Content Sizing Race Condition#182326
Conversation
|
Local testing shows this also fixes the flake (if the listener is always added regardless of if content sizing is enabled - though this PR does NOT remove that condition check). |
|
Thanks for this fix! I can confirm it resolves two issues I was hitting in my project (a video_player fork with PiP and background playback support).
When an Activity enters PiP mode, MediaQuery.sizeOf(context) continued to report the full-screen dimensions (e.g. Size(392.7, 825.5)) instead of the PiP window dimensions (e.g. Size(216.4, 121.8)). This wasn't a Impact: Any widget using MediaQuery or parent constraints to adapt its layout in PiP mode would render at the wrong size. For example, a video centered in a Scaffold would position itself in the middle of an 825dp-tall Workaround I had to implement: Passing the PiP window dimensions directly from Activity.onPictureInPictureModeChanged(boolean, Configuration) via newConfig.screenWidthDp/screenHeightDp through a platform channel event,
When rotating the device (e.g. going landscape for a fullscreen video experience), the viewport metrics were also stale — the layout would briefly render with the old orientation's dimensions before snapping to the Testing
To test, I did: cd $(dirname $(which flutter))/..
git fetch origin pull/182326/head:pr-182326
git checkout pr-182326
flutter precache |
|
@ rwrz - Interesting. Thanks for confirming the fix. Just to be clear, you are using the io.flutter.embedding.android.EnableContentSizing flag to enable the functionality and seeing this behavior? |
Sorry. Not really. Probably I misinterpreted the fix and got the results I needed by luck or another commit. =/ |
@rwrz - even more interesting. How did you decide to use my branch? Were you seeing this line in the logs: "Resize was in response to the engine resizing the view. Not sending viewport metrics."? |
|
@rwrz I guess it's possible another PR fixed the issue somewhere in between stable and HEAD for my PR. I would be extra curious if you cherrypicked just my fix into the stable branch and saw the fix. |
There was a problem hiding this comment.
Code Review
This pull request addresses a race condition in content sizing by making the MaybeResizeSurfaceView call blocking. This is achieved using fml::AutoResetWaitableEvent in C++ to synchronize with the platform thread. On the Java side, a corresponding change posts the layout update to the message loop to prevent deadlocks. The change also removes the shouldSendViewportMetrics AtomicBoolean which is no longer necessary, simplifying the logic. The changes look correct and effectively address the race condition. I've added a couple of suggestions to improve the robustness of the lambda captures in the C++ code.
…er/external_view_embedder_2.cc Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…er/external_view_embedder.cc Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
I am working on integrations tests specifically for content sizing. Rotations were definitely affected and need to be tested thoroughly. #179673 proved that - and that fact that I was able to reproduce the flake without platform views being active (just the rotation) highlighted that. I will add an integration test that enables content sizing based on that test - without platform views - just with rotations in a follow up PR. @rwrz - I believe you were running into problems created by content sizing, but I believe this PR would have fixed your issue - and maybe that was the other commit that fixed your issue. If you are able to provide a small code sample that shows the issue you were having parent constraints I would gladly create an integration test for that as well. Ultimately, it's clear the AtomicBoolean was the problem - but I also believe calling MaybeResizeSurfaceView when unnecessary was contributing to the issue outside of the content sizing use case. |
Only call MaybeResizeSurfaceView if the expected frame_size is not the same size as what is returned from the Dart layout. Also, remove the gating function that prevents viewport metrics from being sent on a resize. The AtomicBoolean that is gating calls back into the engine has to be removed. This is necessary because the buffer for the SurfaceView is only updated once the surface finishes drawing to the screen. Since the frame can be submitted before the buffer is updated (but after the resize call happens), the UI rejects the frame because the buffer dimensions do not match. Therefore, the callback with the updated viewport metrics is necessary for the frame to be re-tried. I believe this AtomicBoolean is ultimately the cause of the initial flake. Since rotations are actually changing the size, the AtomicBoolean would have been set to not send the viewport metrics back when necessary. The call from the engine should remain non-blocking to prevent deadlocks. Fixes: flutter#182242 ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Only call MaybeResizeSurfaceView if the expected frame_size is not the same size as what is returned from the Dart layout. Also, remove the gating function that prevents viewport metrics from being sent on a resize. The AtomicBoolean that is gating calls back into the engine has to be removed. This is necessary because the buffer for the SurfaceView is only updated once the surface finishes drawing to the screen. Since the frame can be submitted before the buffer is updated (but after the resize call happens), the UI rejects the frame because the buffer dimensions do not match. Therefore, the callback with the updated viewport metrics is necessary for the frame to be re-tried. I believe this AtomicBoolean is ultimately the cause of the initial flake. Since rotations are actually changing the size, the AtomicBoolean would have been set to not send the viewport metrics back when necessary. The call from the engine should remain non-blocking to prevent deadlocks. Fixes: flutter#182242 ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Only call MaybeResizeSurfaceView if the expected frame_size is not the same size as what is returned from the Dart layout. Also, remove the gating function that prevents viewport metrics from being sent on a resize.
The AtomicBoolean that is gating calls back into the engine has to be removed. This is necessary because the buffer for the SurfaceView is only updated once the surface finishes drawing to the screen. Since the frame can be submitted before the buffer is updated (but after the resize call happens), the UI rejects the frame because the buffer dimensions do not match. Therefore, the callback with the updated viewport metrics is necessary for the frame to be re-tried.
I believe this AtomicBoolean is ultimately the cause of the initial flake. Since rotations are actually changing the size, the AtomicBoolean would have been set to not send the viewport metrics back when necessary.
The call from the engine should remain non-blocking to prevent deadlocks.
Fixes: #182242
Pre-launch Checklist
///).