[Impeller] Playground tests will always run one frame#187603
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Impeller playground and unit tests by removing custom ImGuiBegin wrappers and instead guarding direct ImGui::Begin calls with IsPlaygroundEnabled(). It also updates Playground::OpenPlaygroundHere to execute the render callback and flush command buffers when the playground is disabled, and transitions dl_unittests.cc to use DlTextImpeller instead of DlTextSkia. Feedback on these changes identifies a potential null pointer dereference in Playground::OpenPlaygroundHere if AcquireSurfaceFrame returns nullptr when the playground is disabled, suggesting a null check be added.
| if (!switches_.enable_playground) { | ||
| std::unique_ptr<Surface> surface = impl_->AcquireSurfaceFrame(context_); | ||
| RenderTarget render_target = surface->GetRenderTarget(); | ||
| if (render_callback(render_target)) { | ||
| return impl_->GetContext()->FlushCommandBuffers(); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If AcquireSurfaceFrame fails to acquire a surface (for example, in headless environments or if the context is lost), it can return nullptr. Calling GetRenderTarget() on a nullptr will result in a null pointer dereference and crash the test. A null check should be added for surface before using it.
| if (!switches_.enable_playground) { | |
| std::unique_ptr<Surface> surface = impl_->AcquireSurfaceFrame(context_); | |
| RenderTarget render_target = surface->GetRenderTarget(); | |
| if (render_callback(render_target)) { | |
| return impl_->GetContext()->FlushCommandBuffers(); | |
| } | |
| return false; | |
| } | |
| if (!switches_.enable_playground) { | |
| std::unique_ptr<Surface> surface = impl_->AcquireSurfaceFrame(context_); | |
| if (!surface) { | |
| return false; | |
| } | |
| RenderTarget render_target = surface->GetRenderTarget(); | |
| if (render_callback(render_target)) { | |
| return impl_->GetContext()->FlushCommandBuffers(); | |
| } | |
| return false; | |
| } |
Currently the playground tests do not do any rendering unless they are run as part of a golden test or if the
--enable_playgroundflag is specified. In that state they test very little except that the associated DisplayList is successfully constructed. These changes will cause them to render the initial frame, thus also testing the Impeller pipelines, even if the playground window is not requested and no goldens are being generated.Additionally some tests were fixed:
ImGuicontrols panel must first check if the playground window is activeFML_DCHECKchecks, but the standard CI flow does not run them with DCHECKs enabled.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.