Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@flar
Copy link
Contributor

@flar flar commented Aug 28, 2020

The blur radius was too aggressive for HTML. The results for CanvasKit matched what we say on mobile hardware pretty well, but the HTML blurs were always too blurry. I don't know why the * 2 factor was chosen, but both the Flutter parameters and the HTML parameters are radii so they should be compatible.

I also adjusted it to be an average of the X & Y values rather than a max. I don't have a strong opinion on that, but I think it chooses a middle ground for when the dimensional radii are different.

Here are the before/after images (snapshots from Chrome, but results on Safari were pretty much the same, and Firefox doesn't seem to implement blur). Ignore the shade of the square in the middle - it was animating and the shade just depends on when I clicked "snapshot":

build backend resulting screenshot
before fix HTML before_html
before fix CanvasKit before_canvaskit
after fix HTML after_html
after fix CanvasKit after_canvaskit
(reference) (Android Skia on Moto G4) flutter_01

@flar flar requested review from harryterkelsen and yjbanov August 28, 2020 01:20
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flar
Copy link
Contributor Author

flar commented Aug 28, 2020

In looking at the golden test failures and trying to reproduce them locally I get different test pictures. I notice in the code that it special cases MacOS and allows an extra percent of failures, so does this mean that I should generate the new goldens on a Linux machine for best matching with the CI job?

There are 18 failures:

Expected

These golden diffs were expected due to the non-doubled average radius calculation now used.
(are there canvaskit goldens to compare against?)

  • 2 x backdrop filter
  • compositing image filter (this test uses sigmaX: 1, sigmaY: 3 which shows my max() -> average() change above)

Unexpected

I have no idea how these tests could have been affected by my changes.

  • draw_text_composite_below
  • path_transform_with_arc
  • smoke_test
  • 8 x svg_arc
  • svg_rect
  • 3 x text_with_placeholders

@yjbanov
Copy link
Contributor

yjbanov commented Aug 28, 2020

You can download the images generated by LUCI and use them to update flutter/goldens.

Sorry for the unexpected failures. Those are warnings. Feel free to ignore them.

@flar
Copy link
Contributor Author

flar commented Aug 28, 2020

Ah, now I see that the links at the bottom of the log aren't all failures, some are warnings. Most of the unrelated tests are, as you say, just warnings when I look into them.

But...

The smoke_test failure was not a warning. The entire wrong string was shown and it netted an actual test failure.

Oddly, the backdrop tests were all under the threshold and are not listed as failures, but I will update their golden images anyway. Those 2 tests probably need to be adjusted so that they better detect changes in the rendering.

At least the ImageFiltered test was accurately noted as a failure.

@flar
Copy link
Contributor Author

flar commented Aug 28, 2020

Waiting on Goldens PR: flutter/goldens#107

@flar
Copy link
Contributor Author

flar commented Aug 31, 2020

Forgot to mention this would fix some, but not all, of the idiosyncrasies observed in flutter/flutter#64828

@yjbanov
Copy link
Contributor

yjbanov commented Aug 31, 2020

The smoke test failure is intentional. It tests that our test harness can fail when necessary. Feel free to ignore it. (note to self: hide the output from the smoke test).

@yjbanov
Copy link
Contributor

yjbanov commented Aug 31, 2020

Oddly, the backdrop tests were all under the threshold and are not listed as failures, but I will update their golden images anyway. Those 2 tests probably need to be adjusted so that they better detect changes in the rendering.

Yeah, we're still looking for the right balance here. I was hoping we'd be on Skia Gold by now and not need to have our own logic. I tightened up some of the screenshots. You could tighten up these with a smaller threshold if you are updating the screenshots anyway.

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

LGTM once the tests pass.

@flar
Copy link
Contributor Author

flar commented Aug 31, 2020

OK, I'll adjust the size of the canvas a bit until I get actual failures on the tests that are "passing by threshold" and then resubmit the goldens...

Hmmm, if you change the canvas size then you get a hard failure, so I'll just have to change to match the size of the tested shapes and assume that they would have been up-voted to "failure"...

@flar flar force-pushed the fix-html-blur-radius branch from 4b99550 to 69c6e01 Compare September 4, 2020 18:24
@flar flar merged commit d77c4e5 into flutter:master Sep 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 5, 2020
flar pushed a commit to flutter/flutter that referenced this pull request Sep 8, 2020
* 30b829e Populates fuchsia node actions in semantics updates. (flutter/engine#20451)

* c2e7010 Roll Skia from 1ee21cdfb6fe to 6763a713f957 (1 revision) (flutter/engine#20982)

* 841401d restore FML_DCHECK removed due to a code reviewing error (flutter/engine#20953)

* 367c6db Don't use GetTaskQueueId() in rasterizer as it breaks Fuchsia (flutter/engine#20983)

* b22a8c6 Let FlutterActivity/Fragment/FragmentActivity have an app bundle path override instead of eager resolving during construction (flutter/engine#20769)

* 0f0ae68 Update test Dart code to pass the latest Dart analyzer rules (flutter/engine#20986)

* d77dd31 Manual roll of Dart b29f228f62...016e8880f0 (flutter/engine#20967)

* c7b3d53 Roll Fuchsia Mac SDK from gOI3W1UNU... to EN2ycWLxi... (flutter/engine#20985)

* 6a6986d improve sensitivity of BackdropFilter web tests (flutter/engine#20915)

* 9fc9cb2 Roll Dart SDK from 016e8880f0ab to 0f0cff3922ad (7 revisions) (flutter/engine#20990)

* 242d522 [Android R] Sync keyboard animation with view insets vs Android 11/R/API 30 WindowInsetsAnimation (flutter/engine#20843)

* b4e0896 Roll Fuchsia Linux SDK from 81O8Kg_Rw... to A0PKwETay... (flutter/engine#20998)

* d77c4e5 adjust blur radius for HTML to match CanvasKit (flutter/engine#20840)

* 0628492 Roll Dart SDK from 0f0cff3922ad to f3a9ca88b664 (1 revision) (flutter/engine#21000)

* d1d848e Roll Fuchsia Mac SDK from EN2ycWLxi... to sih5f60Gt... (flutter/engine#20999)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants