Update DisplayList rendering unittests for new SufaceProvider API and to be able to test Impeller by flar · Pull Request #185820 · flutter/flutter

flar · 2026-04-30T10:27:54Z

#185270 created a refactored SurfaceProvider API that enables better control over which backend tests and benchmarks can use. While the primitive rendering benchmarks were updated for the new code, the rendering validation tests (display_list_rendertests) were still using a hybrid of the old API and some back doors to test on various platforms. This PR updates the rendering tests to use the new API, fills out its implementation on all of the supported platforms and makes the rendering validation tests more appropriate to run on all platforms.

It also removes a few outdated tests that we no longer care about, such as:

How do the bounds of a DisplayList compare to the bounds of an equivalent SkPicture?
Does the Skia backend draw the same thing if we execute commands directly on its SkCanvas compared to recording those same commands in SkPicture or DisplayList?
Do the SkPicture and DisplayList contain approximately the same number of operations?

We still test the following:

Does the rendering operation produce actual output for every combination of attributes (no remaining cases where the rendering back end simply punts on an operation)
Does the rendering draw outside of the bounds declared by the DisplayList?
When we draw something with and without an attribute or context mutation (clip, transform, or save layer) does it actually produce the same or different pixels compared to our expectation?
If a combination of rendering operation and attributes recorded in a DisplayList claims that it supports "group opacity" (i.e. the ability to apply any opacity from outside the DisplayList to each rendering operation so that the caller does not have to wrap the display list in a SaveLayer to achieve the group opacity) then does it do that correctly when we render the constructed DisplayList with a requested group opacity?

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-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.

…st Impeller

…ndering-unittests

gemini-code-assist

Code Review

This pull request refactors the display list testing framework to unify snapshotting capabilities across Skia and Impeller backends. Key changes include adding SnapshotToPixelData and SnapshotToImage to the DlSurfaceInstance interface, implementing backend-specific pixel data wrappers, and updating PlaygroundSwitches handling. Review feedback identifies an off-by-one error in Impeller bounds checking, a significant memory over-allocation in Skia pixel data, and potential null pointer dereferences. It also highlights an inconsistency where the Impeller implementation returns a live texture instead of a static snapshot, which could lead to flaky tests.

gaaclarke

Mostly looks good. I'm confused what this change is doing, can you update the description please? The description talks about what is being removed clearly.

Please make smaller PRs.

gaaclarke · 2026-05-04T23:26:25Z

-    playground_ = MakePlayground(impeller::PlaygroundBackend::kMetalSDF);
+    impeller::PlaygroundSwitches switches;
+    switches.enable_wide_gamut = true;
+    switches.flags.use_sdfs = true;


This is redundant information, can't we just infer it from the PlaygroundBackend?

The use_sdfs is set when you ask for MetalSDF, but it isn't really mentioned by the method which takes a set of switches as an argument so the chain of responsibility for how the switches is set up is not really explicit.

gaaclarke · 2026-05-04T23:29:44Z

+        if (pixels_.get()) {
+          pixmap_.reset(info, pixels_.get(), info.minRowBytes());
+          if (raster_image->readPixels(pixmap_, 0, 0)) {
+            is_valid_ = true;


I would like it if we could leave the "IsValid" pattern behind and use absl::StatusOr with factories instead. Nothing good can come from constructing invalid objects.

This would be really difficult to untangle. The main issue is that SkPixmap is built around raw pointers and so we need to manage their data storage specially. I could try to work around that and do something with raw pointers, but the Skia versions of these backends has a limited future. Some of the tests could be lifted into the calling method, but in the end, we're going to need an IsValid to know if the pixel readback into the private Pixmap field succeeded.

Also, IsValid is private API. This class is private to this file and the method is not exposed through any public API. It is only for the calling internal method to know if the Skia APIs succeeded in the constructor. I agree that public non-IsValid objects are bad, but these aren't public (unless they are valid).

Yea, trying to fix it everywhere is out of scope of this PR. But since we are editing this class we can do the right thing here by introducing the factory method and always returning true for IsValid().

I'm not sure I was clear. There is no "everywhere" here, IsValid doesn't escape this file and is only used in one place. The work that can stump validity has to be done in a constructor, not a factory. So, even if I hide this behind a factory, the constructor needs to have to have a way to return a failure mode. And if it does so, does it matter if that validity is fielded by a factory method in this class or another method in this same file?

I might be able to get around this by constructing a Pixmap temporarily for the readback. It looks like it's just an "annotated pointer". I'd still need a good way to transfer the allocated memory from the factory to the constructor...

Actually, switching this was quite simple.

SkPixmap is just a holder of related data. It doesn't "own" any of it, including not owning the pixel pointer. So, it can be declared, used for read back, and then just dropped on the floor. The only ownership issue is the pixel pointer which is a unique_ptr that can just be transferred into the constructor.

gaaclarke · 2026-05-04T23:30:43Z

+  size_t width() const override { return pixmap_.info().width(); }
+  size_t height() const override { return pixmap_.info().height(); }
+
+  virtual bool write(const std::string& path) const override {


no action required: I'd like it if we used absl::Status here too.

Future work if you file an issue. These are just test utilities, not used in production code.

gaaclarke · 2026-05-04T23:43:19Z

+  static std::string FailureImageDirectory;
+  static bool SaveFailureImages;
+  static std::vector<std::string> FailureImageFilenames;


These don't match naming rules for variables.

The rules are somewhat ambiguous because they define the naming convention for static constant members and define exceptions to that rule. These are likely better served by making the fields private and providing accessor methods for the functionality.

The fields are now private.

gaaclarke · 2026-05-04T23:59:39Z


+std::optional<DlSurfaceProvider::BackendType> DlSurfaceProvider::NameToBackend(
+    const std::string& name) {
+#define PROCESS_BACKEND(name, BACKEND)                             \


Does this need to be a macro? Please add a prefix to avoid colliding macros if you keep it.

Kept the macro, renamed it.

gaaclarke · 2026-05-05T00:00:33Z


+  /// Read back the current contents of the surface and return it as a
+  /// DlPixelData structure.
+  virtual std::shared_ptr<DlPixelData> SnapshotToPixelData() const = 0;


Shouldn't this be returning a unique_ptr?

Why? We don't necessarily need uniqueness and you can't share a unique pointer. shared_ptr seems like the best option here so that we don't limit the use of them by the caller.

It's the opposite of that. By returning shared_ptr you are forcing users to have the semantics for multiple owners. unique_ptr's can be implicitly cast to shared_ptr if someone wants a shared_ptr.

This means I can cache the Providers in the test harness to avoid repeated instantiations. I'm not sure if that will make any performance improvement, but it couldn't hurt. Minimally it saves us from having to allocate the primary surface over and over for those tests that iterate over the providers in an inner loop. Future work...

One down-side to returning a unique_ptr when the caller wants a shared_ptr is that the object is no longer co-allocated with its control block. make_shared does one allocation for both the control block and the object.

gaaclarke

Thanks, changes are looking good. I just have 2 outstanding requests:

Mitigate the IsValid design for the newly added class
return unique_ptr instead of shared_ptr from the factory function

gaaclarke · 2026-05-05T18:23:16Z

+        if (pixels_.get()) {
+          pixmap_.reset(info, pixels_.get(), info.minRowBytes());
+          if (raster_image->readPixels(pixmap_, 0, 0)) {
+            is_valid_ = true;


Yea, trying to fix it everywhere is out of scope of this PR. But since we are editing this class we can do the right thing here by introducing the factory method and always returning true for IsValid().

gaaclarke

thanks jim, lookin good

flar added 3 commits April 30, 2026 02:00

update display_list_rendertests for new SurfaceProvider API and to te…

52819a4

…st Impeller

Merge branch 'master' of github.com:flutter/flutter into update-dl-re…

ea351fd

…ndering-unittests

format and clang

d9e7834

flutter-dashboard Bot added the CICD Run CI/CD label Apr 30, 2026

github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Apr 30, 2026

flar added 2 commits April 30, 2026 13:28

remove unrelated Impeller change (premature potential bug fix)

70ae5ac

conditional impeller metal builds

6b867c5

github-actions Bot removed the CICD Run CI/CD label Apr 30, 2026

flar added the CICD Run CI/CD label Apr 30, 2026

flar added 2 commits April 30, 2026 16:48

Fix missing impeller references on Fuchsia

77666c6

formatting

0265c51

github-actions Bot removed the CICD Run CI/CD label Apr 30, 2026

flar added the CICD Run CI/CD label Apr 30, 2026

flar marked this pull request as ready for review May 1, 2026 02:41

gemini-code-assist Bot reviewed May 1, 2026

View reviewed changes

gemini feedback

1498205

github-actions Bot removed the CICD Run CI/CD label May 1, 2026

flar added the CICD Run CI/CD label May 1, 2026

flar requested review from b-luk, gaaclarke and walley892 May 1, 2026 23:41

gaaclarke reviewed May 5, 2026

View reviewed changes

review feedback

b23f6e0

github-actions Bot removed the CICD Run CI/CD label May 5, 2026

flar added the CICD Run CI/CD label May 5, 2026

flar requested a review from gaaclarke May 5, 2026 10:59

gaaclarke reviewed May 5, 2026

View reviewed changes

review feedback: macro name, unique_ptr and remove unnecessary IsValid

72753ec

github-actions Bot removed the CICD Run CI/CD label May 5, 2026

flar added the CICD Run CI/CD label May 5, 2026

flar requested a review from gaaclarke May 5, 2026 23:58

gaaclarke approved these changes May 6, 2026

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label May 6, 2026

auto-submit Bot added this pull request to the merge queue May 6, 2026

Merged via the queue into flutter:master with commit 240c85c May 6, 2026
201 of 202 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 6, 2026

flar mentioned this pull request May 6, 2026

[Impeller] validation errors and missing output using drawPaint with an ImageFilter #185920

Open

Uh oh!

Conversation

flar commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

flar commented Apr 30, 2026 •

edited

Loading