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

Run iOS Scenario apps with Skia and Impeller #36652

Merged
auto-submit merged 3 commits into from
Oct 12, 2022
Merged

Run iOS Scenario apps with Skia and Impeller #36652

auto-submit merged 3 commits into from
Oct 12, 2022

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Oct 6, 2022 •
edited
Loading

Create a copy of the Info.plist with FLTEnableImpeller set to true. Run the scenario tests a second time pointed at that Info.plist.

https://ci.chromium.org/p/flutter/builders/try/Mac%20Unopt/19565

2022-10-06 17:13:22.112348-0700 Scenarios[6347:1178387] [VERBOSE-2:ios_context_metal_impeller.mm(22)] Using the Impeller rendering backend.

Skip two tests failing only with impeller: flutter/flutter#113250 and flutter/flutter#113251

Fixes flutter/flutter#113057

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.

@jmagman jmagman self-assigned this Oct 6, 2022
jonahwilliams
jonahwilliams reviewed Oct 7, 2022
View reviewed changes
testing/scenario_app/run_ios_tests.sh
clean test \
FLUTTER_ENGINE="$FLUTTER_ENGINE"

echo "Running simulator tests with Impeller"
Copy link
Contributor

@jonahwilliams jonahwilliams Oct 7, 2022

Choose a reason for hiding this comment

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

Can we skip the golden tests in scenarios app?

Copy link
Member Author

@jmagman jmagman Oct 7, 2022

Choose a reason for hiding this comment

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

They have some wiggle room

engine/testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenImage.m

Lines 93 to 94 in 1a6005f

double rmse = sqrt(sum / size);
if (rmse > kRmseThreshold) {

The existing ones passed on impeller with no changes, maybe we should keep them and turn it off when we have a good reason?

wdyt @zanderso

Copy link
Member

@zanderso zanderso Oct 7, 2022

Choose a reason for hiding this comment

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

Let's leave them on. Seeing where/if they fail would help us understand differences between Skia and Impeller.

Copy link
Member Author

@jmagman jmagman Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh wait, they did fail. I don't know why I thought it was all passing when I wrote that comment.

Failing tests:
	-[UnobstructedPlatformViewTests testOneOverlayAndTwoIntersectingOverlays]
	-[BogusFontTextTest testFontRenderingWhenSuppliedWithBogusFont]

Because of flutter/flutter#74344 (comment) I can't actually see the diff screenshots though.

Copy link
Member Author

@jmagman jmagman Oct 8, 2022

Choose a reason for hiding this comment

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

I'll take a look at this again on Monday.

Copy link
Member Author

@jmagman jmagman Oct 11, 2022

Choose a reason for hiding this comment

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

Filed flutter/flutter#113250 and flutter/flutter#113251

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 11, 2022

Choose a reason for hiding this comment

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

Thank you!

@jmagman jmagman force-pushed the impeller-scenario branch from 5c0e49a to 60468f1 Compare October 7, 2022 21:53
jonahwilliams
jonahwilliams approved these changes Oct 11, 2022
View reviewed changes
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

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 12, 2022
@auto-submit auto-submit bot merged commit 30d57de into flutter:main Oct 12, 2022
@jmagman jmagman deleted the impeller-scenario branch October 12, 2022 00:18
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 12, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 12, 2022
@engine-flutter-autoroll
30d57deea Run iOS Scenario apps with Skia and Impeller (flutter/engin…
ea36d32 
…e#36652)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@zanderso zanderso zanderso approved these changes

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Impeller
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Impeller] Run existing scenario app tests with impeller enabled as well.

4 participants

@jmagman @zanderso @jonahwilliams @chinmaygarde