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

Conversation

matanlurey
Copy link
Contributor

Work towards flutter/flutter#147259.

Most of this is just me understanding how FilterQuality.* feeds into the rest of the engine, and updating our testing documentation so we all understand how to re-run these tests in the future. Here is an example output of both Impeller and non-Impeller output of image_filter_test:

Screenshot 2024-05-08 at 2 25 53 PM


/// Return the [image] magnified by a factor of 5.
Future<Image> scaleImage5x(Image image, FilterQuality quality) async {
final ImageFilter filter = ImageFilter.matrix(Float64List.fromList(<double>[
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't necessarily combine the image filter w/ filter quality, since I think it will make it harder to tell the difference? You could just use a drawImage call, the FilterQuality on the paint is used in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's fair, definitely the intent is to test as little as possible. Let me make that change.

@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 #52687 at sha e032957

@flutter-dashboard
Copy link

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

Changes reported for pull request #52687 at sha 057cd00

@jonahwilliams
Copy link
Contributor

I think you should try https://api.flutter.dev/flutter/dart-ui/decodeImageFromPixels.html with a red/green or other repeating pattern with exactly one pixel.

@matanlurey
Copy link
Contributor Author

Makes senes to me, let me give it a shot.

@flutter-dashboard
Copy link

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

Changes reported for pull request #52687 at sha 83ce489

Comment on lines 331 to 339
Future<Image> shrink() async {
final Paint paint = Paint()..filterQuality = quality;
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawImage(image, Offset.zero, paint);

final Picture picture = recorder.endRecording();
return picture.toImage(50, 50);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this anymore, the double sampling may be adding more artifacts.

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 modulo jonahs feedback. code looks good, the idea looks good and the output looks good.

@matanlurey
Copy link
Contributor Author

I ended up scaling this down to just testing FilterQuality.none for now, as that most prolifically shows the issue that needs to be addressed by flutter/flutter#147259 - we can add more tests as needed.

@flutter-dashboard
Copy link

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

Changes reported for pull request #52687 at sha 7996a85

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2024
@auto-submit auto-submit bot merged commit b94215c into flutter:main May 10, 2024
@matanlurey matanlurey deleted the image-filter-quality-tests branch May 10, 2024 22:15
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 11, 2024
…148156)

flutter/engine@ba8e0d3...fad88cb

2024-05-10 [email protected] Roll Skia from f69e8967610e to b6186e1b3995 (2 revisions) (flutter/engine#52726)
2024-05-10 [email protected] Simplify GN pools, use in more places (flutter/engine#52721)
2024-05-10 [email protected] Write `dart:ui` golden-file tests testing `FilterQuality.*` (flutter/engine#52687)
2024-05-10 [email protected] Revert "DisplayListBuilder internal reorganization with better rendering op overlap detection" (flutter/engine#52725)
2024-05-10 [email protected] deletes canvas recorder (flutter/engine#52722)
2024-05-10 [email protected] Roll Skia from d1118d56853c to f69e8967610e (1 revision) (flutter/engine#52723)
2024-05-10 [email protected] [Impeller] treat glyph atlas texture as source of truth, remove copy of SkBitmap. (flutter/engine#52567)
2024-05-10 [email protected] Roll Dart SDK from 01121c008f4d to 13fc7db956c7 (2 revisions) (flutter/engine#52716)
2024-05-10 [email protected] Roll Skia from 11d892ce49b6 to d1118d56853c (1 revision) (flutter/engine#52717)

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App will affect goldens

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants