[Impeller] Replace Vulkan rotation checks with polling (#44361)
tl;dr: Reverts https://github.com/flutter/engine/pull/43214. closes
https://github.com/flutter/flutter/issues/129459.
---
In https://github.com/flutter/engine/pull/43214, @jonahwilliams used
`vkQueuePresentKHR`[^1] [to
check](https://github.com/jonahwilliams/engine/blob/a0df46add166202fe68a853391d379e69ddc7c18/impeller/renderer/backend/vulkan/swapchain_impl_vk.cc#L465-L467)
if device orientations had changed, and if so, the swap-chain was torn
down and replaced with a new instance (that would now have a correct
rotation for ... compositing reasons I don't quite understand).
Unfortunately `vkQueuePresentKHR` is (a) only present on Android 10+
_and_ (b) ... isn't implemented consistently across Android devices. For
example, the popular Samsung Galaxy S10 doesn't return
`VK_SUBOPTIMAL_KHR`, leading to the same rotation bugs described in
https://github.com/flutter/flutter/issues/129459 (i.e. even with
https://github.com/flutter/engine/pull/43214).
This PR implements the polling technique recommended by Android[^2], in
this case on every frame. We should expect this to have a performance
penalty of ~`0.2`ms per frame, but should at least give us consistent
fidelity on Vulkan + Android for the time-being.
<details>
<summary>
Some additional screenshots generated while debugging
</summary>
<img src
="https://github.com/flutter/engine/assets/168174/900e02a8-aa51-4592-9690-c650092130a2"
/>
<img
src="https://github.com/flutter/engine/assets/168174/e7b9e5c0-86b0-407f-aa51-2f76afda4f03)"
/>
<img
src="https://github.com/flutter/engine/assets/168174/6b611090-97f5-4589-9ef9-6ba778efc6b7"
/>
</details>
[^1]:
https://developer.android.com/games/optimize/vulkan-prerotation#detect_device_orientation_changes
[^2]:
https://developer.android.com/games/optimize/vulkan-prerotation#using_polling
diff --git a/impeller/renderer/backend/vulkan/swapchain_impl_vk.cc b/impeller/renderer/backend/vulkan/swapchain_impl_vk.cc
index fbfaac0..c505859 100644
--- a/impeller/renderer/backend/vulkan/swapchain_impl_vk.cc
+++ b/impeller/renderer/backend/vulkan/swapchain_impl_vk.cc
@@ -10,11 +10,17 @@
#include "impeller/renderer/backend/vulkan/formats_vk.h"
#include "impeller/renderer/backend/vulkan/surface_vk.h"
#include "impeller/renderer/backend/vulkan/swapchain_image_vk.h"
+#include "vulkan/vulkan_structs.hpp"
namespace impeller {
static constexpr size_t kMaxFramesInFlight = 3u;
+// Number of frames to poll for orientation changes. For example `1u` means
+// that the orientation will be polled every frame, while `2u` means that the
+// orientation will be polled every other frame.
+static constexpr size_t kPollFramesForOrientation = 1u;
+
struct FrameSynchronizer {
vk::UniqueFence acquire;
vk::UniqueSemaphore render_ready;
@@ -124,16 +130,17 @@
std::shared_ptr<SwapchainImplVK> SwapchainImplVK::Create(
const std::shared_ptr<Context>& context,
vk::UniqueSurfaceKHR surface,
- bool was_rotated,
- vk::SwapchainKHR old_swapchain) {
+ vk::SwapchainKHR old_swapchain,
+ vk::SurfaceTransformFlagBitsKHR last_transform) {
return std::shared_ptr<SwapchainImplVK>(new SwapchainImplVK(
- context, std::move(surface), was_rotated, old_swapchain));
+ context, std::move(surface), old_swapchain, last_transform));
}
-SwapchainImplVK::SwapchainImplVK(const std::shared_ptr<Context>& context,
- vk::UniqueSurfaceKHR surface,
- bool was_rotated,
- vk::SwapchainKHR old_swapchain) {
+SwapchainImplVK::SwapchainImplVK(
+ const std::shared_ptr<Context>& context,
+ vk::UniqueSurfaceKHR surface,
+ vk::SwapchainKHR old_swapchain,
+ vk::SurfaceTransformFlagBitsKHR last_transform) {
if (!context) {
return;
}
@@ -278,8 +285,7 @@
synchronizers_ = std::move(synchronizers);
current_frame_ = synchronizers_.size() - 1u;
is_valid_ = true;
- was_rotated_ = was_rotated;
- is_rotated_ = was_rotated;
+ transform_if_changed_discard_swapchain_ = last_transform;
}
SwapchainImplVK::~SwapchainImplVK() {
@@ -311,6 +317,10 @@
return surface_format_;
}
+vk::SurfaceTransformFlagBitsKHR SwapchainImplVK::GetLastTransform() const {
+ return transform_if_changed_discard_swapchain_;
+}
+
std::shared_ptr<Context> SwapchainImplVK::GetContext() const {
return context_.lock();
}
@@ -321,10 +331,6 @@
return {};
}
- if (was_rotated_ != is_rotated_) {
- return AcquireResult{true /* out of date */};
- }
-
const auto& context = ContextVK::Cast(*context_strong);
current_frame_ = (current_frame_ + 1u) % synchronizers_.size();
@@ -340,6 +346,26 @@
}
//----------------------------------------------------------------------------
+ /// Poll to see if the orientation has changed.
+ ///
+ /// https://developer.android.com/games/optimize/vulkan-prerotation#using_polling
+ current_transform_poll_count_++;
+ if (current_transform_poll_count_ >= kPollFramesForOrientation) {
+ current_transform_poll_count_ = 0u;
+ auto [caps_result, caps] =
+ context.GetPhysicalDevice().getSurfaceCapabilitiesKHR(*surface_);
+ if (caps_result != vk::Result::eSuccess) {
+ VALIDATION_LOG << "Could not get surface capabilities: "
+ << vk::to_string(caps_result);
+ return {};
+ }
+ if (caps.currentTransform != transform_if_changed_discard_swapchain_) {
+ transform_if_changed_discard_swapchain_ = caps.currentTransform;
+ return AcquireResult{true /* out of date */};
+ }
+ }
+
+ //----------------------------------------------------------------------------
/// Get the next image index.
///
auto [acq_result, index] = context.GetDevice().acquireNextImageKHR(
@@ -349,11 +375,6 @@
nullptr // fence
);
- if (acq_result == vk::Result::eSuboptimalKHR) {
- is_rotated_ = true;
- return AcquireResult{true /* out of date */};
- }
-
if (acq_result == vk::Result::eErrorOutOfDateKHR) {
return AcquireResult{true /* out of date */};
}
@@ -470,11 +491,13 @@
// Vulkan guarantees that the set of queue operations will still
// complete successfully.
[[fallthrough]];
- case vk::Result::eSuccess:
- is_rotated_ = false;
- return;
case vk::Result::eSuboptimalKHR:
- is_rotated_ = true;
+ // Even though we're handling rotation changes via polling, we
+ // still need to handle the case where the swapchain signals that
+ // it's suboptimal (i.e. every frame when we are rotated given we
+ // aren't doing Vulkan pre-rotation).
+ [[fallthrough]];
+ case vk::Result::eSuccess:
return;
default:
VALIDATION_LOG << "Could not present queue: "
diff --git a/impeller/renderer/backend/vulkan/swapchain_impl_vk.h b/impeller/renderer/backend/vulkan/swapchain_impl_vk.h
index 7f06197..cacc6b2 100644
--- a/impeller/renderer/backend/vulkan/swapchain_impl_vk.h
+++ b/impeller/renderer/backend/vulkan/swapchain_impl_vk.h
@@ -9,6 +9,7 @@
#include "flutter/fml/macros.h"
#include "impeller/renderer/backend/vulkan/vk.h"
+#include "vulkan/vulkan_enums.hpp"
namespace impeller {
@@ -31,8 +32,9 @@
static std::shared_ptr<SwapchainImplVK> Create(
const std::shared_ptr<Context>& context,
vk::UniqueSurfaceKHR surface,
- bool was_rotated,
- vk::SwapchainKHR old_swapchain = VK_NULL_HANDLE);
+ vk::SwapchainKHR old_swapchain = VK_NULL_HANDLE,
+ vk::SurfaceTransformFlagBitsKHR last_transform =
+ vk::SurfaceTransformFlagBitsKHR::eIdentity);
~SwapchainImplVK();
@@ -48,12 +50,12 @@
: surface(std::move(p_surface)) {}
};
- bool GetIsRotated() const { return is_rotated_; }
-
AcquireResult AcquireNextDrawable();
vk::Format GetSurfaceFormat() const;
+ vk::SurfaceTransformFlagBitsKHR GetLastTransform() const;
+
std::shared_ptr<Context> GetContext() const;
std::pair<vk::UniqueSurfaceKHR, vk::UniqueSwapchainKHR> DestroySwapchain();
@@ -68,14 +70,13 @@
std::vector<std::unique_ptr<FrameSynchronizer>> synchronizers_;
size_t current_frame_ = 0u;
bool is_valid_ = false;
-
- bool was_rotated_ = false;
- bool is_rotated_ = false;
+ size_t current_transform_poll_count_ = 0u;
+ vk::SurfaceTransformFlagBitsKHR transform_if_changed_discard_swapchain_;
SwapchainImplVK(const std::shared_ptr<Context>& context,
vk::UniqueSurfaceKHR surface,
- bool was_rotated,
- vk::SwapchainKHR old_swapchain);
+ vk::SwapchainKHR old_swapchain,
+ vk::SurfaceTransformFlagBitsKHR last_transform);
bool Present(const std::shared_ptr<SwapchainImageVK>& image, uint32_t index);
diff --git a/impeller/renderer/backend/vulkan/swapchain_vk.cc b/impeller/renderer/backend/vulkan/swapchain_vk.cc
index ee958a4..0a09a9e 100644
--- a/impeller/renderer/backend/vulkan/swapchain_vk.cc
+++ b/impeller/renderer/backend/vulkan/swapchain_vk.cc
@@ -12,8 +12,7 @@
std::shared_ptr<SwapchainVK> SwapchainVK::Create(
const std::shared_ptr<Context>& context,
vk::UniqueSurfaceKHR surface) {
- auto impl = SwapchainImplVK::Create(context, std::move(surface),
- /*was_rotated=*/false);
+ auto impl = SwapchainImplVK::Create(context, std::move(surface));
if (!impl || !impl->IsValid()) {
return nullptr;
}
@@ -46,13 +45,12 @@
// This swapchain implementation indicates that it is out of date. Tear it
// down and make a new one.
auto context = impl_->GetContext();
- auto was_rotated = impl_->GetIsRotated();
auto [surface, old_swapchain] = impl_->DestroySwapchain();
- auto new_impl = SwapchainImplVK::Create(context, //
- std::move(surface), //
- was_rotated, //
- *old_swapchain //
+ auto new_impl = SwapchainImplVK::Create(context, //
+ std::move(surface), //
+ *old_swapchain, //
+ impl_->GetLastTransform() //
);
if (!new_impl || !new_impl->IsValid()) {
VALIDATION_LOG << "Could not update swapchain.";
