Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Add subpass blend goldens #40879

Merged
merged 2 commits into from Apr 3, 2023
Merged

Conversation

bdero
Copy link
Member

@bdero bdero commented Apr 3, 2023

Adds a checked macro "loop" for the blend modes in the style of FOR_EACH_DISPLAY_LIST_OP + a quick golden for each blend mode that runs the EntityPass subpass blending paths.

We should also add Aiks tests to verify the filters in a more comprehensive way (including the blend filter). I can do this in a follow-up. The BlendFilter actually uses different shaders than SaveLayer for the advanced blends on Metal -- the two code paths are wildly different, and we've had serious regressions for both cases.

Screen.Recording.2023-04-03.at.1.48.40.AM.mov

@bdero bdero self-assigned this Apr 3, 2023
@bdero bdero changed the title [Impeller] Add blend tests [Impeller] Add subpass blend goldens Apr 3, 2023
@flutter-dashboard
Copy link

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 #40879 at sha 4a3fe49

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me modulo a couple cleanup comments.

Do you think these will be easy to parse these results by human reviewers? The documentation I saw for the blend modes uses pngs of parrots. Do you think maybe we should just copy the documentation's example?

Here's some documentation from skia that could be used that is a bit more simple too: https://skia.org/docs/user/api/skblendmode_overview/

The idea being that it would be easy to look at the documentation to see the difference between what it should be and what is being rendered.

@@ -1939,5 +1910,22 @@ TEST_P(AiksTest, DrawPaintAbsorbsClears) {
ASSERT_EQ(picture.pass->GetClearColor(), Color::CornflowerBlue());
}

Picture BlendModeSaveLayerTest(BlendMode blend_mode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this to an anonymous namespace to avoid collisions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#define _IMPELLER_ASSERT_BLEND_MODE(blend_mode) \
auto enum_##blend_mode = static_cast<std::underlying_type_t<BlendMode>>( \
BlendMode::k##blend_mode); \
if (i != enum_##blend_mode) { \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd turn i into a parameter so that the macro isn't making assumptions about the context it's used in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoping to keep this param out of the macro input of IMPELLER_FOR_EACH_BLEND_MODE if that's cool, since it shouldn't be needed for actual usage. Open to alternative ideas for verifying the macro matches the enum.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a huge deal but here's how you could do it without macros that implicitly capture their context. We don't actually care about the asserted value, just that they are all accounted for, they all exist and there is no duplicates:

#define _IMPELLER_ASSERT_BLEND_MODE(blend_mode) \
  auto enum_##blend_mode = static_cast<std::underlying_type_t<BlendMode>>( \
      BlendMode::k##blend_mode);                                           \
  (void)enum_##blend_mode;
  
#define _IMPELLER_PLUS_ONE(blend_mode) + 1

static constexpr inline bool ValidateBlendModes() {
  IMPELLER_FOR_EACH_BLEND_MODE(_IMPELLER_ASSERT_BLEND_MODE);
  int tally = 0  IMPELLER_FOR_EACH_BLEND_MODE(_IMPELLER_PLUS_ONE);
  if (tally - 1 !=
      static_cast<std::underlying_type_t<BlendMode>>(BlendMode::kLast)) {
    return false;
  }
  return true;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is close, but it doesn't quite prevent duplicates. If kSrcOver was accidentally replaced with a second kSrc (thereby dropping kSrcOver), I think this check would pass?

Copy link
Member

@gaaclarke gaaclarke Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, you'd get a redefinition error when compiling, enum_Src would be declared twice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, nice

@bdero
Copy link
Member Author

bdero commented Apr 3, 2023

Do you think these will be easy to parse these results by human reviewers?

Yeah I think so. Assuming what's being rendered is currently correct, someone introducing a regression will be able to compare with the previous correct golden.

@bdero
Copy link
Member Author

bdero commented Apr 3, 2023

I do like the idea of using the w3c duck. ;) Maybe I can follow-up with that?

Also made a quick issue about improving our blend docs to add inline examples: flutter/flutter#124048

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm I think we could make the tests easier to parse since looking the goldens I can't easily tell if the are correct. This will at least make sure we don't change from the current logic accidentally.

#define _IMPELLER_ASSERT_BLEND_MODE(blend_mode) \
auto enum_##blend_mode = static_cast<std::underlying_type_t<BlendMode>>( \
BlendMode::k##blend_mode); \
if (i != enum_##blend_mode) { \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a huge deal but here's how you could do it without macros that implicitly capture their context. We don't actually care about the asserted value, just that they are all accounted for, they all exist and there is no duplicates:

#define _IMPELLER_ASSERT_BLEND_MODE(blend_mode) \
  auto enum_##blend_mode = static_cast<std::underlying_type_t<BlendMode>>( \
      BlendMode::k##blend_mode);                                           \
  (void)enum_##blend_mode;
  
#define _IMPELLER_PLUS_ONE(blend_mode) + 1

static constexpr inline bool ValidateBlendModes() {
  IMPELLER_FOR_EACH_BLEND_MODE(_IMPELLER_ASSERT_BLEND_MODE);
  int tally = 0  IMPELLER_FOR_EACH_BLEND_MODE(_IMPELLER_PLUS_ONE);
  if (tally - 1 !=
      static_cast<std::underlying_type_t<BlendMode>>(BlendMode::kLast)) {
    return false;
  }
  return true;
}

@bdero bdero merged commit 90b87b1 into flutter:main Apr 3, 2023
37 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
2 participants