Chainable texture filters #67

bdero · 2022-03-09T21:20:22Z

Add FilterContents class that take zero or more texture inputs and generates a result texture.
- Any input may either be an impeller::Texture or another FilterContents which will get resolved at render time.
- When the FilterContents is rendered with an entity, it draws the output texture to the parent pass via TextureContents and respects the entity's transform/stencil/path.
- By default, the size of the first input texture is used as the size of the result texture, but filters may override this behavior depending on the use case. For example, a convolution filter (such as a Gaussian blur) may optionally increase the size of the result texture by a factor of the blur radius.
Add BlendFilterContents filter for simple additive blending/merging textures together.
- Currently, input textures that don't match the size of the first texture stretch.
Add Entity::BlendMode::kPlus blend mode.

Right now the test just blends 3 fixtures together. Will make fancier tests once we have less trivial filters to work with.

Usage:

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

    // Draws kalimba and alpha blends boston over it.
    auto blend0 = FilterContents::MakeBlend(
        Entity::BlendMode::kSourceOver, {kalimba, boston});

    // Adds bridge*3 and the result of the first blend together.
    auto blend1 = FilterContents::MakeBlend(
        Entity::BlendMode::kPlus, {bridge, bridge, blend0, bridge});

    Entity entity;
    entity.SetPath(PathBuilder{}.AddRect({100, 100, 300, 300}).TakePath());
    entity.SetContents(blend1);
    entity.Render(context, pass);

bdero · 2022-03-10T19:05:01Z

This is ready for review

chinmaygarde

This is great. I left perhaps one too many nits but the mechanics look good.

chinmaygarde · 2022-03-10T18:46:57Z

entity/contents/filter_contents.h

+  ///
+  ///        The number of required or optional textures depends on the
+  ///        particular filter's implementation.
+  void SetInputTextures(InputTextures& input_textures);


I think the pass by reference (because of the &) necessitates an unavoidable copy at the caller and you are now depending on the move ctor of the vector to not trash the container on the off chance the caller wants to reuse it. It doesn't but this pattern is somewhat dangerous nonetheless.

The unavoidable copy is because the caller can't move the vector in even if the wanted to because it has to be by reference. Ideally, you want to avoid vector copies both at the call-site and the implementation. The following signature is allows for that:

void SetInputTextures(InputTextures input_textures);

At the call-site, if the caller wants to move the container, they may do foo.SetInputTextures(std::move(texes)) and you are avoiding the copy in the implementation. OTOH, if the want to keep those textures around for further use, they may just not use the move and incur the copy themselves. They need the container, so they should have to copy it out.

I think in this case, the signature with the least bits and bobs in it is the most elegant.

Whoops! Thanks for the great explanation. Done.

chinmaygarde · 2022-03-10T18:49:30Z

entity/contents/filter_contents.cc

+      return texture->GetSize();
+    }
+  }
+  return {0, 0};


Nit: The default ctor zero initializes. So just {} ought to be fine. This is good too.

chinmaygarde · 2022-03-10T18:50:11Z

entity/contents/filter_contents.cc

+}
+
+ISize FilterContents::GetOutputSize() const {
+  if (!input_textures_.empty()) {


Nit: A general note, prefer early returns over nesting. This is fine too for now but may get out of hand in other instances.

Agreed, done.

chinmaygarde · 2022-03-10T18:53:43Z

entity/contents/filter_contents.cc

+
+ISize FilterContents::GetOutputSize() const {
+  if (!input_textures_.empty()) {
+    if (std::holds_alternative<std::shared_ptr<FilterContents>>(


Nit: Prefer std::get_if over std::holds_alternative and get. One, its shorter. But, more importantly, you are performing the second get without the check and this may be brittle if we add more variants.

Something like:

if (auto a = std::get_if<FooA>(var)) { return a->dosomething(); } if (auto b = std::get_if<FooB>(var)) { return a->dosomethingelse(); } FML_UNREACHABLE();

Nice, done.

chinmaygarde · 2022-03-10T18:56:39Z

entity/contents/filter_contents.cc

+  auto uniform_view = host_buffer.EmplaceUniform(frame_info);
+
+  for (const auto& texture : input_textures) {
+    Command cmd;


Drive by comment: Commands are copyable. So you can create commands that are similar except for some fields outside the for loop and update just the relevant bindings in the loop before adding the command to the pass (don't forget to remove the std::move though).

Nice, moved most of this setup out.

chinmaygarde · 2022-03-10T18:58:41Z

entity/contents/filter_contents.h

+
+  FilterContents();
+
+  ~FilterContents();


Nit:: Missing override.

bdero added 3 commits March 9, 2022 13:20

Initial filter attempt

3386cf0

Better default behavior

1141a05

Add BlendFilter

2f86758

bdero force-pushed the bdero/filter branch from 9468c7c to 2f86758 Compare March 9, 2022 21:22

bdero added 2 commits March 9, 2022 15:56

Remove dest rect

59ce84d

map uvs correctly

2e09fa8

bdero changed the title ~~WIP: Chainable texture filters~~ Chainable texture filters Mar 10, 2022

bdero requested review from chinmaygarde and dnfield and removed request for chinmaygarde March 10, 2022 00:46

Update test

8fdc32e

chinmaygarde approved these changes Mar 10, 2022

View reviewed changes

Address comments

819f06a

bdero force-pushed the bdero/filter branch from 6341d3c to 819f06a Compare March 10, 2022 20:20

chinmaygarde approved these changes Mar 10, 2022

View reviewed changes

bdero merged commit cfbde9a into flutter:main Mar 10, 2022

bdero mentioned this pull request Mar 14, 2022

[Impeller] Add advanced (non Porter-Duff) blend modes flutter/flutter#100086

Closed

Chainable texture filters #67

Chainable texture filters #67

Uh oh!

Conversation

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

Uh oh!

Uh oh!

bdero commented Mar 10, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bdero commented Mar 9, 2022 •

edited

Loading