From 81bf8d5e3b754b54d3a865380204645468e63eb7 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 25 May 2023 19:39:44 +0200 Subject: [PATCH] AtlasEngine: Fix Present() of out of bounds glyphs (#15403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `til::rect`'s truthiness check (= rect is valid) returns `false` for any rects that have negative coordinates. This makes sense for buffer handling, but breaks AtlasEngine, where glyph coordinates can go out of bounds and it's entirely valid for that to happen. Closes #15416 ## Validation Steps Performed * Use MesloLGM NF and print NF glyphs in the first row * Text rendering, selection, etc., still works ✅ --------- Co-authored-by: Dustin L. Howett (cherry picked from commit a9f34e309578b259a3e9ac452107a5518f686490) Service-Card-Id: 89326794 Service-Version: 1.18 --- src/renderer/atlas/AtlasEngine.r.cpp | 29 ++++++++++++++-------------- src/renderer/atlas/BackendD2D.cpp | 3 ++- src/renderer/atlas/BackendD2D.h | 2 +- src/renderer/atlas/BackendD3D.cpp | 5 +++-- src/renderer/atlas/BackendD3D.h | 2 +- src/renderer/atlas/common.h | 8 ++++++-- 6 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/renderer/atlas/AtlasEngine.r.cpp b/src/renderer/atlas/AtlasEngine.r.cpp index 1e848884955..fb85d8e3e0b 100644 --- a/src/renderer/atlas/AtlasEngine.r.cpp +++ b/src/renderer/atlas/AtlasEngine.r.cpp @@ -415,31 +415,32 @@ void AtlasEngine::_waitUntilCanRender() noexcept void AtlasEngine::_present() { - // Present1() dislikes being called with an empty dirty rect. - if (!_p.dirtyRectInPx) - { - return; - } - - const til::rect fullRect{ 0, 0, _p.swapChain.targetSize.x, _p.swapChain.targetSize.y }; + const RECT fullRect{ 0, 0, _p.swapChain.targetSize.x, _p.swapChain.targetSize.y }; DXGI_PRESENT_PARAMETERS params{}; RECT scrollRect{}; POINT scrollOffset{}; // Since rows might be taller than their cells, they might have drawn outside of the viewport. - auto dirtyRect = _p.dirtyRectInPx; - dirtyRect.left = std::max(dirtyRect.left, 0); - dirtyRect.top = std::max(dirtyRect.top, 0); - dirtyRect.right = std::min(dirtyRect.right, fullRect.right); - dirtyRect.bottom = std::min(dirtyRect.bottom, fullRect.bottom); + RECT dirtyRect{ + .left = std::max(_p.dirtyRectInPx.left, 0), + .top = std::max(_p.dirtyRectInPx.top, 0), + .right = std::min(_p.dirtyRectInPx.right, fullRect.right), + .bottom = std::min(_p.dirtyRectInPx.bottom, fullRect.bottom), + }; + + // Present1() dislikes being called with an empty dirty rect. + if (dirtyRect.left >= dirtyRect.right || dirtyRect.top >= dirtyRect.bottom) + { + return; + } if constexpr (!ATLAS_DEBUG_SHOW_DIRTY) { - if (dirtyRect != fullRect) + if (memcmp(&dirtyRect, &fullRect, sizeof(RECT)) != 0) { params.DirtyRectsCount = 1; - params.pDirtyRects = dirtyRect.as_win32_rect(); + params.pDirtyRects = &dirtyRect; if (_p.scrollOffset) { diff --git a/src/renderer/atlas/BackendD2D.cpp b/src/renderer/atlas/BackendD2D.cpp index 9f487d3dc62..1d0db35803b 100644 --- a/src/renderer/atlas/BackendD2D.cpp +++ b/src/renderer/atlas/BackendD2D.cpp @@ -628,7 +628,8 @@ void BackendD2D::_debugShowDirty(const RenderingPayload& p) for (size_t i = 0; i < std::size(_presentRects); ++i) { - if (const auto& rect = _presentRects[i]) + const auto& rect = _presentRects[(_presentRectsPos + i) % std::size(_presentRects)]; + if (rect.non_empty()) { const D2D1_RECT_F rectF{ static_cast(rect.left), diff --git a/src/renderer/atlas/BackendD2D.h b/src/renderer/atlas/BackendD2D.h index a68dcd52619..1f70c14c75e 100644 --- a/src/renderer/atlas/BackendD2D.h +++ b/src/renderer/atlas/BackendD2D.h @@ -56,7 +56,7 @@ namespace Microsoft::Console::Render::Atlas u16x2 _cellCount{}; #if ATLAS_DEBUG_SHOW_DIRTY - til::rect _presentRects[9]{}; + i32r _presentRects[9]{}; size_t _presentRectsPos = 0; #endif diff --git a/src/renderer/atlas/BackendD3D.cpp b/src/renderer/atlas/BackendD3D.cpp index 6415160454d..d1990e309c7 100644 --- a/src/renderer/atlas/BackendD3D.cpp +++ b/src/renderer/atlas/BackendD3D.cpp @@ -2047,10 +2047,11 @@ void BackendD3D::_debugShowDirty(const RenderingPayload& p) for (size_t i = 0; i < std::size(_presentRects); ++i) { - if (const auto& rect = _presentRects[i]) + const auto& rect = _presentRects[(_presentRectsPos + i) % std::size(_presentRects)]; + if (rect.non_empty()) { _appendQuad() = { - .shadingType = ShadingType::SolidFill, + .shadingType = ShadingType::Selection, .position = { static_cast(rect.left), static_cast(rect.top), diff --git a/src/renderer/atlas/BackendD3D.h b/src/renderer/atlas/BackendD3D.h index 020ecd575cc..d9f9a13c172 100644 --- a/src/renderer/atlas/BackendD3D.h +++ b/src/renderer/atlas/BackendD3D.h @@ -293,7 +293,7 @@ namespace Microsoft::Console::Render::Atlas bool _requiresContinuousRedraw = false; #if ATLAS_DEBUG_SHOW_DIRTY - til::rect _presentRects[9]{}; + i32r _presentRects[9]{}; size_t _presentRectsPos = 0; #endif diff --git a/src/renderer/atlas/common.h b/src/renderer/atlas/common.h index 9f53c398b5f..c0b69b808ec 100644 --- a/src/renderer/atlas/common.h +++ b/src/renderer/atlas/common.h @@ -531,8 +531,12 @@ namespace Microsoft::Console::Render::Atlas std::array colorBitmapGenerations{ 1, 1 }; // In columns/rows. til::rect cursorRect; - // In pixel. - til::rect dirtyRectInPx; + // The viewport/SwapChain area to be presented. In pixel. + // NOTE: + // This cannot use til::rect, because til::rect generally expects positive coordinates only + // (`operator!()` checks for negative values), whereas this one can go out of bounds, + // whenever glyphs go out of bounds. `AtlasEngine::_present()` will clamp it. + i32r dirtyRectInPx{}; // In rows. range invalidatedRows{}; // In pixel.