Repository navigation
Makes sure impeller context's are plumbed through to LayerTree::Flatten. - #191017
Conversation
|
/gemini review |
![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 updates LayerTree::Flatten to accept an optional impeller::AiksContext parameter, propagating it to the PaintContext to support Impeller-enabled rendering. These changes are integrated into DlDeferredImageGPUImpeller and Picture rasterization, and null-safety checks are added to EmbedderExternalTextureGL along with corresponding unit tests. The feedback suggests simplifying redundant ternary checks on aiks_context to directly call aiks_context.get() in display_list_deferred_image_gpu_impeller.cc and picture.cc to improve readability.
8a1357c to
65b7bb5
Compare
fa05d57 to
e9687d8
Compare
|
we might want to consider this for cherry-pickin |
There was a problem hiding this comment.
Code Review
This pull request updates LayerTree::Flatten to accept an impeller::AiksContext parameter, enabling the propagation of Impeller context information during layer tree flattening. These changes are integrated into DlDeferredImageGPUImpeller and Picture::DoRasterizeToImage. Additionally, safety checks are added to EmbedderExternalTextureGL to prevent crashes when the context is null, supported by new unit tests. The review feedback suggests simplifying the implementation in picture.cc and display_list_deferred_image_gpu_impeller.cc by passing the AiksContext pointer directly to Flatten to avoid unnecessary local variables and complex ternary expressions.
| auto aiks_context = is_impeller_enabled | ||
| ? snapshot_delegate->GetAiksContext() | ||
| : nullptr; | ||
| snapshot_display_list = layer_tree->Flatten( | ||
| DlRect::MakeWH(width, height), | ||
| snapshot_delegate->GetTextureRegistry(), | ||
| is_impeller_enabled ? nullptr : snapshot_delegate->GetGrContext(), | ||
| aiks_context.get()); |
There was a problem hiding this comment.
We can simplify this code and avoid the ternary operator type deduction issue with auto and nullptr by passing the AiksContext pointer directly to Flatten. Since the ternary operator uses short-circuit evaluation, GetAiksContext() will only be called if is_impeller_enabled is true, and the temporary std::shared_ptr returned will remain alive until the end of the statement.
snapshot_display_list = layer_tree->Flatten(
DlRect::MakeWH(width, height),
snapshot_delegate->GetTextureRegistry(),
is_impeller_enabled ? nullptr : snapshot_delegate->GetGrContext(),
is_impeller_enabled ? snapshot_delegate->GetAiksContext().get() : nullptr);References
- Suggest simplification and refactoring to enhance readability and maintainability. (link)
andywolff
left a comment
andywolff
left a comment
There was a problem hiding this comment.
LGTM, makes sense to me why this would fix the issue. I confirmed the new unit test fails without the change and passes with it
gaaclarke
added
the
autosubmit
gaaclarke
added
the
cp: stable
Flutter 3.47.1 在 Windows + Impeller 下渲染 DXGI 共享句柄 GPU 外部纹理 （media_kit 视频输出）时，退出直播间会触发路由退场动画的快照路径： LayerTree::Flatten 构造的 PaintContext 中 aiks_context 为空且 GrDirectContext 为 null，EmbedderExternalTextureGL::ResolveTextureSkia 未做空值检查直接解引用，导致 flutter_windows.dll 读取地址 0x0 崩溃。 上游 issue: flutter/flutter#190774, flutter/flutter#191468 修复 PR: flutter/flutter#191017（3.47 分支 cherry-pick #191222， 2026-08-24 合入，故 3.47.1 不含修复） - simple_live_app/.fvmrc: 3.47.1 -> 3.47.7 - simple_live_tv_app/.fvmrc: 3.47.1 -> 3.47.7 - AGENTS.md / README.md: 同步 Flutter 版本说明
Flutter renders with Impeller on iOS and Android, but release builds of this embedder could only use Skia. On Jolla Phone 2026, Impeller removes Skia's long stutters on blur-heavy frames: in a menu animation the slowest tenth of frames went from 52 ms to 39 ms, and the worst from 211 ms to 59 ms. Embedder patch 0015 makes Impeller the default, with FLUTTER_SFOS_IMPELLER=0 to use Skia. Two engine patches make it work here: 0002 applies the embedder's surface transformation in Impeller's OpenGL surface, which ignored it and drew rotated apps sideways, and 0003 backports flutter/flutter#191017, without which a page transition away from a playing video crashed on the raster thread. PORTING has a Rendering section. Tested on Jolla Phone 2026, SFOS 5.2.0.20, in a sandboxed copy of a real app
fixes #190774
There was just some required plumbing that had to happen for impeller with external textures in certain cases.
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.