Repository navigation
Reland: Only render views that need to be rendered - #192128
Conversation
github-actions
Bot
added
platform-android
![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 introduces onTextureFrameAvailable and onMarkAllViewsNeedRender callbacks to PlatformDispatcher across native, web, and Android embedding to notify the framework when a texture has a new frame or when all views need to be re-rendered. RendererBinding and TextureBox are updated to handle these events, optimizing rendering by marking views and texture boxes as needing paint or compositing only when necessary. The review feedback suggests optimizing handleTextureFrameAvailable in RendererBinding with an early return when no callbacks are registered, and using the private backing field _onMarkAllViewsNeedRender in EnginePlatformDispatcher for consistency.
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
I run the pr through agent, it points out several issue which i think is valid
| for (final PipelineOwner child in _children) { | ||
| child.flushPaint(); | ||
| } | ||
| onFlushedPaint?.call(dirtyNodes.isNotEmpty); |
There was a problem hiding this comment.
a child pipeline owner may be responsible for drawing a section on part of the flutterview. for example https://gist.github.com/chunhtai/abb5fea62500a3ced1074cc241d66825
a child pipeline owner may be doing expansive calculation to draw the subscreen to not block the parent pipeline owner.
In this case, the dirty node in child pipeline will not notify the parent pipeline owner.
This is a niche case though
There was a problem hiding this comment.
Should we recursively ask there whether the child pipeline owners have any dirty nodes?
There was a problem hiding this comment.
Should we recursively ask there whether the child pipeline owners have any dirty nodes?
I lean towards not doing this.
In the ViewAnchor case (eg, a child window), the child view has a separate child pipeline owner. Ideally if the child view repaints, that does not cause a re-render of the parent view.
I think we do support this scenario of multiple pipeline owners for a single view. If you manually create a pipeline owner, you can pass in a PipelineOwner.onFlushPaint callback that invokes RenderView.markNeedsCompositeFrame on the shared view.
| fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), | ||
| [engine = engine_->GetWeakPtr()]() { | ||
| if (engine) { | ||
| engine->ScheduleFrame(false); |
There was a problem hiding this comment.
ScheduleFrame(false) actually trigger a shortcircuit in Animator::RequestFrame to DrawLastLayerTrees, which completely bypass the dart
There was a problem hiding this comment.
Yes. That is not a problem. The problem only occurs when engine->ScheduleFrame(false); is called and something else triggers layer tree regeneration. This is fixed by the preceding call to engine->NotifyTextureFrameAvailable(texture_id);.
b4f6460 to
0477100
Compare
|
This is ready for re-review. |
c91c984 to
28f856c
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
3bad5ab to
731ec75
Compare
|
@knopp Do we need to update this guy to pass a flutter/examples/layers/rendering/src/binding.dart Lines 55 to 68 in 72dc94b |
|
Reason for revert: broke flutter -> packages autoroller |
|
Successfully created revert PR: #193360 |
flutteractionsbot
removed
the
revert
Reverts: [Reland: Only render views that need to be rendered ](#192128) Initiated by: @elliette Reason for reverting: broke flutter -> packages autoroller Original PR Author: @knopp Reviewed By: @loic-sharma The original PR description is provided below: This relands #179874, reverted in #191252. The reason for revert was stalled web benchmarks (#191251). Stalling was caused by `_autoUpdateBenchmarkPhase` inside `macrobenchmarks/lib/src/web/recorder.dart` expecting certain number of `preroll_frame` and `apply_frame` events. These events are only published for frames that are composited (ui.Scene being rendered), but some of the web benchmarks request frame without making any render objects dirty, which after #191252 did not result on view being composited. The fix is to force view composition on every frame when the benchmarks are running. cc @yjbanov regarding the changes in `recorder.dart`. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [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 in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [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. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. 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. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md Co-authored-by: Elliott Brooks <[email protected]>
6 participants
This relands #179874, reverted in #191252.
The reason for revert was stalled web benchmarks (#191251). Stalling was caused by
_autoUpdateBenchmarkPhaseinsidemacrobenchmarks/lib/src/web/recorder.dartexpecting certain number ofpreroll_frameandapply_frameevents. These events are only published for frames that are composited (ui.Scene being rendered), but some of the web benchmarks request frame without making any render objects dirty, which after #191252 did not result on view being composited.The fix is to force view composition on every frame when the benchmarks are running.
cc @yjbanov regarding the changes in
recorder.dart.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.