[Impeller] Add support for ImageFilter.shader #53490

jonahwilliams · 2024-06-20T16:59:00Z

Copy-pasta docs:

  /// Creates an image filter from a [FragmentShader].
  ///
  /// The fragment shader provided here has additional requirements to be used
  /// by the engine for filtering. The first uniform value must be a vec2, this
  /// will be set by the engine to the size of the bound texture. There must
  /// also be at least one sampler2D uniform, the first of which will be set by
  /// the engine to contain the filter input.
  ///
  /// For example, the following is a valid fragment shader that can be used
  /// with this API. Note that the uniform names are not required to have any
  /// particular value.
  ///
  /// ```glsl
  /// #include <flutter/runtime_effect.glsl>
  ///
  /// uniform vec2 u_size;
  /// uniform float u_time;
  ///
  /// uniform sampler2D u_texture_input;
  ///
  /// out vec4 frag_color;
  ///
  /// void main() {
  ///   frag_color = texture(u_texture_input, FlutterFragCoord().xy / u_size) * u_time;
  ///
  /// }
  ///
  /// ```
  ///
  /// This API is only supported when using the Impeller rendering engine. On
  /// other backends a [UnsupportedError] will be thrown. This error can be
  /// caught and used for feature detection.

Fixes jonahwilliams/flutter_shaders#34
Fixes jonahwilliams/flutter_shaders#26
Fixes flutter/flutter#132099

…ader

flutter-dashboard · 2024-07-05T21:24:03Z

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.

Changes reported for pull request #53490 at sha 77e290b

flutter-dashboard · 2024-07-08T23:47:42Z

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

Changes reported for pull request #53490 at sha 5bb8824

flutter-dashboard · 2024-07-09T07:53:05Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

jonahwilliams · 2024-11-07T17:07:29Z

From offline discussion, lets add a Matrix option to provide some means of transforming the input pixels.

…ader

flutter-dashboard · 2024-11-07T20:33:45Z

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

Changes reported for pull request #53490 at sha 2edae62

jonahwilliams · 2024-11-07T21:23:33Z

NVM we're not going to do the matrix approach. Adding @gaaclarke for the impeller side of things

flar

The Dart API and the DisplayList parts look good to me. I only looked through some of the changes to Impeller to implement it.

flar · 2024-11-07T20:37:00Z

display_list/effects/dl_image_filter_unittests.cc

+  DlRuntimeEffectImageFilter filter_c(
+      nullptr, {nullptr}, std::make_shared<std::vector<uint8_t>>(1));
+
+  EXPECT_NE(filter_a, filter_c);


Also a version that is different based on samplers? I suppose it would be too hard to test equality if the shader was different - or do we have a RTE test fixture somewhere you could use for testing?

flar · 2024-11-07T20:40:16Z

display_list/effects/dl_image_filter_unittests.cc

+  EXPECT_EQ(output_bounds, input_bounds);
+}
+
+TEST(DisplayListImageFilter, RuntimeEffectModifiesTransparentBlack) {


Not an issue with this PR, but this engaged a more global thought in me...

This reminds me that the name is a little off because the shader might modify transparent black and that's OK. What this method really is asking/answering is "does it modify transparent black pixels everywhere on the entire plane regardless of whether they were really "involved" in the original operation" which is a question apropos mostly for a CF, but IF needs to answer as well in case it is wrapping a CF. But the name itself doesn't really capture the question being asked. Perhaps ...WithPrejudice() or something...

flar · 2024-11-07T20:43:24Z

impeller/entity/contents/runtime_effect_contents.cc

          FML_DCHECK(renderer.GetContext()->GetBackendType() ==
                     Context::BackendType::kVulkan);
          ShaderUniformSlot uniform_slot;
-          uniform_slot.name = uniform.name.c_str();


Not sure why this is deleted???

gaaclarke

This is looking good from my perspective. Code looks good, API looks good, documentation looks good. I just have a few style nits and a question about the user experience when errors happen.

display_list/dl_builder.cc

gaaclarke · 2024-11-07T21:43:00Z

display_list/effects/dl_image_filter.h


+class DlRuntimeEffectImageFilter final : public DlImageFilter {
+ public:
+  explicit DlRuntimeEffectImageFilter(


nit: I wish these were in a .cc file. Recompilation time was killing me the other day.

There are a few things I'd like to clean up here as well (and the other files in this directory). (There is a .cc file for each, we just don't use it much.)

Move more code to the .cc file as @gaaclarke is suggesting.

Deskiafying the geometry and other simple objects, naturally.

Disallow stack allocation and make them all shared pointers. It's great for unit tests where we don't care about performance, but causes extra copies in the primary code. We can still inline them into the DL records with private constructors, but if any DlOpReceiver code wants to save them "for later", it then needs to deal with reconstructing a shared version. :( Still, the amount of annoyance to deal with stack allocation vs shared_ptr feels like it should be cleaned up. For example, if you ever put it into a DlPaint, we have to re-share it when we should just keep a (shared) copy of the original.

Having them be shared pointers means we never construct a NOP filter because it would notice that it was NOP and return a null instead of a constructed object that has an identity (am I useful?) crisis.

Have different files for the different types??? This would explode the source files in this directory, but isolate the implementations. There would still be a main "dl_image_filter.h" that included the others as it needs to support the type and asFoo() mechanisms, but we wouldn't have one giant regurgitation of code in the main file.

Use more vectors instead of count/ptr pairs.

gaaclarke · 2024-11-07T21:56:11Z

display_list/skia/dl_sk_conversions.cc

+    case DlImageFilterType::kRuntimeEffect:
+      // UNSUPPORTED.
+      return nullptr;
+      break;


nit: break isn't needed.

gaaclarke · 2024-11-07T21:58:49Z

impeller/display_list/aiks_dl_runtime_effect_unittests.cc

+  auto runtime_stages =
+      OpenAssetAsRuntimeStage("runtime_stage_filter_example.frag.iplr");
+
+  auto runtime_stage =


Suggested change

auto runtime_stage =

std::shared_ptr<RuntimeStage> runtime_stage =

gaaclarke · 2024-11-07T22:00:14Z

impeller/display_list/image_filter.cc

          FilterInput::Make(WrapInput(inner_dl_filter.get(), input)));
    }
+    case flutter::DlImageFilterType::kRuntimeEffect: {
+      auto fragment_program_filter = filter->asRuntimeEffectFilter();


Suggested change

auto fragment_program_filter = filter->asRuntimeEffectFilter();

const DlRuntimeEffectImageFilter* fragment_program_filter = filter->asRuntimeEffectFilter();

gaaclarke · 2024-11-07T22:02:37Z

impeller/display_list/image_filter.cc

+      auto fragment_program_filter = filter->asRuntimeEffectFilter();
+      FML_DCHECK(fragment_program_filter);
+      const flutter::DlRuntimeEffectImageFilter* runtime_filter =
+          fragment_program_filter->asRuntimeEffectFilter();


We already have this fragment_program_filter, right? I think auto is just making it hard to see. The autos here in general aren't helping.

gaaclarke · 2024-11-07T22:05:15Z

impeller/display_list/image_filter.cc

+          return nullptr;
+        }
+        auto* image = sampler->asImage();
+        if (!sampler->asImage()) {


Suggested change

if (!sampler->asImage()) {

if (!image) {

gaaclarke · 2024-11-07T22:09:42Z

impeller/entity/contents/filters/runtime_effect_filter_contents.cc

+    VALIDATION_LOG
+        << "Invalid fragment shader in RuntimeEffectFilterContents. "
+        << "Shader must have at least one sampler and a vec2 size uniform.";


How does this manifest for users when they have an invalid shader? Are they getting dart exceptions, crashes, is this message visible to them? Is rendering after this aborted?

I think ideally this would not abort any rendering, the error would be visible to the filter being rendered in the UI, the user could hot reload to address the issue.

Rendering a magenta rect with a loud console message would be fine too.

I believe we should always detect this in dart:ui and throw an exception when the filter is constructed:

https://github.com/flutter/engine/pull/53490/files#diff-1deac5f38d2c30929d40af54c24c990c604c6b7d7bb21a103d28ffd50c8dcb91R4102

However, we also need to check here since there may eventually be non dart users of this funciton. returning nullopt here causes the filter to not show up but anything that was not rendered as a child of the filter will still show uo.

Okay, sounds good to me. Can we expand the dart test to test these 2 cases individually, uniform size being wrong and texture inputs being wrong? Right now it just tests them together.

gaaclarke · 2024-11-07T22:17:36Z

impeller/fixtures/runtime_stage_filter_example.frag

+out vec4 frag_color;
+
+void main() {
+  frag_color = texture(u_texture, FlutterFragCoord().xy / u_size);


Should this do something more noticeable like swizzle color channels for the golden test?

Yeah @flar suggested making this more interesting as well, working on that...

I was hoping for something location based which is one of the "features" of these new shaders. Just swizzling color channels in place or globally doesn't necessarily tell you that it is getting the right locations.

Gave it a shot

This reverts commit 933a4ce.

This reverts commit bc0a436.

gaaclarke · 2024-11-07T23:11:51Z

lib/ui/painting.dart

+    if (!_impellerEnabled) {
+      throw UnsupportedError('ImageFilter.shader only supported with Impeller rendering engine.');
+    }
+    if (shader._floats.length < 2 || !shader._validateImageFilter()) {


Can this error message be more apropos to what they did wrong. For example if they are missing just the sampler, can we just have the error talk about the sampler? This will save developers time as they respond to the problem.

… into image_filter_shader

gaaclarke

lgtm, thanks!

gaaclarke · 2024-11-07T23:34:57Z

lib/ui/painting.dart

+      if (invalidFloats) {
+        buffer.write('The shader has fewer than two float uniforms.\n');
+      }
+      if (invalidSampler) {
+        buffer.write('The shader is missing a sampler uniform.\n');
+      }


Oh nice, I thought this was a good idea too. I didn't want to pester you about it though, thanks.

flutter-dashboard · 2024-11-08T00:45:08Z

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

Changes reported for pull request #53490 at sha 10e9e59

…ne#53490)

…158362) flutter/engine@8e19915...44d788f 2024-11-08 [email protected] [Impeller] Add support for ImageFilter.shader (flutter/engine#53490) 2024-11-07 [email protected] Roll Skia from 2ed9606702f1 to 7ae36ecfe93d (4 revisions) (flutter/engine#56442) 2024-11-07 [email protected] Roll Dart SDK from 6a3684b96121 to 50c620224f27 (1 revision) (flutter/engine#56438) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Copy-pasta docs: ``` /// Creates an image filter from a [FragmentShader]. /// /// The fragment shader provided here has additional requirements to be used /// by the engine for filtering. The first uniform value must be a vec2, this /// will be set by the engine to the size of the bound texture. There must /// also be at least one sampler2D uniform, the first of which will be set by /// the engine to contain the filter input. /// /// For example, the following is a valid fragment shader that can be used /// with this API. Note that the uniform names are not required to have any /// particular value. /// /// ```glsl /// #include <flutter/runtime_effect.glsl> /// /// uniform vec2 u_size; /// uniform float u_time; /// /// uniform sampler2D u_texture_input; /// /// out vec4 frag_color; /// /// void main() { /// frag_color = texture(u_texture_input, FlutterFragCoord().xy / u_size) * u_time; /// /// } /// /// ``` /// /// This API is only supported when using the Impeller rendering engine. On /// other backends a [UnsupportedError] will be thrown. This error can be /// caught and used for feature detection. ``` Fixes jonahwilliams/flutter_shaders#34 Fixes jonahwilliams/flutter_shaders#26 Fixes flutter#132099

jonahwilliams added 6 commits June 16, 2024 13:26

first draft

95a3554

Merge branch 'main' of github.com:flutter/engine into image_filter_sh…

587271b

…ader

++

a299234

format

ec1b101

add basic doc comment.

2f9f72d

rename

1f206da

github-actions bot added the e: impeller label Jun 20, 2024

++

0ec37a8

github-actions bot added the platform-web Code specifically for the web engine label Jun 20, 2024

jonahwilliams added 12 commits June 20, 2024 11:12

add ignore

c3d1e0d

add unit testing.

c4ce95d

fix include.

b07ef04

add aiks test.

216b604

++

e851999

++

217253b

++

6ead435

8 uint 8s?

d848d66

not sure why this works.

59cecc3

Merge branch 'main' of github.com:flutter/engine into image_filter_sh…

c786ea8

…ader

fixups for entity transform.

cda52b0

++

77e290b

jonahwilliams changed the title ~~[WIP] Add support for ImageFilter.shader~~ [Impeller] Add support for ImageFilter.shader Jul 5, 2024

jonahwilliams marked this pull request as ready for review July 5, 2024 21:22

flutter-dashboard bot added the will affect goldens label Jul 5, 2024

Merge branch 'main' into image_filter_shader

5bb8824

jonahwilliams marked this pull request as draft July 9, 2024 07:51

Merge branch 'main' into image_filter_shader

4c55427

jonahwilliams added 4 commits November 7, 2024 09:45

start adding matrix support.

bc0a436

Merge branch 'main' of github.com:flutter/engine into image_filter_sh…

f592560

…ader

maybe its ok

933a4ce

Merge branch 'main' into image_filter_shader

2edae62

jonahwilliams requested a review from gaaclarke November 7, 2024 21:23

flar approved these changes Nov 7, 2024

View reviewed changes

gaaclarke reviewed Nov 7, 2024

View reviewed changes

jonahwilliams added 4 commits November 7, 2024 15:03

Revert "maybe its ok"

8b8b6ee

This reverts commit 933a4ce.

Revert "start adding matrix support."

41f333c

This reverts commit bc0a436.

partial feedback

a876417

jazz up the shader.

117ed3e

gaaclarke reviewed Nov 7, 2024

View reviewed changes

jonahwilliams added 4 commits November 7, 2024 15:12

Merge branch 'image_filter_shader' of github.com:jonahwilliams/engine…

eaf4ac4

… into image_filter_shader

moar tests

f16d0c6

straggler.

9dd17af

++

c6d3248

gaaclarke approved these changes Nov 7, 2024

View reviewed changes

gaaclarke reviewed Nov 7, 2024

View reviewed changes

fix include.

10e9e59

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2024

auto-submit bot merged commit 44d788f into flutter:main Nov 8, 2024
35 checks passed

engine-flutter-autoroll mentioned this pull request Nov 8, 2024

Roll Flutter Engine from 8e19915c19fc to 44d788f4651b (3 revisions) flutter/flutter#158362

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2024

44d788f46 [Impeller] Add support for ImageFilter.shader (flutter/engi…

ece01b5

…ne#53490)

jonahwilliams mentioned this pull request Jan 31, 2025

[FragmentProgram] Allow defining an ImageFilter flutter/flutter#96823

Closed

	auto runtime_stage =
	std::shared_ptr<RuntimeStage> runtime_stage =

	auto fragment_program_filter = filter->asRuntimeEffectFilter();
	const DlRuntimeEffectImageFilter* fragment_program_filter = filter->asRuntimeEffectFilter();

[Impeller] Add support for ImageFilter.shader #53490

[Impeller] Add support for ImageFilter.shader #53490

Uh oh!

Conversation

jonahwilliams commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Jul 5, 2024

Uh oh!

flutter-dashboard bot commented Jul 8, 2024

Uh oh!

flutter-dashboard bot commented Jul 9, 2024

Uh oh!

jonahwilliams commented Nov 7, 2024

Uh oh!

flutter-dashboard bot commented Nov 7, 2024

Uh oh!

jonahwilliams commented Nov 7, 2024

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

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!

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!

gaaclarke left a comment

Choose a reason for hiding this comment

jonahwilliams commented Jun 20, 2024 •

edited

Loading