Support deferred GPU images in ImageShaders #35323

dnfield · 2022-08-10T22:18:56Z

De-skiafy image shaders and layers
Turn the runtime effect color source into its own object/op
Add a TODO for applying this to text. Image shaders using GPU
images on text foreground or background will not work yet.

Fixes flutter/flutter#109286
Fixes flutter/flutter#105085

Creates a new TODO related to flutter/flutter#95434, specifically around the need for SkParagraph to be decoupled from SkPaint.

@jonahwilliams @c-h-i-a-m-a-k-a-2 FYI - patching this in for local testing should enable image shaders for pattern support the way we want to use them in vector_graphics.

Working on this is making me realize that ImageShader is lacking a dispose method and it should definitely have one. Tracked as part of flutter/flutter#82832.

- De-skiafy image shaders and layers - Turn the runtime effect color source into its own object/op - Add a TODO for applying this to text. Image shaders using GPU images on text foreground or background will not work yet.

chinmaygarde · 2022-08-10T22:25:07Z

display_list/display_list_builder.cc


 #include "flutter/display_list/display_list_builder.h"

+#include "display_list_color_source.h"


Nit: flutter/display_list/display_list_color_source.h for consistency with the rest of the includes?

My IDE does this sometimes and I always miss it. Done.

flar · 2022-08-11T00:31:56Z

lib/ui/painting/paint.cc

+      // This currently is only used by paragraph code. Once SkParagraph does
+      // not need to take an SkPaint, we won't be restricted in this way because
+      // we will not need to access the shader on the UI task runner.
+      if (color_source->owning_context() != DlImage::OwningContext::kRaster) {


I'm wondering where SkParagraph accesses this. AFAIK, they store the SkPaint object and they use it to render to our SkCanvas adapter, but they never actually do anything with its contents. Is this a theoretical concern, or was there a case where we ended up accessing the ImageSource from the wrong thread?

It's access here:

engine/lib/ui/text/paragraph_builder.cc

Lines 468 to 482 in 5f92d36

Paint background(background_objects, background_data);

if (background.isNotNull()) {

SkPaint sk_paint;

style.has_background = true;

style.background = *background.paint(sk_paint);

}

}

if (mask & kTSForegroundMask) {

Paint foreground(foreground_objects, foreground_data);

if (foreground.isNotNull()) {

SkPaint sk_paint;

style.has_foreground = true;

style.foreground = *foreground.paint(sk_paint);

}

Which gets passed along to SkPAragraph eventually

It is copying the paint into a local field, but it isn't accessing the contents of the paint.

Those get held and then sent to us when the paragraph is drawn into a ui.Picture, and then we hold on to it until we eventually use it to render on the raster thread. Or at least that is my understanding of how we interact with SkParagraph.

We shouldn't need to create an SkPaint at all here once @jason-simmons' work is done, AFAIU.

flar · 2022-08-11T00:35:42Z

flow/layers/shader_mask_layer_unittests.cc

  const SkPaint child_paint = SkPaint(SkColors::kYellow);
  auto layer_filter =
      SkPerlinNoiseShader::MakeFractalNoise(1.0f, 1.0f, 1, 1.0f);
+  auto dl_filter = DlColorSource::From(layer_filter);


Eeek. Someone created these random SkShader objects for the tests here that represent nothing we'd ever see in practice and we aren't testing that "we do the right thing with a Fractal Noise shader", we're testing that "we do the right thing with a shader (any shader)". So, we should really just change these to instantiate some token Dl...ColorSource directly and stay away from creating an SkShader and using the ::From() converter. If we don't do that now, we'll eventually do it when we go through and get rid of Skia from the source base so rather than add a usage of ::From() here, switch to a natively constructed Dl*ColorSource, of any type.

... in all of the unit tests ...

And all of the blend mode values should just be s/Sk/Dl/ if we switch to that type for SML construction...

These tests largely assert against a mock canvas object that derives from Skia. I have a slight preference for fixing this up in a follow on patch. WDYT?

The Skia object is obsolete and randomly obscure and completely irrelevant (type-wise) to the test. I've been suggesting replacing rather than adding a new line of code to wrap them wherever we are updating the unit tests.

Hmm, it would complicate the MockCanvas validation at the end of the unit test. I see that issue now. For new unit tests I've been pushing a migration to the new DL comparison method of checking the output, but for existing unit tests that's a bit of a haul.

Point conceded.

But we should update the layer to take a DlBlendMode...

flar · 2022-08-11T00:39:10Z

display_list/display_list_image.h

  ///
  virtual std::shared_ptr<impeller::Texture> impeller_texture() const = 0;

+  virtual bool isOpaque() const = 0;


We should document that this might conservatively return false when it cannot guarantee an opaque image. I'm guessing that it might return false for an ARGB image even if the image consists entirely of every pixel with an alpha value of 255, for instance. And some of our image types just can't tell. So we should include a doc comment here and mention the conservative intention of the return value.

Skia doesn't treat this as whether the image contains any transparent pixels, but rather whether transparent pixels ignore the alpha because of the alpha type of the image info. I think documenting that this conservatively returns false is fine though.

They have a really semantically odd way of combining pixel formats that may or may not refer to an alpha channel with an override setting saying whether the alpha is to be honored, but in the end it boils down to the combination of the two specifying a true pixel format and they don't necessarily honor your request to combine all SkColorType and SkAlphaType - they validate them, replacing some alpha types with others and just rejecting other combinations. In the end, if you created an info with (ARGB, Opaque_Alpha_Type) what you really did was to create an image that has no effective alpha channel and it's really an xRGB image with a really strange set of parameters to say that.

So, what I was saying is true - they are basing the opaque property on the type of the pixel format of the image (i.e. whether it has no alpha channel at all, or whether you specifically combined an alpha-bearing memory layout with a manual opaque override setting) - i.e. the overall combined pixel format is determining the answer.

And I did say that they don't care if an alpha image happens to have all opaque values, they only care what the format is. We're saying the same thing on that point.

We don't really expose pixel formats via DlImage, do we? If we do, I'd strongly recommend not following the confusing and somewhat self-contradicting Skia Color/AlphaType tuple.

Added docs.

flar · 2022-08-11T00:41:08Z

lib/ui/compositing/scene_builder.cc

  auto layer = std::make_shared<flutter::ShaderMaskLayer>(
-      shader->shader(sampling)->skia_object(), rect,
-      static_cast<SkBlendMode>(blendMode));
+      shader->shader(sampling), rect, static_cast<SkBlendMode>(blendMode));


We should have SML take a DlBlendMode now that we're converting it to DlColorSource. I believe it is the same cast, just with s/Sk/Dl/ (I think there is a test case that confirms 1:1 of Sk/Dl blend modes)

flar · 2022-08-11T00:41:33Z

flow/layers/shader_mask_layer.cc

 namespace flutter {

-ShaderMaskLayer::ShaderMaskLayer(sk_sp<SkShader> shader,
+ShaderMaskLayer::ShaderMaskLayer(std::shared_ptr<DlColorSource> shader,


And change blend_mode param to DlBlendMode while we're de-skiafying here...

flar · 2022-08-11T00:45:13Z

lib/ui/painting/fragment_shader.cc

-                                                   sk_sp<SkShader> shader) {
+fml::RefPtr<FragmentShader> FragmentShader::Create(
+    Dart_Handle dart_handle,
+    std::shared_ptr<DlColorSource> shader) {


Should we tighten this to a RuntimeEffectColorSource?

flar · 2022-08-11T00:54:16Z

display_list/display_list_color_source.h

+  bool is_opaque() const override { return false; }
+
+  sk_sp<SkRuntimeEffect> runtime_effect() const { return runtime_effect_; }
+  std::vector<std::shared_ptr<DlColorSource>> samplers() const {


All of these should be const return values (immutable)

(runtime_effect / samplers / uniform_data)

flar · 2022-08-11T00:56:09Z

display_list/display_list_ops.h

  }
 };

+// 4 byte header + 80 bytes for the embedded DlImageColorSource


Is that the right size?

Also, this should be added to the list of DlColorSource samples in display_list_unittests.cc (where you have to specify a buffer size for verifying that the pointer math and dl->bytes() come out right)

I can't seem to find the test you're referring to. For now I deleted these lines. I'm a little concerned about how a std::vector<std::shared_ptr> will be measured across different platforms.

std::vector should be 24 bytes on most modern platforms

An example of the new color source type should be added to the unit tests at:

engine/display_list/display_list_test_utils.cc

Line 137 in e8d5233

{"SetColorSource",

chinmaygarde · 2022-08-18T20:23:53Z

Ping @flar. I believe all comments have been addressed along with tests.

flar

Just a couple of nits about == testing on sk_sp and direct DL rendering in SML. The latter is not required for this to avoid regressions, but I don't think it will work under Impeller until that happens. The former is more about enabling more cases where == succeeds so the current answer does not violate the method contract, it is just less optimal. Either could be managed in a followup if necessary...

flar · 2022-08-18T22:28:26Z

display_list/display_list_color_source.h

+    if (runtime_effect_ != that->runtime_effect_) {
+      return false;
+    }
+    if (uniform_data_ != that->uniform_data_) {


Is this meant to compare their contents? Does it end up comparing their addresses instead?

This compares if they're pointing to the same SkData - I think that's ok for now.

flar · 2022-08-18T22:34:05Z

flow/layers/shader_mask_layer.cc

+  paint.setBlendMode(ToSk(blend_mode_));
+  if (shader_) {
+    paint.setShader(shader_->skia_object());
+  }


This is a case where we might want to have "if (context.leaf_nodes_builder) {" and then do this natively in the builder language to avoid loss of information. I think CFLayer is "builder aware" as an example.

Filed flutter/flutter#109812

…5323)

Support deferred GPU images in ImageShaders

0bcd793

- De-skiafy image shaders and layers - Turn the runtime effect color source into its own object/op - Add a TODO for applying this to text. Image shaders using GPU images on text foreground or background will not work yet.

dnfield requested review from chinmaygarde, flar and jason-simmons August 10, 2022 22:18

dnfield added the e: impeller label Aug 10, 2022

chinmaygarde approved these changes Aug 10, 2022

View reviewed changes

chinmaygarde assigned dnfield Aug 10, 2022

dnfield added 4 commits August 10, 2022 15:41

fix equality comparison

d1480fd

chinmay review

fe19f83

copy paste error

eae6b1a

fix nullptr deref

7f90f10

flar suggested changes Aug 11, 2022

View reviewed changes

c-h-i-a-m-a-k-a-2 mentioned this pull request Aug 12, 2022

<Pattern> Support dnfield/vector_graphics#110

Merged

jonahwilliams mentioned this pull request Aug 16, 2022

Scene.toImage/toImageSync does not include TextureLayer contents flutter/flutter#107576

Closed

dnfield added 2 commits August 17, 2022 10:11

Merge remote-tracking branch 'upstream/main' into gpu_img_shader

a8a4a08

flar feedback

d6afb6d

dnfield requested a review from flar August 17, 2022 20:27

dnfield added 4 commits August 17, 2022 15:03

size test

aeccaa1

test

171ce97

Delete size test

9621246

save file before uploading

40332b6

flar approved these changes Aug 18, 2022

View reviewed changes

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 18, 2022

dnfield mentioned this pull request Aug 18, 2022

Make ShaderMaskLayer code builder aware flutter/flutter#109812

Closed

auto-submit bot merged commit 4fbe41d into flutter:main Aug 18, 2022

dnfield deleted the gpu_img_shader branch August 18, 2022 23:49

engine-flutter-autoroll mentioned this pull request Aug 19, 2022

Roll Flutter Engine from 50cfc83e2c9b to 5c30ead197b9 (6 revisions) flutter/flutter#109826

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 19, 2022

4fbe41d Support deferred GPU images in ImageShaders (flutter/engine#3…

4b7eb35

…5323)

flar mentioned this pull request Apr 18, 2023

Switch layers to DlImageFiter, DlColorFilter, and DlColorSource objects flutter/flutter#100454

Closed


		#include "flutter/display_list/display_list_builder.h"

		#include "display_list_color_source.h"

	Paint background(background_objects, background_data);
	if (background.isNotNull()) {
	SkPaint sk_paint;
	style.has_background = true;
	style.background = *background.paint(sk_paint);
	}
	}

	if (mask & kTSForegroundMask) {
	Paint foreground(foreground_objects, foreground_data);
	if (foreground.isNotNull()) {
	SkPaint sk_paint;
	style.has_foreground = true;
	style.foreground = *foreground.paint(sk_paint);
	}

Support deferred GPU images in ImageShaders #35323

Support deferred GPU images in ImageShaders #35323

Uh oh!

Conversation

dnfield commented Aug 10, 2022

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!

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!

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!

chinmaygarde commented Aug 18, 2022

Uh oh!

flar 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