[Impeller] Turns on the Metal validator for impeller_unittests.

[gaaclarke]

fixes flutter/flutter#121017

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.

[gaaclarke]

I patched in jonahs change to verify it fixes the problem (i don't have an x86_64 machine to test locally). I'll revert it after the test.

[jonahwilliams]

You'll need one more cmmit to make it work (I messed up the mipmap setting)

[jonahwilliams]

I rebased after the upload to private patch landed. If these all pass we should land it this week

[chinmaygarde]

Does this need another rebase?

[gaaclarke]

Does this need another rebase?

Ahh no I don't think so. It looks like our tests genuinely fail with validation turned on. I can take a look at this later today or next week. I thought this passed locally and didn't follow up with the CI results.

[gaaclarke]

On ci GlyphAtlasTextureIsRecreatedIfTypeChanges failed, but locally it passed for me. I will try rebasing.

[gaaclarke]

Looks like the tests pass on debug builds, but on profile builds and release builds there are validation errors.

[gaaclarke]

I've tried to reproduce the failure in the profile build locally 3 times, but it runs fine locally. I'll attempt a rerun to see if we get the same failure or a different failure. For reference the previous failure was:

[ RUN      ] Play/RendererTest.TheImpeller/Metal

STDERR: 
[INFO:test_timeout_listener.cc(76)] Test timeout of 120 seconds per test case will be enforced.
2023-04-27 15:17:25.839 impeller_unittests[82338:468259] Metal API Validation Enabled
-[MTLDebugDevice newTextureWithDescriptor:]:1008: failed assertion `Texture Descriptor Validation
MTLStorageModeShared not allowed for textures
'

link: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8782624655967358433/+/u/test:_Impeller-golden__dart_and_engine_tests_for_host_release__3_/stdout

reproductions steps

./flutter/tools/gn --runtime-mode=profile --goma --unoptimized --no-lto --mac-cpu=x64
cd flutter/testing
./run_tests.py --type=engine --variant host_profile_unopt

[gaaclarke]

This is jonah's fix to the validation problem: #41001

There is no obvious reason why that change would behave differently on debug and profile builds.

[gaaclarke]

Weird, on "Mac mac_host_engine" the debug build passes, on "Linux mac_unopt" it doesn't pass so we can just run with validation for debug builds.

@jonahwilliams Do you have any theories why we are still running into this issue, even after a rebase and why we seem to be seeing different results locally?

[jonahwilliams]

WTF is "Linux mac_unopt" 🤔

[zanderso]

WTF is "Linux mac_unopt" 🤔

The orchestrator is Linux and the actual build is on a mac. It's an engine v2 thing because we have more Linux capacity.

[jonahwilliams]

Do we know which tests are failing? There is a lot more usage of StorageMode::kHostVisible than the one I removed

[gaaclarke]

I've seen these fail:

• Play/RendererTest.TheImpeller/Metal
• Play/SceneTest.FlutterLogo/Metal

Do we have bots that run on arm64? Maybe we could just turn off x86_64 validation for now.

[gaaclarke]

Oh you know what, you probably need a discrete graphics card to actually get the failure locally. Running through rosetta probably isn't enough. I can simulate this failure with conditional compilation possibly.

In the meantime I've asked godofredo if we can possibly just turn this on for arm64 bots. I'm not sure if we are guaranteed an execution on an arm64 bot.

[gaaclarke]

I can reproduce the problem with the following patch:

--- a/impeller/renderer/backend/metal/texture_mtl.mm
+++ b/impeller/renderer/backend/metal/texture_mtl.mm
@@ -15,6 +15,10 @@ TextureMTL::TextureMTL(TextureDescriptor p_desc,
     : Texture(p_desc), texture_(texture) {
   const auto& desc = GetTextureDescriptor();
 
+#if __x86_64__
+  FML_DCHECK(desc.storage_mode == impeller::StorageMode::kDevicePrivate);
+#endif
+
   if (!desc.IsValid() || !texture_) {
     return;
   }

Naively switching to kDevicePrivate doesn't work since the assumption of kHostVisible is built into TextureMTL::OnSetContents. The apple documentation says we'll have to forego the replaceRegion call and do the following instead:

Don't use this method for textures with a private storage mode. To copy data to a private texture, copy that
data to a texture that has a non-private storage mode, and then use a
[MTLBlitCommandEncoder](https://developer.apple.com/documentation/metal/mtlblitcommandencoder) to copy
the data to the private texture.

[jonahwilliams]

Lets revert the patched I added to upload to device private then. We can deal with x86_64 when we get to it