Makes Skia's vkQueueSubmit threadsafe. #45459
Conversation
gaaclarke
force-pushed
the
threadsafe-skia-vkqueuesubmit
branch
3 times, most recently
from
ecc6882 to
0d16c56
Compare
| } | ||
|
|
||
| #if SHELL_ENABLE_VULKAN | ||
| TEST_F(EmbedderTest, |
There was a problem hiding this comment.
@bdero please advice how you want these tests to be. They are just straight copies of the software rendering now. It was the easiest way I could find to test this.
There was a problem hiding this comment.
Oh interesting -- does this test reproduce the problem and fail without your change? And if not, is the existing EmbedderTestMultiBackend.CanRenderGradientWithCompositorOnNonRootLayer sufficient for covering the code in this PR?
If the test doesn't repro the problem, I would just remove it. Otherwise, tossing it into embedder_gl_unittests and writing it using EmbedderTestMultiBackend to cover both GL and Vulkan seems like a good approach for the moment.
Later on we should add software context support to the multi-backend class, but that's perhaps a job for another day.
Basically, anything that's Vulkan-specific gets changed to a switchboard method:
auto& context = GetEmbedderContext(EmbedderTestContextType::kVulkanContext)becomesauto& context = GetEmbedderContext(GetParam());builder.SetRenderTargetType(EmbedderTestBackingStoreProducer::RenderTargetType::kVulkanImage);becomesbuilder.SetRenderTargetType(GetRenderTargetFromBackend(GetParam(), false));- etc
There was a problem hiding this comment.
The problem is a race condition which I can't force to happen. So this doesn't assert that the race condition is gone, but it does exercise the code to make sure that it functions. It's possible that the failure would happen in this test infrequently without the change.
Running embedder_unittests this is the only way I could get this code to be hit. The BUILD.gn file only compiles embedder_gl_unittests.cc if test_enable_gl == true which isn't possible to enable on macos. Let me see if I can detangle some of this.
There was a problem hiding this comment.
I pulled the new tests into a embedder_vk_unittests.cc to mirror the ones in embedder_gl_unittest.cc and embedder_metal_unittests.cc. It has a comment in there that says some of the tests are in embedder_gl_unittests.cc and I added an issue to clean them up more later (noted in comment).
| } | ||
|
|
||
| #if SHELL_ENABLE_VULKAN | ||
| TEST_F(EmbedderTest, |
There was a problem hiding this comment.
Oh interesting -- does this test reproduce the problem and fail without your change? And if not, is the existing EmbedderTestMultiBackend.CanRenderGradientWithCompositorOnNonRootLayer sufficient for covering the code in this PR?
If the test doesn't repro the problem, I would just remove it. Otherwise, tossing it into embedder_gl_unittests and writing it using EmbedderTestMultiBackend to cover both GL and Vulkan seems like a good approach for the moment.
Later on we should add software context support to the multi-backend class, but that's perhaps a job for another day.
Basically, anything that's Vulkan-specific gets changed to a switchboard method:
auto& context = GetEmbedderContext(EmbedderTestContextType::kVulkanContext)becomesauto& context = GetEmbedderContext(GetParam());builder.SetRenderTargetType(EmbedderTestBackingStoreProducer::RenderTargetType::kVulkanImage);becomesbuilder.SetRenderTargetType(GetRenderTargetFromBackend(GetParam(), false));- etc
gaaclarke
force-pushed
the
threadsafe-skia-vkqueuesubmit
branch
from
c145143 to
4ef4e5a
Compare
gaaclarke
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/engine/45459, due to - The status or check suite triage has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
I'll try to rebase this to see if that flushes out the failure. |
gaaclarke
force-pushed
the
threadsafe-skia-vkqueuesubmit
branch
from
4ef4e5a to
67f1bd8
Compare
gaaclarke
added
the
autosubmit
…134328) flutter/engine@3a5f3ad...1f2da3d 2023-09-09 [email protected] Makes Skia's vkQueueSubmit threadsafe. (flutter/engine#45459) 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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
I'm going to revert this. It works fine in tests, but it's not going to work for real usage of the embedder since the user has free access to the vkQueue and thus has no way to ensure that the vkQueue isn't used concurrently. |
This reverts commit 1f2da3d.
Reverts #45459 I'm going to revert this. It works fine in tests, but it's not going to work for real usage of the embedder since the user has free access to the vkQueue and thus has no way to ensure that the vkQueue isn't used concurrently. See: flutter/flutter#133933 (comment)
2 participants
fixes flutter/flutter#133933
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.