Make the filter DAG render all textures at the correct resolution #99

bdero · 2022-03-23T03:54:58Z

This changes a lot about how the filters work and lines us up to make the paint image filter/advanced blends work. A lot of the changes here were made on the fly in order to avoid landing intermediary changes that reduce functionality.

Changes:

All Contents can now compute their own screen space bounds given an entity/transform.
FilterContents computes its bounds as the union of its inputs' bounds.
As a consequence, FilterContents has enough information at render any contents to a texture at the correct end resolution, and so I extended filter inputs to allow for any Contents type in addition to filters and textures. This will make supporting the paint filter very easy.
The gaussian blur filter has a similar interface to Skia's blur now. And now that filters use correct texture sizes, the blur directions are transformed using current transform's basis to match Skia's behavior.

Optimization ideas for later:

Texture inputs are currently transformed and rendered to the correctly sized texture -- we can refactor this pass away by having FilterContents compute either a local transform or mapped UVs per input and give this info to the filter implementations to use for drawing.
There are a bunch of constructors we can add to Matrix to make this overall less pessimized.
Cache bounds rects.

Screen.Recording.2022-03-22.at.7.51.00.PM.mov

Usage:

    auto bridge = CreateTextureForFixture("bay_bridge.jpg");
    auto boston = CreateTextureForFixture("boston.jpg");

    auto blend = FilterContents::MakeBlend(Entity::BlendMode::kPlus, {boston, bridge, bridge});
    auto blur = FilterContents::MakeGaussianBlur(blend, 20, 20);

    Entity entity;
    entity.SetPath(PathBuilder{}.AddRect({100, 100, 300, 300}).TakePath());
    entity.SetContents(blur);
    entity.SetTransformation(my_fancy_transform);

    entity.Render(content_context, render_pass);

chinmaygarde

Overall looks good but I think there is an opportunity to handle this without static pointer casts. I've provided suggestions inline but happy to discuss alternatives. Rest are API UX and styling nits.

chinmaygarde · 2022-03-23T16:01:14Z

aiks/aiks_unittests.cc

@@ -442,6 +442,7 @@ TEST_F(AiksTest, TransformMultipliesCorrectly) {
             -3,   0,   0,   0,
              0,   0,   0,   0,
           -500, 400,   0,   1));
+  // clang-format on


Good catch.

chinmaygarde · 2022-03-23T16:05:37Z

entity/contents/filters/gaussian_blur_filter_contents.cc

-void DirectionalGaussianBlurFilterContents::SetClipBorder(bool clip) {
-  clip_ = clip;
+void DirectionalGaussianBlurFilterContents::SetBlurVector(Vector2 blur_vector) {
+  if (blur_vector.GetLengthSquared() < 1e-3f) {


Maybe we should put Scalar kEhCloseEnough = 1e-3f; in scalar.h as I've seen this used in a couple of spots now.

Done. Good name. 😄

chinmaygarde · 2022-03-23T16:05:55Z

entity/contents/filters/gaussian_blur_filter_contents.cc

@@ -3,8 +3,11 @@
 // found in the LICENSE file.

 #include "impeller/entity/contents/filters/gaussian_blur_filter_contents.h"
+#include <valarray>


Uber small nit: Newline between different include types.

chinmaygarde · 2022-03-23T16:10:59Z

entity/contents/filters/blend_filter_contents.h

@@ -11,7 +11,8 @@ namespace impeller {
 class BlendFilterContents : public FilterContents {
 public:
  using AdvancedBlendProc = std::function<bool(
-      const std::vector<std::shared_ptr<Texture>>& input_textures,
+      const std::vector<std::tuple<std::shared_ptr<Texture>, Rect>>&


Not a huge fan of tuples from an API UX perspective. It is hard to tell what the Rect is meant to be and even harder to document it (later perhaps) using headerdoc and have editors and IDEs provide inline hints for it. Prefer a struct that does the same.

sgtm, I had a thonk about this and ended up standardizing around "snapshot" for this. Let me know what you think.

chinmaygarde · 2022-03-23T16:12:02Z

entity/contents/filters/filter_contents.cc

@@ -48,32 +51,29 @@ std::shared_ptr<FilterContents> FilterContents::MakeBlend(
      new_blend->SetBlendMode(blend_mode);
      blend = new_blend;
    }
-    return std::get<std::shared_ptr<FilterContents>>(blend);
+    auto contents = std::get<std::shared_ptr<Contents>>(blend);
+    // This downcast is safe because we know blend is a BlendFilterContents.


FML_DCHECK(contents->IsFilter) << "This downcast is safe because we know blend is a BlendFilterContents"

IsFilter no longer exists, but maybe we can do some type trickery to compare the vtable ptr? Stuff like &contents->RenderToTexture == &FilterContents::RenderToTexture doesn't work in clang.

chinmaygarde · 2022-03-23T16:19:30Z

entity/contents/contents.h

@@ -33,6 +34,14 @@ class Contents {
                      const Entity& entity,
                      RenderPass& pass) const = 0;

+  /// @brief Returns true if this Contents is a FilterContents. This is useful
+  ///        for downcasting to FilterContents without RTTI.
+  virtual bool IsFilter() const;


I wonder if we are going down the path of reinventing RTTI (poorly)? I works great for simpler use cases but quickly gets out of hand.

Presumably, you are using this so you can perform the filter resolve later. Can this not be handled by a virtual that returns a resolved texture that only filters can provide an implementation for?

Done. I added RenderToTexture to Contents instead of having the filter do this dispatch inline.

Note that all contents can actually be rendered to a texture, which makes your approach doubly good.
We'll likely need this in a number of non-filter situations down the line too, so we would have eventually ended up with this design by necessity. Now that I think about it, RenderToTexture is probably the real star of the show in this PR. :)

…s through the filter graph, make gaussian blur interface match Skia better

… a virtual

bdero requested review from chinmaygarde and removed request for chinmaygarde March 23, 2022 03:54

bdero force-pushed the bdero/better-filter-chaining branch from 7133ce2 to 61f017b Compare March 23, 2022 06:02

bdero requested review from chinmaygarde, dnfield and flar March 23, 2022 08:29

bdero force-pushed the bdero/better-filter-chaining branch from 97b8df0 to e853a2c Compare March 23, 2022 08:54

chinmaygarde reviewed Mar 23, 2022

View reviewed changes

bdero requested a review from chinmaygarde March 23, 2022 23:49

bdero added 8 commits March 23, 2022 19:42

Geometry additions

cf54bd0

Add GetBounds to all Contents

776142e

Support using any Contents for filter parameters, track bounds/offset…

7eff5ab

…s through the filter graph, make gaussian blur interface match Skia better

Fix gaussian blur UVs

69ef94f

Transform inputs properly

015ccef

Make test adjustments

9714b5b

Tests

4f3c1e7

Address comments, make texture snapshotting logic live in Contents as…

98b7fbd

… a virtual

bdero force-pushed the bdero/better-filter-chaining branch from 22a0abc to 98b7fbd Compare March 24, 2022 02:43

chinmaygarde approved these changes Mar 24, 2022

View reviewed changes

bdero merged commit b9c7af2 into flutter:main Mar 24, 2022

zanderso mentioned this pull request Mar 25, 2022

Roll impeller to 0ce2e789fc0852d6d9ee63fb68097327447cd7d8 flutter/engine#32264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make the filter DAG render all textures at the correct resolution #99

Make the filter DAG render all textures at the correct resolution #99

Uh oh!

bdero commented Mar 23, 2022 •

edited

Loading

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Mar 23, 2022

Uh oh!

chinmaygarde Mar 23, 2022

Uh oh!

bdero Mar 23, 2022

Uh oh!

chinmaygarde Mar 23, 2022

Uh oh!

bdero Mar 23, 2022

Uh oh!

chinmaygarde Mar 23, 2022

Uh oh!

bdero Mar 23, 2022

Uh oh!

chinmaygarde Mar 23, 2022

Uh oh!

bdero Mar 24, 2022

Uh oh!

chinmaygarde Mar 23, 2022

Uh oh!

bdero Mar 23, 2022 •

edited

Loading

Uh oh!

Uh oh!

Make the filter DAG render all textures at the correct resolution #99

Make the filter DAG render all textures at the correct resolution #99

Uh oh!

Conversation

bdero commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde 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!

bdero Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdero commented Mar 23, 2022 •

edited

Loading

bdero Mar 23, 2022 •

edited

Loading