From 31fa9475dac35bf2eaea1804bed74c9fbcd0ea91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Sto=C5=A1i=C4=87?= Date: Mon, 29 Mar 2021 13:39:16 +0200 Subject: [PATCH] [FancyZones] Fix deadlocks in ZoneWindowDrawing (#10461) * Fixed deadlocks in ZoneWindowDrawing Moved all possibly reentrant or blocking calls to ShowWindow out of critical sections. * Initialize bools * Tune flashing visuals * Address PR comments * Use = true; to initialize bools * Remove tracing from GetAnimationAlpha * Use member initialization when constructing AnimationInfo * Refactor rendering * Whitespace * Hide window on render failure --- src/modules/fancyzones/lib/FancyZones.cpp | 4 +- src/modules/fancyzones/lib/ZoneWindow.cpp | 6 +- .../fancyzones/lib/ZoneWindowDrawing.cpp | 121 ++++++++++-------- .../fancyzones/lib/ZoneWindowDrawing.h | 16 ++- 4 files changed, 80 insertions(+), 67 deletions(-) diff --git a/src/modules/fancyzones/lib/FancyZones.cpp b/src/modules/fancyzones/lib/FancyZones.cpp index 8d8880805e..6665b28322 100644 --- a/src/modules/fancyzones/lib/FancyZones.cpp +++ b/src/modules/fancyzones/lib/FancyZones.cpp @@ -647,7 +647,7 @@ void FancyZones::ToggleEditor() noexcept wil::unique_cotaskmem_string virtualDesktopId; if (!SUCCEEDED(StringFromCLSID(m_currentDesktopId, &virtualDesktopId))) { - return; + return; } /* @@ -677,7 +677,7 @@ void FancyZones::ToggleEditor() noexcept { params += FancyZonesUtils::GenerateUniqueIdAllMonitorsArea(virtualDesktopId.get()) + divider; /* Monitor id where the Editor should be opened */ } - + // device id map std::unordered_map displayDeviceIdxMap; diff --git a/src/modules/fancyzones/lib/ZoneWindow.cpp b/src/modules/fancyzones/lib/ZoneWindow.cpp index aed00f01a5..4754430dac 100644 --- a/src/modules/fancyzones/lib/ZoneWindow.cpp +++ b/src/modules/fancyzones/lib/ZoneWindow.cpp @@ -163,8 +163,6 @@ private: std::vector m_highlightZone; WPARAM m_keyLast{}; size_t m_keyCycle{}; - static const UINT m_showAnimationDuration = 200; // ms - static const UINT m_flashDuration = 1000; // ms std::unique_ptr m_zoneWindowDrawing; }; @@ -384,7 +382,7 @@ ZoneWindow::ShowZoneWindow() noexcept { SetAsTopmostWindow(); m_zoneWindowDrawing->DrawActiveZoneSet(m_activeZoneSet->GetZones(), m_highlightZone, m_host); - m_zoneWindowDrawing->Show(m_showAnimationDuration); + m_zoneWindowDrawing->Show(); } } @@ -428,7 +426,7 @@ ZoneWindow::FlashZones() noexcept { SetAsTopmostWindow(); m_zoneWindowDrawing->DrawActiveZoneSet(m_activeZoneSet->GetZones(), {}, m_host); - m_zoneWindowDrawing->Flash(m_flashDuration); + m_zoneWindowDrawing->Flash(); } } diff --git a/src/modules/fancyzones/lib/ZoneWindowDrawing.cpp b/src/modules/fancyzones/lib/ZoneWindowDrawing.cpp index b47b62090f..0731f184c2 100644 --- a/src/modules/fancyzones/lib/ZoneWindowDrawing.cpp +++ b/src/modules/fancyzones/lib/ZoneWindowDrawing.cpp @@ -9,6 +9,12 @@ #include +namespace +{ + const int FadeInDurationMillis = 200; + const int FlashZonesDurationMillis = 700; +} + namespace NonLocalizable { const wchar_t SegoeUiFont[] = L"Segoe ui"; @@ -16,25 +22,23 @@ namespace NonLocalizable float ZoneWindowDrawing::GetAnimationAlpha() { - // Lock is being held + // Lock is held by the caller + if (!m_animation) { return 0.f; } auto tNow = std::chrono::steady_clock().now(); - auto alpha = (tNow - m_animation->tStart).count() / (1e6f * m_animation->duration); + auto millis = (tNow - m_animation->tStart).count() / 1e6f; - if (m_animation->fadeIn) + if (m_animation->autoHide && millis > FlashZonesDurationMillis) { - // Return a positive value to avoid hiding - return std::clamp(alpha, 0.001f, 1.f); - } - else - { - // Quadratic function looks better - return std::clamp(1.f - alpha * alpha, 0.f, 1.f); + return 0.f; } + + // Return a positive value to avoid hiding + return std::clamp(millis / FadeInDurationMillis, 0.001f, 1.f); } ID2D1Factory* ZoneWindowDrawing::GetD2DFactory() @@ -91,7 +95,7 @@ ZoneWindowDrawing::ZoneWindowDrawing(HWND window) D2D1::PixelFormat(DXGI_FORMAT_UNKNOWN, D2D1_ALPHA_MODE_PREMULTIPLIED), 96.f, 96.f); - + auto renderTargetSize = D2D1::SizeU(m_clientRect.right - m_clientRect.left, m_clientRect.bottom - m_clientRect.top); auto hwndRenderTargetProperties = D2D1::HwndRenderTargetProperties(window, renderTargetSize); @@ -106,20 +110,20 @@ ZoneWindowDrawing::ZoneWindowDrawing(HWND window) m_renderThread = std::thread([this]() { RenderLoop(); }); } -void ZoneWindowDrawing::Render() +ZoneWindowDrawing::RenderResult ZoneWindowDrawing::Render() { std::unique_lock lock(m_mutex); if (!m_renderTarget) { - return; + return RenderResult::Failed; } float animationAlpha = GetAnimationAlpha(); if (animationAlpha <= 0.f) { - return; + return RenderResult::AnimationEnded; } m_renderTarget->BeginDraw(); @@ -186,91 +190,96 @@ void ZoneWindowDrawing::Render() lock.unlock(); m_renderTarget->EndDraw(); + return RenderResult::Ok; } void ZoneWindowDrawing::RenderLoop() { while (!m_abortThread) { - float animationAlpha; - { - // The lock must be held by the caller when calling GetAnimationAlpha - std::unique_lock lock(m_mutex); - animationAlpha = GetAnimationAlpha(); - } - - // Check whether the animation expired and we need to hide the window - if (animationAlpha <= 0.f) - { - Hide(); - } - { // Wait here while rendering is disabled std::unique_lock lock(m_mutex); m_cv.wait(lock, [this]() { return (bool)m_shouldRender; }); } - - Render(); + + auto result = Render(); + + if (result == RenderResult::AnimationEnded || result == RenderResult::Failed) + { + Hide(); + } } } void ZoneWindowDrawing::Hide() { _TRACER_; - std::unique_lock lock(m_mutex); - - m_animation.reset(); - - if (m_shouldRender) + bool shouldHideWindow = true; { + std::unique_lock lock(m_mutex); + m_animation.reset(); + shouldHideWindow = m_shouldRender; m_shouldRender = false; + } + + if (shouldHideWindow) + { ShowWindow(m_window, SW_HIDE); } } -void ZoneWindowDrawing::Show(unsigned animationMillis) +void ZoneWindowDrawing::Show() { _TRACER_; - std::unique_lock lock(m_mutex); - - if (!m_shouldRender) + bool shouldShowWindow = true; + { + std::unique_lock lock(m_mutex); + shouldShowWindow = !m_shouldRender; + m_shouldRender = true; + + if (!m_animation) + { + m_animation.emplace(AnimationInfo{ .tStart = std::chrono::steady_clock().now(), .autoHide = false }); + } + else if (m_animation->autoHide) + { + // Do not change the starting time of the animation, just reset autoHide + m_animation->autoHide = false; + } + } + + if (shouldShowWindow) { ShowWindow(m_window, SW_SHOWNA); } - animationMillis = max(animationMillis, 1); - - if (!m_animation || !m_animation->fadeIn) - { - m_animation.emplace(AnimationInfo{ std::chrono::steady_clock().now(), animationMillis, true }); - } - - m_shouldRender = true; m_cv.notify_all(); } -void ZoneWindowDrawing::Flash(unsigned animationMillis) +void ZoneWindowDrawing::Flash() { _TRACER_; - std::unique_lock lock(m_mutex); + bool shouldShowWindow = true; + { + std::unique_lock lock(m_mutex); + shouldShowWindow = !m_shouldRender; + m_shouldRender = true; - if (!m_shouldRender) + m_animation.emplace(AnimationInfo{ .tStart = std::chrono::steady_clock().now(), .autoHide = true }); + } + + if (shouldShowWindow) { ShowWindow(m_window, SW_SHOWNA); } - animationMillis = max(animationMillis, 1); - - m_animation.emplace(AnimationInfo{ std::chrono::steady_clock().now(), animationMillis, false }); - - m_shouldRender = true; m_cv.notify_all(); } void ZoneWindowDrawing::DrawActiveZoneSet(const IZoneSet::ZonesMap& zones, - const std::vector& highlightZones, - winrt::com_ptr host) + const std::vector& highlightZones, + winrt::com_ptr host) { _TRACER_; std::unique_lock lock(m_mutex); diff --git a/src/modules/fancyzones/lib/ZoneWindowDrawing.h b/src/modules/fancyzones/lib/ZoneWindowDrawing.h index b9cceb16b1..06a204ea14 100644 --- a/src/modules/fancyzones/lib/ZoneWindowDrawing.h +++ b/src/modules/fancyzones/lib/ZoneWindowDrawing.h @@ -25,8 +25,14 @@ class ZoneWindowDrawing struct AnimationInfo { std::chrono::steady_clock::time_point tStart; - unsigned duration; - bool fadeIn; + bool autoHide; + }; + + enum struct RenderResult + { + Ok, + AnimationEnded, + Failed, }; HWND m_window = nullptr; @@ -42,7 +48,7 @@ class ZoneWindowDrawing static IDWriteFactory* GetWriteFactory(); static D2D1_COLOR_F ConvertColor(COLORREF color); static D2D1_RECT_F ConvertRect(RECT rect); - void Render(); + RenderResult Render(); void RenderLoop(); std::atomic m_shouldRender = false; @@ -55,8 +61,8 @@ public: ~ZoneWindowDrawing(); ZoneWindowDrawing(HWND window); void Hide(); - void Show(unsigned animationMillis); - void Flash(unsigned animationMillis); + void Show(); + void Flash(); void DrawActiveZoneSet(const IZoneSet::ZonesMap& zones, const std::vector& highlightZones, winrt::com_ptr host);