Change default TileMode for blur ImageFilter objects to null #55552

flar · 2024-10-01T09:42:02Z

Historically ImageFilter.blur supported setting a TileMode and had a default mode of clamp, but few developers actually set the value and the default was not appropriate for some common uses like as a backdrop filter where the clamp mode produces flashing when scrolling high frequency pixel content underneath a blurred title bar.

This PR removes the default tile mode instead allowing a null value as the default which will allow the engine to use an appropriate context-dependent default tile mode depending on the action being performed. Typically:

decal for rendering operations and saveLayers and ImageFilterLayer
clamp for image operations
mirror for backdrop filters

jonahwilliams

I'm not sure that I followed the discussion on clamp for image operations vs decal. I think it can be a bit fuzzy whether or not something is an image operation, because different paint settings (like mask filter) can lead to something ending up as a saveLayer more or less.

We also need to change this for the web engine.

flar · 2024-10-03T00:32:22Z

I'm not sure that I followed the discussion on clamp for image operations vs decal. I think it can be a bit fuzzy whether or not something is an image operation, because different paint settings (like mask filter) can lead to something ending up as a saveLayer more or less.

We also need to change this for the web engine.

drawImage(Rect/Nine/Atlas) are image operations.
saveLayer is not.

I figure that saveLayer is a collection of rendering and rendering operations have space all around them out to infinity so they should reflect that in the choice of mode.

drawImage et al, the source is a physical image that has edges. Within the bounds of the image it should be smeared from its contents. Granted it will spill over the theoretical "frame" which they might clip out.

If we take some other operation and render it to a temp layer then we need to respect whatever tile mode was chosen for the content even as we perform that operation (whether it is a defaulted tile mode or whether they explicitly set it), so I don't think that case should affect the defaults. It's an implementation detail. The defaults were chosen based on the base operation itself and what is happening...

chinmaygarde · 2024-10-07T18:10:01Z

Triage: This is waiting on an update to the web engine.

flar · 2024-10-21T23:56:57Z

I've plumbed most of the web stuff, but haven't yet gotten it all working. I think I got canvaskit renderer to accept the defaults, but it doesn't implement the tile mode in saveLayer so you can't really tell. web gives me some sort of "null canvas" and "width/height > 0" assertion failure. I haven't tried with skwasm as I don't have that building in my local engine case...

flar · 2024-10-23T02:01:50Z

I'm hoping this last commit fixes all of the tests, but I still want to have a rendering test for the various blur tile mode values to catch specific regressions.

flutter-dashboard · 2024-10-23T12:31:10Z

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 #55552 at sha a0d7867

eyebrowsoffire

Looks good to me, have just a couple non-blocking requests.

eyebrowsoffire · 2024-10-23T15:50:25Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/filters.dart

@@ -60,7 +60,7 @@ abstract class SkwasmImageFilter implements SceneImageFilter {
  ///
  /// The handle is deleted immediately after [borrow] returns. The [borrow]
  /// function must not store the handle to avoid dangling pointer bugs.
-  void withRawImageFilter(ImageFilterHandleBorrow borrow);
+  void withRawImageFilter(ImageFilterHandleBorrow borrow, {ui.TileMode defaultMode = ui.TileMode.clamp});


Two things: can you actually call this defaultTileMode to be a bit more clear, and secondly can you modify the doc comment to mention this parameter and what it does?

I went with defaultBlurTileMode since it only applies to blurs...

flar · 2024-10-23T19:26:47Z

Triage: This is waiting on an update to the web engine.

-ish.

For optics it would be nice if the new default for the backdropfilters worked on all platforms since that is the primary reason to make this change to the defaults. That requires changes to CanvasKit. There are similar changes needed to Skwasm which could be made as part of this PR or as part of the PR that adopts the CanvasKit changes - either way. Then the backdropfilter's tile mode will start working on web (except HTML where there's little hope).

But, meanwhile, this PR could be landed and effect the API change without those changes, it would work for most rendering on CK and Skwasm, but not for BDFs until the above changes are made.

If we can get the web engine to honor the filter mode for BDFs while I finish up some details on this PR, that would be a good path forward so we can announce the breaking change along with "and this works on all platforms (except the deprecated HTML renderer)".

jonahwilliams · 2024-10-23T19:28:13Z

We're a long way out from a release cut, so assuming the only web change that needs to happen is exposing an API on the canvaskit side, landing this earlier SGTM

jonahwilliams

LGTM

flutter-dashboard · 2024-10-25T20:47:24Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55552 at sha 768f91b

jonahwilliams · 2024-10-28T19:16:15Z

The breaking change notice is now waiting on this PR to land: flutter/website#11338

flar · 2024-10-28T23:54:34Z

I have now completed all of the golden tests for this PR. It is ready for final reviews.

flutter-dashboard · 2024-10-29T00:42:16Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55552 at sha bb0b87d

flar · 2024-10-29T01:02:58Z

The goldens look good to me, leaving them untriaged for reviewers.

jonahwilliams

LGTM

eyebrowsoffire

LGTM. I have one tiny non-blocking nitpick.

eyebrowsoffire · 2024-10-29T19:28:54Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/canvas.dart

    final SkwasmImageFilter nativeFilter = SkwasmImageFilter.fromUiFilter(imageFilter);
-    final paintHandle = (paint as SkwasmPaint).toRawPaint();
+    final ui.TileMode? backdropTileMode = nativeFilter.backdropTileMode;
+    final paintHandle = (paint as SkwasmPaint).toRawPaint(/*ui.TileMode.decal*/);


I don't really love this little inline comment here. Either we actually want the argument to be visible, and we shouldn't provide a default value, or we should just nix this comment and allow the default argument to be implicit and not visible in the code. (Also, the comment should probably be like /*defaultBlurTileMode: ui.TileMode.decal*/ since it is a named argument, but I we should just remove it).

The comment above this talks about how there are 2 different filters involved in this operation and one uses mirror and the other uses decal as the defaults. If this comment isn't there then you can see where "mirror" is used, but there is no indication of where "decal" is used because it is implicit. I tried just specifying it manually (without the comments), but the analyzer rejected the idea of manually supplying the value that is the default (why?). So, I put it in comments to satisfy the analyzer while still be indicative of the "2 different defaults" I had described earlier.

Why won't the analyzer just let me say the default without complaining?

If I provide no default in the call arguments then every draw command in this file has to supply decal even though it is the default.

flutter/engine#55552)

…157833) flutter/engine@999797a...57ed5d3 2024-10-29 [email protected] [Flutter GPU] Fix MSAA sample size and HostBuffer alignment. (flutter/engine#56218) 2024-10-29 [email protected] Change default TileMode for blur ImageFilter objects to null (flutter/engine#55552) 2024-10-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Reland: disable AHBs on devices that were upgraded to 29. (#56213)" (flutter/engine#56220) 2024-10-29 [email protected] Made et compilation errors relative to the CWD (flutter/engine#56177) 2024-10-29 [email protected] [Impeller] Reland: disable AHBs on devices that were upgraded to 29. (flutter/engine#56213) 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 [email protected],[email protected] 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

…/engine#55552) Fixes flutter#154935 Historically ImageFilter.blur supported setting a TileMode and had a default mode of `clamp`, but few developers actually set the value and the default was not appropriate for some common uses like as a backdrop filter where the clamp mode produces flashing when scrolling high frequency pixel content underneath a blurred title bar. This PR removes the default tile mode instead allowing a null value as the default which will allow the engine to use an appropriate context-dependent default tile mode depending on the action being performed. Typically: - decal for rendering operations and saveLayers and ImageFilterLayer - clamp for image operations - mirror for backdrop filters

flar force-pushed the ui-blur-image-filter-null-default-tile-mode branch from 4c5d450 to 56a2fcb Compare October 1, 2024 19:56

flar requested review from jason-simmons and jonahwilliams October 1, 2024 20:43

jason-simmons approved these changes Oct 2, 2024

View reviewed changes

jonahwilliams requested changes Oct 2, 2024

View reviewed changes

flar force-pushed the ui-blur-image-filter-null-default-tile-mode branch from 56a2fcb to 1b20a34 Compare October 21, 2024 23:51

github-actions bot added the platform-web Code specifically for the web engine label Oct 21, 2024

flar requested a review from yjbanov October 21, 2024 23:55

flar force-pushed the ui-blur-image-filter-null-default-tile-mode branch from c5ca242 to 30e02d2 Compare October 23, 2024 02:00

flutter-dashboard bot added the will affect goldens label Oct 23, 2024

eyebrowsoffire approved these changes Oct 23, 2024

View reviewed changes

jonahwilliams self-requested a review October 23, 2024 19:28

jonahwilliams approved these changes Oct 23, 2024

View reviewed changes

flar added 10 commits October 24, 2024 15:26

Change default TileMode for blur ImageFilter objects to null

798afe9

fix handling of NOP blur filters

42c999d

fix analyzer errors and add unit tests

163b7b3

implement defaults for web

d65a09f

fix analyzer errors in dart code

97a62a4

finish plumbing defaults through web backends

676db6e

fix legacy platform test defaults

089d1f3

visual test for BackdropFilter default

bcfb66d

renaming and doc comment suggestions from review

2bf5b61

complete plumbing and fix bdf tile modes on web backends

9bb40b3

flar force-pushed the ui-blur-image-filter-null-default-tile-mode branch from 64bb51b to 9bb40b3 Compare October 25, 2024 00:34

flar added 5 commits October 24, 2024 19:30

backdrop filter tile mode golden tests for web

f5e0f78

analyzer

8bb3b4f

adjust previous web goldent test to use backwards compatible tile mode

c629fbb

make skwasm workaround invisible

42fa206

rendering blur tile mode golden tests

768f91b

web rendering golden tests

bb0b87d

flar requested review from eyebrowsoffire and jonahwilliams October 28, 2024 23:54

jonahwilliams approved these changes Oct 29, 2024

View reviewed changes

eyebrowsoffire approved these changes Oct 29, 2024

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 29, 2024

auto-submit bot merged commit cfb19e4 into flutter:main Oct 29, 2024
35 checks passed

engine-flutter-autoroll mentioned this pull request Oct 29, 2024

Roll Flutter Engine from 999797a2f690 to 57ed5d338e7e (5 revisions) flutter/flutter#157833

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 29, 2024

cfb19e449 Change default TileMode for blur ImageFilter objects to null (

50a15b4

flutter/engine#55552)

flar mentioned this pull request Oct 30, 2024

Evaluate ImageFilter.blur tileMode behaviors flutter/flutter#157841

Open

This was referenced Nov 1, 2024

Non-decal tile mode ImageFilter.blur() above a scrollable widget looks glitchy at screen edges flutter/flutter#110318

Closed

Canvaskit and Skwasm need to support blur tile modes for backdrop filters flutter/flutter#157363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default TileMode for blur ImageFilter objects to null #55552

Change default TileMode for blur ImageFilter objects to null #55552

flar commented Oct 1, 2024 •

edited

Loading

jonahwilliams left a comment

flar commented Oct 3, 2024

chinmaygarde commented Oct 7, 2024

flar commented Oct 21, 2024

flar commented Oct 23, 2024

flutter-dashboard bot commented Oct 23, 2024

eyebrowsoffire left a comment

eyebrowsoffire Oct 23, 2024

flar Oct 23, 2024

flar commented Oct 23, 2024 •

edited

Loading

jonahwilliams commented Oct 23, 2024

jonahwilliams left a comment

flutter-dashboard bot commented Oct 25, 2024

jonahwilliams commented Oct 28, 2024

flar commented Oct 28, 2024

flutter-dashboard bot commented Oct 29, 2024

flar commented Oct 29, 2024

jonahwilliams left a comment

eyebrowsoffire left a comment

eyebrowsoffire Oct 29, 2024

flar Oct 29, 2024

flar Oct 29, 2024

Change default TileMode for blur ImageFilter objects to null #55552

Change default TileMode for blur ImageFilter objects to null #55552

Conversation

flar commented Oct 1, 2024 • edited Loading

jonahwilliams left a comment

Choose a reason for hiding this comment

flar commented Oct 3, 2024

chinmaygarde commented Oct 7, 2024

flar commented Oct 21, 2024

flar commented Oct 23, 2024

flutter-dashboard bot commented Oct 23, 2024

eyebrowsoffire left a comment

Choose a reason for hiding this comment

eyebrowsoffire Oct 23, 2024

Choose a reason for hiding this comment

flar Oct 23, 2024

Choose a reason for hiding this comment

flar commented Oct 23, 2024 • edited Loading

jonahwilliams commented Oct 23, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Oct 25, 2024

jonahwilliams commented Oct 28, 2024

flar commented Oct 28, 2024

flutter-dashboard bot commented Oct 29, 2024

flar commented Oct 29, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

eyebrowsoffire left a comment

Choose a reason for hiding this comment

eyebrowsoffire Oct 29, 2024

Choose a reason for hiding this comment

flar Oct 29, 2024

Choose a reason for hiding this comment

flar Oct 29, 2024

Choose a reason for hiding this comment

flar commented Oct 1, 2024 •

edited

Loading

flar commented Oct 23, 2024 •

edited

Loading