Repository navigation
Wait for web rendering before first-frame event - #189500
auto-submit[bot] merged 8 commits into
Conversation
github-actions
Bot
added
engine
![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 ensures that the flutter-first-frame browser event waits for any in-progress scene renders to complete before being dispatched by tracking pending renders in _sceneRendersInProgress. Feedback recommends updating the documentation comment for _sceneRendersInProgress to use /// instead of // to comply with the repository style guide.
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
Thanks for this fix! Please take a look at my comment
|
|
||
| final Arena frameArena = Arena(); | ||
|
|
||
| /// The framework reports its first frame from a post-frame callback, while |
There was a problem hiding this comment.
I think a Set<Future<void>> is overkill for what we need here. We really just want to be able to answer the question "has the first batch of renders completed yet?"
What if we just have a Completer<void>? _firstFrameCompleter (and a counter since in multi-view apps we need to wait for multiple render requests to finish) which completes after the first frame has finished rendering? It could look something like this:
int _pendingFirstFrameRenders = 0;
Completer<void>? _firstFrameCompleter = Completer<void>();
// ... inside render():
if (shouldRender) {
final Future<void> sceneRender = renderer.renderScene(scene, target);
if (_firstFrameCompleter != null) {
_pendingFirstFrameRenders++;
// If a render fails, immediately fail the completer.
sceneRender.catchError((Object error, StackTrace stack) {
if (_firstFrameCompleter != null && !_firstFrameCompleter!.isCompleted) {
_firstFrameCompleter!.completeError(error, stack);
}
});
sceneRender.whenComplete(() {
_pendingFirstFrameRenders--;
// When all views in the first frame have finished rendering...
if (_pendingFirstFrameRenders == 0 && _firstFrameCompleter != null) {
if (!_firstFrameCompleter!.isCompleted) {
_firstFrameCompleter!.complete();
}
// Null it out to completely disable tracking for all future frames.
_firstFrameCompleter = null;
}
});
}
await sceneRender;
}and then in _dispatchFirstFrameEventAfterRender:
Future<void> _dispatchFirstFrameEventAfterRender() async {
try {
if (_firstFrameCompleter != null) {
await _firstFrameCompleter!.future;
}
} catch (_) {
// Rendering owns error reporting. A failed render is not a first frame.
return;
}
domWindow.requestAnimationFrame((_) {
domWindow.dispatchEvent(createDomEvent('Event', 'flutter-first-frame'));
});
}This way, we remove the overhead of adding and removing the render Futures to a Set on every frame for the entire app lifetime, and instead only track the thing we actually care about ("has the first frame rendered yet?")
There was a problem hiding this comment.
Implemented, with one deviation: completion also happens at the end of the first invokeOnDrawFrame, not only when a render finishes.
_firstFrameCompleter != null as the only guard breaks two ways, both reproduced in a VM harness copied from renderScene. Renders that outlast the frame interval keep the count above zero: the count oscillates between 1 and 2 for ten frames and never reaches 0, because later frames keep feeding it. A first frame that renders nothing never completes, so anything hiding a loading overlay on flutter-first-frame stays up. Master sent the event unconditionally.
So the guard is a bool _isBuildingFirstFrame, flipped where frameArena.collect() already means the frame is done. Happy to paste the harness runs if useful.
The Set of pending futures becomes a Completer plus a counter, as asked in review. Completion also has to happen at the end of the first invokeOnDrawFrame: a startup animation whose renders outlast the frame interval never lets the count reach zero, and a first frame that renders nothing would otherwise leave the completer pending, which regresses against both master and the Set version for an app that hides its loading screen on the browser event.
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
I have a couple small comments about the tests, but otherwise I think this looks ready to land! Thanks for your contribution
| final Zone innerZone = Zone.current.fork(); | ||
| test('dispatches browser event asynchronously on flutter/service_worker channel', () async { | ||
| // Each dispatcher tracks its own first frame, and only the first one. The | ||
| // singleton's is already over by now, because 'onDrawFrame preserves the |
There was a problem hiding this comment.
nit: just say "previous tests" instead of naming the test above, otherwise this could quickly get out of date
There was a problem hiding this comment.
Done, switched to "previous tests".
flutter-zl
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189500, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Thanks for the reviews, and for merging master in. @harryterkelsen this is blocked on Google testing now. Could you add it to the Google testing queue? It is not in the project yet. One thing I found while looking at it. I am digging into the warm-up path. |
|
Found it, I think. That is source reading rather than a run, since exercising it needs an engine build. If it holds, the release cannot hang off the engine's frame loop, and I would rather have your read on the shape than guess at one. |
flutter-zl
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189500, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
@harryterkelsen |
|
@Yusufihsangorgel Sorry, I forgot to check in on this PR. I updated the branch and re-applied the CICD label. Hopefully the tests deflake this time and we can get this landed. Thanks for your patience |
harryterkelsen
added
the
autosubmit
|
Thanks for re-applying it. I'll keep an eye on the run. |
|
autosubmit label was removed for flutter/flutter/189500, because - The status or check suite Mac_arm64_verify_binaries has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
Bot
removed
the
autosubmit
|
|
harryterkelsen
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
3 participants
The
flutter-first-framebrowser event currently fires as soon as theframework sends the
flutter/service_workerplatform message. Web scenerendering is asynchronous, so the event can remove an HTML splash while
SkWasm is still rasterizing and briefly expose a blank frame.
This change tracks scene renders that are still in progress, waits for them,
and dispatches the event on the next browser animation frame. The tests cover
both the asynchronous browser-event contract and an explicitly gated scene
render, so the latter proves that the event cannot overtake rasterization.
Fixes #189499.
Validation
felt test --gcs-prod --browser chrome test/engine/window_test.dart test/ui/async_rendering_test.dartpassed in all seven selected suites:
dart analyzereported no issues for the implementation and both test files.event in 3/3 cold runs before this change. The first visible Flutter frame
followed the event by 90.3–143.0 ms.
Pre-launch Checklist
///), or this change does not alter public API documentation.If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.