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

[Impeller] moved to bgra10_xr #52019

Merged
merged 17 commits into from
Apr 15, 2024
Merged

[Impeller] moved to bgra10_xr #52019

merged 17 commits into from
Apr 15, 2024

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Apr 10, 2024

fixes flutter/flutter#145933

This required that we moved the golden image tests to arm64 since the wide gamut tests would now require BGRA10_XR and that's only available to arm64.

tests: in framework repo https://github.com/flutter/flutter/tree/master/dev/integration_tests/wide_gamut_test. There was a test added to that suite specifically for this case when we turned off BGRA10_XR the first time.

This has a dependency on #51998 which includes the necessary skia change.

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

@gaaclarke
Copy link
Member Author

@gaaclarke
Copy link
Member Author

gaaclarke commented Apr 10, 2024

The failure seems to be related to using the bgra10_xr format on the CI machine. It passes locally on my machine, this may be a limit to their GPU?

[ RUN      ] Play/AiksTest.BlendModePlusAlphaColorFilterWideGamut/Metal
2024-04-10 08:45:52.059 impeller_unittests[68274:476517] *** Terminating app due to uncaught exception 'CAMetalLayerInvalid', reason: 'unsupported extended range format'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007ff80e3c011a __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007ff80dee60b7 objc_exception_throw + 48
	2   CoreFoundation                      0x00007ff80e3bff80 +[NSException raise:format:] + 214
	3   QuartzCore                          0x00007ff815bfc3c1 -[CAMetalLayer setPixelFormat:] + 339
	4   impeller_unittests                  0x0000000109bf62a2 _ZN8impeller17PlaygroundImplMTLC2ENS_18PlaygroundSwitchesE + 2210
	5   impeller_unittests                  0x0000000109bf4cce _ZN8impeller14PlaygroundImpl6CreateENS_17PlaygroundBackendENS_18PlaygroundSwitchesE + 94
	6   impeller_unittests                  0x0000000109bf15a4 _ZN8impeller10Playground12SetupContextENS_17PlaygroundBackendERKNS_18PlaygroundSwitchesE + 132
	7   impeller_unittests                  0x000000010a2dc0d6 _ZN8impeller14PlaygroundTest5SetUpEv + 262
	8   impeller_unittests                  0x000000010a58f4c3 _ZN7testing4Test3RunEv + 163
	9   impeller_unittests                  0x000000010a590537 _ZN7testing8TestInfo3RunEv + 343
	10  impeller_unittests                  0x000000010a5911c5 _ZN7testing9TestSuite3RunEv + 805
	11  impeller_unittests                  0x000000010a59fedd _ZN7testing8internal12UnitTestImpl11RunAllTestsEv + 1101
	12  impeller_unittests                  0x000000010a59fa0b _ZN7testing8UnitTest3RunEv + 123
	13  impeller_unittests                  0x0000000109ccd37d main + 445
	14  dyld                                0x00007ff80df1341f start + 1903
)
libc++abi: terminating due to uncaught exception of type NSException

@gaaclarke gaaclarke marked this pull request as ready for review April 10, 2024 17:27
@gaaclarke
Copy link
Member Author

@chinmaygarde @jonahwilliams this is ready for review now, please don't land it until I get back into the office though.

Copy link
Member

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

Overall LGTM, I'd want @zanderso to double check the CI configs though

@gaaclarke
Copy link
Member Author

Overall LGTM, I'd want @zanderso to double check the CI configs though

Zach is out of office for a minute so I'm going to land this.

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 15, 2024
@auto-submit auto-submit bot merged commit 092710a into flutter:main Apr 15, 2024
33 checks passed
@jonahwilliams jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Apr 15, 2024
Copy link
Contributor

auto-submit bot commented Apr 15, 2024

A reason for requesting a revert of flutter/engine/52019 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Apr 15, 2024
@jonahwilliams
Copy link
Member

Reason for revert: I believe the tree failures are due to changes in the build configuration this file introduced.

@jonahwilliams jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Apr 15, 2024
auto-submit bot pushed a commit that referenced this pull request Apr 15, 2024
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Apr 15, 2024
auto-submit bot added a commit that referenced this pull request Apr 15, 2024
Reverts: #52019
Initiated by: jonahwilliams
Reason for reverting: I believe the tree failures are due to changes in the build configuration this file introduced.
Original PR Author: gaaclarke

Reviewed By: {jonahwilliams}

This change reverts the following previous change:
fixes flutter/flutter#145933

This required that we moved the golden image tests to arm64 since the wide gamut tests would now require BGRA10_XR and that's only available to arm64.

tests: in framework repo https://github.com/flutter/flutter/tree/master/dev/integration_tests/wide_gamut_test. There was a test added to that suite specifically for this case when we turned off BGRA10_XR the first time.

This has a dependency on #51998 which includes the necessary skia change.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@gaaclarke
Copy link
Member Author

@jonahwilliams
Copy link
Member

@gaaclarke
Copy link
Member Author

gaaclarke commented Apr 16, 2024

Mac Production Engine - Release

link

https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/313370/infra

error

[ RUN      ] Play/AiksTest.CanRenderAdvancedBlendColorFilterWithSaveLayer/Vulkan
[ERROR:flutter/impeller/playground/backend/vulkan/playground_impl_vk.cc(224)] Attempting to initialize a Vulkan playground on macOS where Vulkan cannot be found. It can be installed via MoltenVK and make sure to install it globally so dlopen can find it.
[FATAL:flutter/impeller/playground/playground_impl.cc(40)] Check failed: false. Attempted to create playground with backend that isn't available or was disabled on this platform: Vulkan
[ERROR:flutter/fml/backtrace.cc(108)] Caught signal SIGABRT during program execution.
Frame 0: 0x100d830dc fml::LogMessage::~LogMessage()
Frame 1: 0x100d830cc fml::LogMessage::~LogMessage()
Frame 2: 0x101018e1c impeller::PlaygroundImpl::Create()
Frame 3: 0x101130490 impeller::(anonymous namespace)::MakeVulkanPlayground()
Frame 4: 0x10112f060 impeller::GoldenPlaygroundTest::SetUp()
Frame 5: 0x101148d14 testing::Test::Run()
Frame 6: 0x101149e8c testing::TestInfo::Run()
Frame 7: 0x10114aafc testing::TestSuite::Run()
Frame 8: 0x101159828 testing::internal::UnitTestImpl::RunAllTests()
Frame 9: 0x1011593a8 testing::UnitTest::Run()
Frame 10: 0x100c79598 main
Frame 11: 0x194ffbf28 start

evaluation

This happens when swiftshader isn't available to the test runner. I had to fix this for some of the presubmit steps. There are 2 variables that seem to effect this:

  1. The gn flag --use-glfw-swiftshader
  2. The presence of libvk_swiftshader.dylib in the output directory. I copied what we had for x64 but it seems like there isn't an explicit dependency on swiftshader so it is possible to build the test runner without building swiftshader.

Mac Production Engine - Debug

link

https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/313368/overview

error

[4943/7663] CXX obj/third_party/libcxx/src/filesystem/libcxx.operations.o
FAILED: obj/third_party/libcxx/src/filesystem/libcxx.operations.o 
../../../flutter/buildtools/mac-x64/clang/bin/clang++ -MMD -MF obj/third_party/libcxx/src/filesystem/libcxx.operations.o.d -D_LIBCPP_NO_EXCEPTIONS -D_LIBCPP_NO_RTTI -D_LIBCPP_BUILDING_LIBRARY -DLIBCXX_BUILDING_LIBCXXABI -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D_LIBCPP_DISABLE_AVAILABILITY=1 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../.. -Igen -I../../../third_party/libcxx/include -I../../../third_party/libcxxabi/include -I../../../flutter/build/secondary/third_party/libcxx/config -I../../../third_party/libcxx/src -fno-strict-aliasing -fstack-protector-all --target=arm64-apple-macos -arch arm64 -fcolor-diagnostics -Wall -Wextra -Wendif-labels -Werror -Wno-missing-field-initializers -Wno-unused-parameter -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wno-implicit-int-float-conversion -Wno-deprecated-copy -Wno-psabi -Wno-deprecated-literal-operator -Wno-unqualified-std-cast-call -Wno-non-c-typedef-for-linkage -Wno-range-loop-construct -Wunguarded-availability -Wno-deprecated-declarations -fvisibility=hidden -Wstring-conversion -Wnewline-eof -O2 -fno-ident -fdata-sections -ffunction-sections -g2 -Wno-thread-safety-analysis -fvisibility-inlines-hidden -nostdinc++ -nostdinc++ -fvisibility=hidden -fno-exceptions -stdlib=libc++ -std=c++20 -isysroot ../../../../../osx_sdk/xcode_15a240d/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -mmacosx-version-min=10.14.0  -c ../../../third_party/libcxx/src/filesystem/operations.cpp -o obj/third_party/libcxx/src/filesystem/libcxx.operations.o
clang++: error: clang frontend command failed with exit code 139 (use -v to see invocation)
Fuchsia clang version 18.0.0 (https://llvm.googlesource.com/llvm-project 725656bdd885483c39f482a01ea25d67acf39c46)
Target: arm64-apple-macos
Thread model: posix
InstalledDir: /Volumes/Work/s/w/ir/cache/builder/src/out/ci/mac_debug_arm64/../../../flutter/buildtools/mac-x64/clang/bin
clang++: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang++: note: diagnostic msg: /Volumes/Work/s/w/ir/x/w/recipe_cleanup/tmpyiytyz5o/operations-73c986.cpp
clang++: note: diagnostic msg: /Volumes/Work/s/w/ir/x/w/recipe_cleanup/tmpyiytyz5o/operations-73c986.sh
clang++: note: diagnostic msg: Crash backtrace is located in
clang++: note: diagnostic msg: /Users/chrome-bot/Library/Logs/DiagnosticReports/llvm_<YYYY-MM-DD-HHMMSS>_<hostname>.crash
clang++: note: diagnostic msg: (choose the .crash file that corresponds to your crash)
clang++: note: diagnostic msg: 

********************

evaluation

This is a crash in the complier when compiling files unrelated to this PR for arm64. I am surprised that this would cause things to be compiled for arm64 that were not already compiled for arm64 for debug builds since I the golden tests who were moved to arm64 should be release only builds.

I did notice that the failure is when cross-compiling on an x64 machine to arm64. We might be able to fix that problem by moving to an arm64 machine.


I'm not sure what Mac Production Engine's are, but hopefully we can get them running in presubmit when we reland this.

@jonahwilliams
Copy link
Member

I think the clang crash is an ongoing issue related to the goma / RBE migration

@gaaclarke
Copy link
Member Author

I think the clang crash is an ongoing issue related to the goma / RBE migration

I accidentally submitted my comment before it was finished. I updated it. I hypothesize that we can git rid of that crash by moving that bot to an arm64 mac. Right now it's cross compiling for arm64 on x64.

auto-submit bot pushed a commit that referenced this pull request Apr 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 16, 2024
…146790)

flutter/engine@07ae93c...503e7e8

2024-04-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] moved to bgra10_xr (#52019)" (flutter/engine#52140)
2024-04-15 [email protected] Roll Skia from d27e288efd54 to 0b5df0201734 (1 revision) (flutter/engine#52132)
2024-04-15 [email protected] Roll Skia from 0fe107da5a4e to d27e288efd54 (6 revisions) (flutter/engine#52130)
2024-04-15 [email protected] [Impeller] moved to bgra10_xr (flutter/engine#52019)
2024-04-15 [email protected] Roll Skia from 9e20a146c024 to 0fe107da5a4e (33 revisions) (flutter/engine#52129)
2024-04-15 [email protected] [Impeller] Use Wang's formula for quad/cubic subdivision. (flutter/engine#52079)
2024-04-15 [email protected] [Impeller] organize shaders a bit, make filter shaders use same vertex source. (flutter/engine#52113)

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],[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
gaaclarke added a commit that referenced this pull request Apr 16, 2024
Now that #52019 has landed, we
shouldn't need it anymore.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
…lutter#146790)

flutter/engine@07ae93c...503e7e8

2024-04-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] moved to bgra10_xr (flutter#52019)" (flutter/engine#52140)
2024-04-15 [email protected] Roll Skia from d27e288efd54 to 0b5df0201734 (1 revision) (flutter/engine#52132)
2024-04-15 [email protected] Roll Skia from 0fe107da5a4e to d27e288efd54 (6 revisions) (flutter/engine#52130)
2024-04-15 [email protected] [Impeller] moved to bgra10_xr (flutter/engine#52019)
2024-04-15 [email protected] Roll Skia from 9e20a146c024 to 0fe107da5a4e (33 revisions) (flutter/engine#52129)
2024-04-15 [email protected] [Impeller] Use Wang's formula for quad/cubic subdivision. (flutter/engine#52079)
2024-04-15 [email protected] [Impeller] organize shaders a bit, make filter shaders use same vertex source. (flutter/engine#52113)

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],[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 e: impeller platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Impeller] Convert to BGRAXR_10 for iOS wide gamut surface/offscreen buffers
4 participants