engine icon indicating copy to clipboard operation
engine copied to clipboard

Change default TileMode for blur ImageFilter objects to null

Open flar opened this issue 1 year ago • 4 comments

Fixes https://github.com/flutter/flutter/issues/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 avatar Oct 01 '24 09:10 flar

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...

flar avatar Oct 03 '24 00:10 flar

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

chinmaygarde avatar Oct 07 '24 18:10 chinmaygarde

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 avatar Oct 21 '24 23:10 flar

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.

flar avatar Oct 23 '24 02:10 flar

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 a0d78679b03c937f976bf7104e8c4ab6368d6238

flutter-dashboard[bot] avatar Oct 23 '24 12:10 flutter-dashboard[bot]

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)".

flar avatar Oct 23 '24 19:10 flar

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 avatar Oct 23 '24 19:10 jonahwilliams

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

Changes reported for pull request #55552 at sha 768f91b191ef54135cb4ac8e235aac6bd7e28c01

flutter-dashboard[bot] avatar Oct 25 '24 20:10 flutter-dashboard[bot]

The breaking change notice is now waiting on this PR to land: https://github.com/flutter/website/pull/11338

jonahwilliams avatar Oct 28 '24 19:10 jonahwilliams

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

flar avatar Oct 28 '24 23:10 flar

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

Changes reported for pull request #55552 at sha bb0b87dced2335d6432265cc548291017defd9e7

flutter-dashboard[bot] avatar Oct 29 '24 00:10 flutter-dashboard[bot]

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

flar avatar Oct 29 '24 01:10 flar