[Impeller] scales blur coverage to match rendered output #47621

gaaclarke · 2023-11-02T21:56:10Z

I'm starting to write tests for the new blur, but I wanted to make tests for the old blur to make sure I understood the coverage rules. I decided to check this in separately while I work on the other blur.

Even though the goal is to delete this class eventually. Most of this work transfers over to the new blur.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2023-11-02T22:41:50Z

impeller/entity/contents/filters/directional_gaussian_blur_filter_contents_unittests.cc

+  std::optional<Entity> result =
+      contents->GetEntity(renderer, entity, coverage_hint);
+  ASSERT_TRUE(result.has_value());
+  ASSERT_EQ(result.value().GetBlendMode(), BlendMode::kSourceOver);


I would like to have better asserts here about the coverage of the result. I'm not 100% up to date about what the values should be considering the coverage hint yet though.

matanlurey

Actual approval should come from someone who understands the implementation IMO

matanlurey · 2023-11-02T22:59:15Z

impeller/renderer/testing/mocks.h

Thanks for adding these!

matanlurey · 2023-11-02T23:20:47Z

impeller/entity/contents/filters/directional_gaussian_blur_filter_contents_unittests.cc

+  entity.SetTransformation(Matrix::MakeTranslation({100, 100, 0}));
+  std::optional<Rect> coverage = contents->GetFilterCoverage(
+      inputs, entity, /*effect_transform=*/Matrix::MakeScale({2.0, 2.0, 1.0}));
+  ASSERT_EQ(coverage, Rect::MakeLTRB(100 - 2, 100, 200 + 2, 200));


Here and elsewhere:

I might either expand on the test name, or leave a comment (or does ASSERT support reasons, i.e. << "Reason") of why this is the expected result. From someone totally unfamiliar with this code, I don't understand what the test would mean if it failed or why this is the expected result.

jonahwilliams

I think this is an example I would use of questionable mock usage. the DirectionalGaussianBlur is written against the HAL, which we can already test on CI with the real implementations. See entity_unittests for examples, but we don't need to, nor should we mock out APIs that are already abstracted from the platform.

matanlurey · 2023-11-02T23:33:59Z

I think this is an example I would use of questionable mock usage. the DirectionalGaussianBlur is written against the HAL, which we can already test on CI with the real implementations. See entity_unittests for examples, but we don't need to, nor should we mock out APIs that are already abstracted from the platform.

What is the value of the "real" HAL here? Or are you saying you wouldn't want to see tests that assert C++ math would want to see golden/UI tests produced from a real backend instead?

jonahwilliams · 2023-11-03T00:04:35Z

Assertions on the contents are great! but you don't need to mock out the HAL to write this test. Just ... use the HAL as it already exists?

jonahwilliams

Basically to do so, you'd take a similar structure to the golden tests like https://github.com/flutter/engine/blob/main/impeller/entity/entity_unittests.cc#L261

Except rather than making an assertion on the pixels, you can introspect on the contents/entity

…ocking. (#47631) As a response to my comments in #47621

gaaclarke · 2023-11-03T17:43:00Z

I think this is an example I would use of questionable mock usage. the DirectionalGaussianBlur is written against the HAL, which we can already test on CI with the real implementations. See entity_unittests for examples, but we don't need to, nor should we mock out APIs that are already abstracted from the platform.

@jonahwilliams I gave entity_unittests a look. Having gone through the google accreditation process for writing c++ tests there are 2 guiding principles applicable here: 1) Everything has tests 2) Prefer concrete classes over mocks.

Preferring concrete classes is why in these tests I used a real ContentContext instead of just mocking that, for example. It's also why I gave your suggestion serious consideration. I mentioned it in the other PR but the approach you are proposing is flawed as-is because this layer of Impeller changes its behavior based on the capabilities of the hardware. So without some effort it cannot be used for all testing needs. One of my goals with this PR is to provide the tools so people can test any logic.

There are some potential scaling issues with that approach as well. Managing hardware profiles can be more cumbersome than directly specifying them in the test with the mocks. All tests will be run 3 times, which assuming the bottom layers are correct (a correct assumption for a unit test), will be redundant. Plus they are incurring extra cost of starting and tearing down the hardware contexts.

I think it's really important we get the type of integration testing we'd get from using the real objects, for that we have the golden image tests. I'm in the middle of writing out the new blur but I'm using this testing harness to make sure I get coverage correct. I'll use golden images to make sure the visual results are correct.

Sound good? I'm happy to discuss further over chat or brainjam on a design that broadens out your testing harness.

gaaclarke · 2023-11-03T17:46:27Z

Ugh, i really messed up this branch. I accidentally had it tracking the branch that I tried to split this from. I'll try to sort it out.

gaaclarke · 2023-11-03T21:44:25Z

Jonah Matan and I met and chatted about this. We agreed to move the tests to the concrete types and minimize the use of mocking to cases where it is unavoidable (such as upspecing the capabilities of the test hardware). We also agreed to come up with a mechanism to downspec the capabilities in these concrete classes to aide testing.

jonahwilliams · 2023-11-06T18:47:07Z

impeller/renderer/testing/mocks.h

+              (override));
+};
+
+class MockCapabilities : public Capabilities {


You don't need to MockCapabilities, there is a CapabilitiesBuilder/StandardCapabilities that lets you set whatever you want.

I just removed all this work for now. They aren't used. I figured it didn't hurt and may potentially save time in the future.

I'll probably end up landing most of this with the other blur anyways where it is actually used. We'll see.

flutter-dashboard · 2023-11-06T19:42:29Z

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 #47621 at sha 20f2fc8

jonahwilliams · 2023-11-06T19:46:28Z

impeller/entity/contents/filters/directional_gaussian_blur_filter_contents_unittests.cc

+    std::optional<Rect> contents_coverage = contents->GetCoverage(entity);
+    EXPECT_TRUE(result_coverage.has_value());
+    EXPECT_TRUE(contents_coverage.has_value());
+    if (result_coverage.has_value() && contents_coverage.has_value()) {


Consider adding an Expect_rect_near macro

That'll problem happen if the new blur has better test coverage like i hope.

jonahwilliams

LGTM

nit: this is adding more testings only right? I would adjust PR title if so.

gaaclarke · 2023-11-06T19:47:27Z

LGTM

nit: this is adding more testings only right? I would adjust PR title if so.

Nope, there is actually a coverage fix in there.

jonahwilliams · 2023-11-06T19:48:17Z

impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc

-      transform.TransformDirection(blur_direction_ * Radius{blur_sigma_}.radius)
+      transform
+          .TransformDirection(blur_direction_ *
+                              Radius{ScaleSigma(blur_sigma_)}.radius)


Oh, missed this!

jonahwilliams · 2023-11-06T19:48:49Z

Oh, I missed that. Maybe "Ensure blur coverage sigma is also scaled..." or something like that.

…lutter/engine#47621)

…137979) flutter/engine@461d815...b1a5d77 2023-11-07 skia-flutter-autoroll@skia.org Roll Skia from 7e3119240ae4 to 9106e374e08d (1 revision) (flutter/engine#47732) 2023-11-07 skia-flutter-autoroll@skia.org Roll Dart SDK from 82731b940e7f to aded6314af29 (1 revision) (flutter/engine#47731) 2023-11-07 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from HQfFSkurc6-jKM2jh... to wmM4lS2lYcphFSHPV... (flutter/engine#47730) 2023-11-07 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from _vGlgDiKOrtlKrZAP... to NaQb_BU6PSbKXSAoU... (flutter/engine#47728) 2023-11-07 skia-flutter-autoroll@skia.org Roll Skia from bd5f57c9bd1a to 7e3119240ae4 (10 revisions) (flutter/engine#47726) 2023-11-06 gspencergoog@users.noreply.github.com Add `KeyEventDeviceType` to `KeyData` (flutter/engine#47315) 2023-11-06 skia-flutter-autoroll@skia.org Roll Skia from 2b218381e226 to bd5f57c9bd1a (2 revisions) (flutter/engine#47719) 2023-11-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 07d4c9d85a5a to 82731b940e7f (1 revision) (flutter/engine#47717) 2023-11-06 skia-flutter-autoroll@skia.org Roll Skia from 77aeee3b81a5 to 2b218381e226 (1 revision) (flutter/engine#47715) 2023-11-06 30870216+gaaclarke@users.noreply.github.com [Impeller] scales blur coverage to match rendered output (flutter/engine#47621) 2023-11-06 jonahwilliams@google.com [Impeller] Fix EntityPassTarget::Flip with implict MSAA. (flutter/engine#47701) 2023-11-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 9211fc6406bb to 07d4c9d85a5a (1 revision) (flutter/engine#47709) 2023-11-06 jonahwilliams@google.com [Impeller] fix drawVertices dest fast path to apply alpha. (flutter/engine#47695) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from _vGlgDiKOrtl to NaQb_BU6PSbK fuchsia/sdk/core/mac-amd64 from HQfFSkurc6-j to wmM4lS2lYcph 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 rmistry@google.com,zra@google.com 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

github-actions bot added the e: impeller label Nov 2, 2023

gaaclarke marked this pull request as draft November 2, 2023 21:57

gaaclarke force-pushed the blur-tests branch 4 times, most recently from f216f4b to e6bdcc6 Compare November 2, 2023 22:39

gaaclarke changed the title ~~[Impeller] backfills blur tests~~ [Impeller] expand mock library and backfills blur tests Nov 2, 2023

gaaclarke commented Nov 2, 2023

View reviewed changes

gaaclarke changed the title ~~[Impeller] expand mock library and backfills blur tests~~ [Impeller] expands mock library and backfills blur tests Nov 2, 2023

gaaclarke requested review from matanlurey and jonahwilliams November 2, 2023 22:42

matanlurey reviewed Nov 2, 2023

View reviewed changes

jonahwilliams requested changes Nov 2, 2023

View reviewed changes

jonahwilliams reviewed Nov 3, 2023

View reviewed changes

jonahwilliams mentioned this pull request Nov 3, 2023

[Impeller] add example of testing entity with "real" HAL instead of mocking. #47631

Merged

auto-submit bot pushed a commit that referenced this pull request Nov 3, 2023

[Impeller] add example of testing entity with "real" HAL instead of m…

d5ccb5b

…ocking. (#47631) As a response to my comments in #47621

gaaclarke changed the title ~~[Impeller] expands mock library and backfills blur tests~~ [Impeller] fixes blur coverage Nov 3, 2023

gaaclarke added 6 commits November 3, 2023 10:19

[Impeller] added new gaussian blur

9554a95

added directional_gaussian unittests to help

3d86c8f

added filtersourcecoverage

48c754e

added more tests

d61ac96

got a possitive rendering test

789a5da

refactor

348f8ee

gaaclarke force-pushed the blur-tests branch from dd86d7a to 8567c94 Compare November 3, 2023 17:19

added coverage fix

b7a328e

gaaclarke force-pushed the blur-tests branch from 8567c94 to b7a328e Compare November 3, 2023 17:45

removed stray gaussian blur files

88e7f35

gaaclarke force-pushed the blur-tests branch from de0724d to 88e7f35 Compare November 3, 2023 18:41

gaaclarke added 5 commits November 3, 2023 13:19

license

33c3faf

moved to entityplayground

05711b8

renamed test

9bade3d

license

d46a891

lint fix

6017a65

gaaclarke marked this pull request as ready for review November 3, 2023 21:46

gaaclarke requested review from jonahwilliams and matanlurey November 3, 2023 21:46

gaaclarke added 3 commits November 3, 2023 15:00

tidy

19f7381

lint

a2b8868

lint, sorry really should have checked these locally

f46ff8c

jonahwilliams reviewed Nov 6, 2023

View reviewed changes

removed mocks

20f2fc8

gaaclarke requested a review from jonahwilliams November 6, 2023 18:51

flutter-dashboard bot added the will affect goldens label Nov 6, 2023

jonahwilliams reviewed Nov 6, 2023

View reviewed changes

jonahwilliams approved these changes Nov 6, 2023

View reviewed changes

jonahwilliams reviewed Nov 6, 2023

View reviewed changes

gaaclarke changed the title ~~[Impeller] fixes blur coverage~~ [Impeller] scales blur coverage to match rendered output Nov 6, 2023

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 6, 2023

auto-submit bot merged commit 99fc852 into flutter:main Nov 6, 2023
29 checks passed

engine-flutter-autoroll mentioned this pull request Nov 7, 2023

Roll Flutter Engine from 461d815b48a0 to b1a5d772792f (13 revisions) flutter/flutter#137979

Merged

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

99fc852d9 [Impeller] scales blur coverage to match rendered output (f…

747dce2

…lutter/engine#47621)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] scales blur coverage to match rendered output #47621

[Impeller] scales blur coverage to match rendered output #47621

gaaclarke commented Nov 2, 2023 •

edited

Loading

gaaclarke Nov 2, 2023

matanlurey left a comment

matanlurey Nov 2, 2023

matanlurey Nov 2, 2023

jonahwilliams left a comment

matanlurey commented Nov 2, 2023

jonahwilliams commented Nov 3, 2023

jonahwilliams left a comment

gaaclarke commented Nov 3, 2023

gaaclarke commented Nov 3, 2023

gaaclarke commented Nov 3, 2023

jonahwilliams Nov 6, 2023

gaaclarke Nov 6, 2023

gaaclarke Nov 6, 2023

flutter-dashboard bot commented Nov 6, 2023

jonahwilliams Nov 6, 2023

gaaclarke Nov 6, 2023

jonahwilliams left a comment

gaaclarke commented Nov 6, 2023

jonahwilliams Nov 6, 2023

jonahwilliams commented Nov 6, 2023

[Impeller] scales blur coverage to match rendered output #47621

[Impeller] scales blur coverage to match rendered output #47621

Conversation

gaaclarke commented Nov 2, 2023 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

matanlurey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

matanlurey commented Nov 2, 2023

jonahwilliams commented Nov 3, 2023

jonahwilliams left a comment

Choose a reason for hiding this comment

gaaclarke commented Nov 3, 2023

gaaclarke commented Nov 3, 2023

gaaclarke commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

gaaclarke commented Nov 6, 2023

Choose a reason for hiding this comment

jonahwilliams commented Nov 6, 2023

gaaclarke commented Nov 2, 2023 •

edited

Loading