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

Conversation

@jonahwilliams
Copy link
Contributor

Generally this is fine to do regardless of whether or not we're on UMA . However I never finished flutter/flutter#123468 so it will only work on metal.

I don't think we have a way to runtime check if metal, so I added an ifdef for macos. Im not sure if we have vulkan on macos tests on CI, so lets find out.

Fixes flutter/flutter#124428

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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. Just a note that what you are doing here to copy the texture is probably something that happens elsewhere and/or is worth capturing for easy use elsewhere.

It would be nice to verify that this unblocks #40998.

Comment on lines +403 to +404
// TODO(https://github.com/flutter/flutter/issues/123468): copying buffers to
// textures is not implemented for GLES/Vulkan.
Copy link
Member

Choose a reason for hiding this comment

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

The comment doesn't seem to align with the #if. The if blocks a platform, the comment talks about backends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't have an ifdef for metal as far as I know. Technically macOS is only metal, except that we can run vulkan locally so this change breaks that without a modification to this code.

We can also fix this by finishing the blit pass implementation and removing the check altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this did break something, but hard to tell from CI logs.

@gaaclarke
Copy link
Member

Close, we get a new error now:

[INFO:test_timeout_listener.cc(76)] Test timeout of 120 seconds per test case will be enforced.
2023-04-07 16:14:10.433 impeller_unittests[28742:155517] Metal API Validation Enabled
-[MTLDebugBlitCommandEncoder generateMipmapsForTexture:]:1109: failed assertion `Generate Mipmaps For Texture Validation
[tex mipmapLevelCount](1) must be > 1.
'
[ERROR:flutter/fml/backtrace.cc(108)] Caught signal SIGABRT during program execution.
Frame 0: 0x7ff80b0f40cb err
Frame 1: 0x7ff813c2e8a4 MTLGetEnvCase<>()
Frame 2: 0x7ff813c18c05 MTLReportFailure
Frame 3: 0x7ff813c11378 _MTLMessageContextEnd
Frame 4: 0x7ff80b99a281 -[MTLDebugBlitCommandEncoder generateMipmapsForTexture:]
Frame 5: 0x10482b753 impeller::TextureMTL::GenerateMipmap()
Frame 6: 0x104814bee impeller::BlitPassMTL::EncodeCommands()
Frame 7: 0x1048148b0 impeller::BlitPassMTL::EncodeCommands()
Frame 8: 0x103f289e0 impeller::Playground::CreateTextureForMapping()
Frame 9: 0x103f28c84 impeller::Playground::CreateTextureForFixture()
Frame 10: 0x103eebeb6 impeller::testing::EntityTest_FilterCoverageRespectsCropRect_Test::TestBody()
Frame 11: 0x1049cc2ac testing::Test::Run()
Frame 12: 0x1049cd265 testing::TestInfo::Run()
Frame 13: 0x1049cdf75 testing::TestSuite::Run()
Frame 14: 0x1049dcb0d testing::internal::UnitTestImpl::RunAllTests()
Frame 15: 0x1049dc574 testing::UnitTest::Run()
Frame 16: 0x104093f5e main
Frame 17: 0x11094a52e start

@gaaclarke
Copy link
Member

}
blit_pass->SetLabel("Mipmap Blit Pass");
blit_pass->AddCopy(buffer->AsBufferView(), dest_texture);
if (texture_descriptor.size.MipCount() > 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to fix this

@gaaclarke
Copy link
Member

Oops looks like thats a different function making that fixture than CreateTextureForDecompressedImage

@jonahwilliams
Copy link
Contributor Author

Had to fix enable_mipmapping. Hopefully we're just using that one function for all playground tests...

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 10, 2023
@auto-submit auto-submit bot merged commit 2e8ee81 into flutter:main Apr 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2023
zhongwuzw pushed a commit to zhongwuzw/engine that referenced this pull request Apr 14, 2023
[Impeller] playground upload to device private on macOS
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 needs tests

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] Play/DisplayListTest.CanDrawImage/Metal fails metal validation

3 participants