From a9f34e309578b259a3e9ac452107a5518f686490 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 --- 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 1e84888495..fb85d8e3e0 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 f9e1427511..50e6e90d9b 100644 --- a/src/renderer/atlas/BackendD2D.cpp +++ b/src/renderer/atlas/BackendD2D.cpp @@ -627,7 +627,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 a68dcd5261..1f70c14c75 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 6e26a2cbda..9063573fcf 100644 --- a/src/renderer/atlas/BackendD3D.cpp +++ b/src/renderer/atlas/BackendD3D.cpp @@ -2042,10 +2042,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 020ecd575c..d9f9a13c17 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 9f53c398b5..c0b69b808e 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.