[wimp] Implement images for wimp. by eyebrowsoffire · Pull Request #183913 · flutter/flutter

eyebrowsoffire · 2026-03-20T00:40:53Z

This change implements image support for wimp, addressing #175371.

Refactored some impeller bits to support images which lazily create their textures on the raster thread. This plumbs a texture cache and impeller context through calls so that images can actually generate their texture using the impeller context when it is actually rendered for the first time, and then use a cached version thereafter.
Implements images from pictures, from pixels, and from external textures in wimp.
Turns golden testing on for wimp unit tests.
Enables a number of web engine unit tests which were skipped due to a lack of image support.
Wrote a few unit tests on the impeller side to ensure the image->texture logic and caching work as expected.

gemini-code-assist

Code Review

This pull request implements image support for wimp, a significant feature for the web impeller backend. The core of the change is the introduction of lazy texture creation for DlImage objects, which is crucial for performance and correctness in the web environment. This is achieved by adding a new GetImpellerTexture method that takes an Impeller context. To avoid re-creating textures, a TextureCache is introduced and plumbed through the rendering pipeline. The changes are well-structured, with updates to the display list dispatcher, canvas, and paint objects to support the new mechanism. Unit tests for the texture caching logic have been added, and a number of existing tests have been enabled for wimp, demonstrating the feature's correctness. Overall, this is a solid implementation. I have one suggestion to reduce some code duplication.

flutter-dashboard · 2026-03-20T19:49:15Z

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #183913 at sha 3de2dbd

This change implements image support for wimp. * Refactored some impeller bits to support images which lazily create their textures on the raster thread. This plumbs a texture cache and impeller context through calls so that images can actually generate their texture using the impeller context when it is actually rendered for the first time, and then use a cached version thereafter. * Implements images from pictures, from pixels, and from external textures in wimp. * Turns golden testing on for wimp unit tests. * Enables a number of web engine unit tests which were skipped due to a lack of image support. * Wrote a few unit tests on the impeller side to ensure the image->texture logic and caching work as expected.

… for it.

eyebrowsoffire · 2026-03-24T22:08:12Z

Turning on the golden tests on for wimp reveals that something is wrong with how the blur filter is working. I started fixing it here, but then reverted those changes and put them into a separate PR to experiment: #184087

We'll probably just ignore/accept the incorrect blur when landing this PR, since it's basically unrelated to the wimp image implementation.

flutter-dashboard · 2026-03-25T01:17:22Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #183913 at sha 8be18be

flar

I've reviewed everything except for the skwasm directory so far. Going to come back to that tomorrow.

Meanwhile, I think @gaaclarke should probably take a look at all the snapshot changes and verify how they manage data now wrt async/sync factories...

flar · 2026-04-16T05:52:20Z

                                  const DlPaint* paint) {
  SkOptionalPaint sk_paint(paint);
-  sk_sp<SkImage> sk_image = image->skia_image();
+  auto skia_image = image ? image->asSkiaImage() : nullptr;


Even with these extra null checks, we'll die eventually when we call skia_image.get() anyway.

flar · 2026-04-16T05:53:42Z

-  canvas_->drawImage(image ? image->skia_image() : nullptr, point.x, point.y,
-                     ToSk(sampling), safe_paint(render_with_attributes));
+  auto skia_image = image ? image->asSkiaImage() : nullptr;
+  canvas_->drawImage(skia_image ? skia_image->skia_image() : nullptr, point.x,


This is an interesting case in that most SkCanvas methods that take an image just NOP on a null image, so these null checks do prevent a crash unlike the ones in the other file that just bounce it down the line.

flar · 2026-04-16T05:54:51Z

                        .setImageFilter(image_filter);
-    builder.DrawImage(DlImage::Make(test_image_src_data->image()),
+    builder.DrawImage(DlImageSkia::Make(test_image_src_data->image()),
+


blank line?

flar · 2026-04-16T05:57:48Z

-    FML_DCHECK(image_color_source &&
-               image_color_source->image()->impeller_texture());
-    auto texture = image_color_source->image()->impeller_texture();
+    auto texture =


flar · 2026-04-16T06:08:43Z

  std::unique_ptr<TextShadowCache> text_shadow_cache_;

+  bool is_texture_caching_enabled_ = false;
+  mutable std::unordered_map<const flutter::DlImage*, std::shared_ptr<Texture>>


Are we worried about image pointers becoming reused due to heap reuse? Does it make sense to have an ID on (at least) DlImageImpeller?

I actually considered that. In practice, at least at the current moment, this is not actually a concern. Wimp is the only one that actually uses this cache, and when wimp images are destroyed, we send a message to the raster thread to clear it from the context's cache. The messages to the raster thread are strictly serially ordered, so that is guaranteed to be processed before any subsequent calls which might create an image at the same heap address and attempt to draw it (and therefore populate a texture in the cache).

If and when we decide to enable this cache and use it on the native side, we'd have to implement a similar strategy anyway for evicting the cache when an image goes away. So I think this is fine.

flar · 2026-04-16T06:11:33Z

  MOCK_METHOD(DlISize, GetSize, (), (const, override));
-  MOCK_METHOD(sk_sp<SkImage>, skia_image, (), (const, override));
  MOCK_METHOD(bool, isOpaque, (), (const, override));
-  MOCK_METHOD(bool, isTextureBacked, (), (const, override));


This is missing after the changes. It looks like the object does have the method, but I guess we don't need a mock if we aren't going to call it in testing? (It's just a "true" stub).

Since MockDlImage inherits from DlImageImpeller, and DlImageImpeller::isTextureBacked() returns true directly (not virtual override of a pure virtual in DlImageImpeller itself, but overrides DlImage), it inherits the implementation that returns true. So it doesn't need to be mocked unless we want to return false in some tests.

flar · 2026-04-16T06:12:08Z

+// |DlImage|
+flutter::DlColorSpace DlDeferredImageGPUImpeller::GetColorSpace() const {
+  if (!wrapper_) {
+    return flutter::DlColorSpace::kSRGB;


Still don't understand the implications here.

flar · 2026-04-16T06:18:00Z

                                raster_task_runner]() {
-    auto image = dl_image->skia_image();
+    auto skia_image = dl_image->asSkiaImage();
+    auto image = skia_image ? skia_image->skia_image() : nullptr;


These lines are repeated all over - maybe make an access utility method somewhere?

flar · 2026-04-16T06:25:45Z

+sk_sp<SkImage> SnapshotControllerImpeller::MakeSkiaTextureImage(
+    sk_sp<SkImage> image,
+    SnapshotPixelFormat pixel_format) {
+  return nullptr;


Some of these are UNREACHABLE, others return null. Should they be consistent?

Is the rule "If you can return, return null, otherwise UNREACHABLE"?

flar · 2026-04-16T06:26:55Z

+    DlISize size,
+    SnapshotPixelFormat pixel_format) {
+  FML_UNREACHABLE();
+  return nullptr;


Ooh, some are UNREACHABLE and also return null...

…aint.

gaaclarke · 2026-04-16T20:55:46Z

Meanwhile, I think @gaaclarke should probably take a look at all the snapshot changes and verify how they manage data now wrt async/sync factories..

I was trying to stay out of it now that flar took over while I was ooo. I was worried of "too many cooks in the kitchen". @eyebrowsoffire let me know if you really want me to review it, I'm happy to defer to flar. I know you already integrated my feedback weeks ago.

flar · 2026-04-16T22:58:05Z

Meanwhile, I think @gaaclarke should probably take a look at all the snapshot changes and verify how they manage data now wrt async/sync factories..

I was trying to stay out of it now that flar took over while I was ooo. I was worried of "too many cooks in the kitchen". @eyebrowsoffire let me know if you really want me to review it, I'm happy to defer to flar. I know you already integrated my feedback weeks ago.

I think I'm pretty much on it. Most of this isn't really publicly exposed yet, but it does touch the decoders and the snapshot delegates and changes the way they deliver their data. It looked fine to me, but I'm not up to speed on all of the different ways we use that code and their threading considerations, so that part might benefit from an extra set of eyes.

flar

I don't see any blocking problems, but left a few questions and suggestions to consider...

flar

LGTM

github-actions Bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine related. See also e: labels. platform-web Web applications specifically e: impeller Impeller rendering backend issues and features requests labels Mar 20, 2026

eyebrowsoffire added the CICD Run CI/CD label Mar 20, 2026

gemini-code-assist Bot reviewed Mar 20, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/display_list/dl_dispatcher.cc

eyebrowsoffire force-pushed the wimp_images branch from 5b8da49 to 58efb94 Compare March 20, 2026 16:19

github-actions Bot removed the CICD Run CI/CD label Mar 20, 2026

eyebrowsoffire added the CICD Run CI/CD label Mar 20, 2026

github-actions Bot removed the CICD Run CI/CD label Mar 20, 2026

eyebrowsoffire added the CICD Run CI/CD label Mar 20, 2026

flutter-dashboard Bot added the will affect goldens Changes to golden files label Mar 20, 2026