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

Update display_list_image_filter_unittests to be permit Skia roll #37327

Merged
merged 3 commits into from
Nov 8, 2022

Conversation

lhkbob
Copy link
Contributor

@lhkbob lhkbob commented Nov 4, 2022

https://skia-review.googlesource.com/c/skia/+/571798 had to be reverted because it causes DisplayListImageFilter.LocalImageFilterBounds to fail. With SkMatrixTransformImageFilter (returned by SkImageFilters::MatrixTransform) supporting complex matrices, makeWithLocalMatrix(rotation) will no longer return null.

This is at odds with the behavior of DlImageFilter's implementation which tries to mimic the existing behavior of image filters. This relaxes the test case so that Skia can improve its implementation without breaking Flutter's tests. Flutter can then update DlImageFilter to match out-of-band.

https://bugs.chromium.org/p/skia/issues/detail?id=9283
flutter/flutter#114723

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

https://skia-review.googlesource.com/c/skia/+/571798 had to be reverted because it causes DisplayListImageFilter.LocalImageFilterBounds to fail. With SkMatrixTransformImageFilter (returned by SkImageFilters::MatrixTransform) supporting complex matrices, makeWithLocalMatrix(rotation) will no longer return null.

This is at odds with the behavior of DlImageFilter's implementation which tries to mimic the existing behavior of image filters. This relaxes the test case so that Skia can improve its implementation without breaking Flutter's tests. Flutter can then update DlImageFilter to match out-of-band.
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the comment update!

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 7, 2022

auto label is removed for flutter/engine, pr: 37327, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 7, 2022

auto label is removed for flutter/engine, pr: 37327, due to - The status or check suite Windows Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 7, 2022

auto label is removed for flutter/engine, pr: 37327, due to Validations Fail.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2022
@flar
Copy link
Contributor

flar commented Nov 7, 2022

The rebase has passed all Windows test targets. so I'm adding the autosubmit label

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 7, 2022

auto label is removed for flutter/engine, pr: 37327, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 7, 2022

auto label is removed for flutter/engine, pr: 37327, due to Validations Fail.

@zanderso
Copy link
Member

zanderso commented Nov 7, 2022

This needed a second review to make the autosubmit bot happy. I approved, but I'll let @flar add back the label if/when appropriate.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2022
@auto-submit auto-submit bot merged commit c6f26e0 into flutter:main Nov 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Nov 9, 2022
…utter#37327)

* Update display_list_image_filter_unittests to be permit Skia roll

https://skia-review.googlesource.com/c/skia/+/571798 had to be reverted because it causes DisplayListImageFilter.LocalImageFilterBounds to fail. With SkMatrixTransformImageFilter (returned by SkImageFilters::MatrixTransform) supporting complex matrices, makeWithLocalMatrix(rotation) will no longer return null.

This is at odds with the behavior of DlImageFilter's implementation which tries to mimic the existing behavior of image filters. This relaxes the test case so that Skia can improve its implementation without breaking Flutter's tests. Flutter can then update DlImageFilter to match out-of-band.

* Update comment to reference flutter issue.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Nov 9, 2022
…115008)

* de0b58e82 clang-tidy: added the ability to shard jobs (flutter/engine#37265)

* 8aefb8b61 Roll Dart SDK from d97d5ad98893 to b22b36c0f52e (1 revision) (flutter/engine#37400)

* 2f043090f Roll Skia from aef6d301c0b5 to 0c8127b3dd7d (6 revisions) (flutter/engine#37402)

* dc7cb20ad Fix boolean json property. (flutter/engine#37403)

* 512fa40b6 Adding release_build:true property to windows builders. (flutter/engine#37397)

* 0af87071f Add test for image readback (flutter/engine#37401)

* c6f26e035 Update display_list_image_filter_unittests to be permit Skia roll (flutter/engine#37327)

* 4e9e97b4b Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter/engine#37409)

* 42c2940ff Pass the correct name for gclient variables in ci.yaml (flutter/engine#37429)

* 2e2a2fd97 Revert "Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (#37409)" (flutter/engine#37430)

* 02cb78905 Roll Fuchsia Mac SDK from sa5bVGimNo3JwLV27... to d4l6A1aPw6Z0YjxmA... (flutter/engine#37432)

* c24ae1872 [web] Use v8BreakIterator where possible (flutter/engine#37317)

* 306b0fe9c Add rects to accumulator rather than bounds (flutter/engine#37435)

* c76035429 Roll Dart SDK from b22b36c0f52e to c15cdb978761 (2 revisions) (flutter/engine#37437)
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
…utter#37327)

* Update display_list_image_filter_unittests to be permit Skia roll

https://skia-review.googlesource.com/c/skia/+/571798 had to be reverted because it causes DisplayListImageFilter.LocalImageFilterBounds to fail. With SkMatrixTransformImageFilter (returned by SkImageFilters::MatrixTransform) supporting complex matrices, makeWithLocalMatrix(rotation) will no longer return null.

This is at odds with the behavior of DlImageFilter's implementation which tries to mimic the existing behavior of image filters. This relaxes the test case so that Skia can improve its implementation without breaking Flutter's tests. Flutter can then update DlImageFilter to match out-of-band.

* Update comment to reference flutter issue.
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
…lutter#115008)

* de0b58e82 clang-tidy: added the ability to shard jobs (flutter/engine#37265)

* 8aefb8b61 Roll Dart SDK from d97d5ad98893 to b22b36c0f52e (1 revision) (flutter/engine#37400)

* 2f043090f Roll Skia from aef6d301c0b5 to 0c8127b3dd7d (6 revisions) (flutter/engine#37402)

* dc7cb20ad Fix boolean json property. (flutter/engine#37403)

* 512fa40b6 Adding release_build:true property to windows builders. (flutter/engine#37397)

* 0af87071f Add test for image readback (flutter/engine#37401)

* c6f26e035 Update display_list_image_filter_unittests to be permit Skia roll (flutter/engine#37327)

* 4e9e97b4b Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter/engine#37409)

* 42c2940ff Pass the correct name for gclient variables in ci.yaml (flutter/engine#37429)

* 2e2a2fd97 Revert "Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter#37409)" (flutter/engine#37430)

* 02cb78905 Roll Fuchsia Mac SDK from sa5bVGimNo3JwLV27... to d4l6A1aPw6Z0YjxmA... (flutter/engine#37432)

* c24ae1872 [web] Use v8BreakIterator where possible (flutter/engine#37317)

* 306b0fe9c Add rects to accumulator rather than bounds (flutter/engine#37435)

* c76035429 Roll Dart SDK from b22b36c0f52e to c15cdb978761 (2 revisions) (flutter/engine#37437)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#115008)

* de0b58e82 clang-tidy: added the ability to shard jobs (flutter/engine#37265)

* 8aefb8b61 Roll Dart SDK from d97d5ad98893 to b22b36c0f52e (1 revision) (flutter/engine#37400)

* 2f043090f Roll Skia from aef6d301c0b5 to 0c8127b3dd7d (6 revisions) (flutter/engine#37402)

* dc7cb20ad Fix boolean json property. (flutter/engine#37403)

* 512fa40b6 Adding release_build:true property to windows builders. (flutter/engine#37397)

* 0af87071f Add test for image readback (flutter/engine#37401)

* c6f26e035 Update display_list_image_filter_unittests to be permit Skia roll (flutter/engine#37327)

* 4e9e97b4b Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter/engine#37409)

* 42c2940ff Pass the correct name for gclient variables in ci.yaml (flutter/engine#37429)

* 2e2a2fd97 Revert "Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter#37409)" (flutter/engine#37430)

* 02cb78905 Roll Fuchsia Mac SDK from sa5bVGimNo3JwLV27... to d4l6A1aPw6Z0YjxmA... (flutter/engine#37432)

* c24ae1872 [web] Use v8BreakIterator where possible (flutter/engine#37317)

* 306b0fe9c Add rects to accumulator rather than bounds (flutter/engine#37435)

* c76035429 Roll Dart SDK from b22b36c0f52e to c15cdb978761 (2 revisions) (flutter/engine#37437)
@lhkbob lhkbob deleted the matrix-filter-test-fix branch January 5, 2024 15:00
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants