Repository: microsoft/terminal (language: cpp)

Issue:
Wrong terminal content redrawing after some time of inactivity... 
### Windows Terminal version

Bulded from source... Last commit fd74d59c6c87a1ac49ffea11142ce273e07f95f0

### Windows build number

10.0.25151.0

### Other Software

PowerShell
Far Manager

### Steps to reproduce

After some time of inactivity we lost content redrawing... some parts still visible, links still exists and works as expected (click works and url under link is still right, underlined on hoover), background also seems ok for example... we can copy content of terminal and copy is fine. All tabs in same (wrong) state after some time... new ones is ok.

clear in powershell for example make some differences... but not solve problem: 3rd attached screenshot

On attached screenshots first tab with PowerShell after some time of inactivity and second tab in same terminal - new - with PowerShell in same scroll position.

### Expected Behavior

![image](https://user-images.githubusercontent.com/1778999/178478620-5e4475e1-cfc8-4755-900c-0eafa465cd52.png)


### Actual Behavior

![image](https://user-images.githubusercontent.com/1778999/178478532-9fc017aa-9e68-4e68-8c8f-53500b3d4357.png)
![image](https://user-images.githubusercontent.com/1778999/178481548-1628040d-213f-40a9-aeaa-2600d7ceb847.png)



Discussion / hints:
Huh. Are you using the experimental atlas engine?

/cc @lhecker 
> Huh. Are you using the experimental atlas engine?
> 
> /cc @lhecker

Yes - Atlas
Welp, there's a couple fresh commits here that might have caused this. 

If you'd be willing to help with debugging - does this repro on either of:
* 32379c29f025ffdede0c56884ec5a30871c2a8c2
* bbc570d107b6b67b68454355e04724ecea9c9796

Those are the commits before a couple PRs in this area recently.

(thanks for selfhosting the bleeding edge here ☺️)
> Welp, there's a couple fresh commits here that might have caused this.
> 
> If you'd be willing to help with debugging - does this repro on either of:
> 
> * [32379c2](https://github.com/microsoft/terminal/commit/32379c29f025ffdede0c56884ec5a30871c2a8c2)
> * [bbc570d](https://github.com/microsoft/terminal/commit/bbc570d107b6b67b68454355e04724ecea9c9796)
> 
> Those are the commits before a couple PRs in this area recently.
> 
> (thanks for selfhosting the bleeding edge here ☺️)

Will check...
Coincidentally I noticed the same just yesterday. I find it easiest to reproduce by changing the font size in OpenConsole.

FYI you can run OpenConsole with AtlasEngine by setting `HKEY_CURRENT_USER\Console\UseDx` to `2` (`DWORD`), setting `Host.EXE` as the startup project and launching it.
So this issue consists of two bugs:
* Rows outside of the `_api.dirtyRect` (i.e. rows that haven't changed) need to be pushed to the head of the LRU queue so that they don't get invalidated. For instance if you use `tmux` and only scroll the bottom half, the glyphs of the top half need to be retained in the texture atlas of course. I actually planned for that, but forgot to add it in my PR so this was easy to fix. I'm sure this is what causes most of the above issues.
* Hard problem: https://github.com/microsoft/terminal/commit/d74b66a0a7955de529f02fd394d6f6536a96e88c has a bug where just opening OpenConsole's Properties dialog and clicking Ok without changing anything causes the texture atlas to go all black. I checked it in QRenderDoc and the texture is absolutely blank (no alpha, no nothing).
  * Puzzling part 1: Reverting the changes to `AtlasEngine::_drawGlyph` doesn't work. No text is produced.
  * Puzzling part 2: The `ID2D1RenderTarget` must work in principle however, because it _does_ produce the cursor texture correctly. Only the text is gone. It can't be the `_r.brush` either because I'm recreating it whenever I recreate the `ID2D1RenderTarget`.
  * Puzzling part 3: It works when the size of the texture grows naturally (i.e. from 256x256 to 512x256). Hence, the bug must be related to or caused by `AtlasEngine::_recreateFontDependentResources()`.
> So this issue consists of two bugs:
> 
> * Rows outside of the `_api.dirtyRect` (i.e. rows that haven't changed) need to be pushed to the head of the LRU queue so that they don't get invalidated. For instance if you use `tmux` and only scroll the bottom half, the glyphs of the top half need to be retained in the texture atlas of course. I actually planned for that, but forgot to add it in my PR so this was easy to fix. I'm sure this is what causes most of the above issues.
> * Hard problem: [d74b66a](https://github.com/microsoft/terminal/commit/d74b66a0a7955de529f02fd394d6f6536a96e88c) has a bug where just opening OpenConsole's Properties dialog and clicking Ok without changing anything causes the texture atlas to go all black. I checked it in QRenderDoc and the texture is absolutely blank (no alpha, no nothing).
>   
>   * Puzzling part 1: Reverting the changes to `AtlasEngine::_drawGlyph` doesn't work. No text is produced.
>   * Puzzling part 2: The `ID2D1RenderTarget` must work in principle however, because it _does_ produce the cursor texture correctly. Only the text is gone. It can't be the `_r.brush` either because I'm recreating it whenever I recreate the `ID2D1RenderTarget`.

And... not all text is gone... some glyphs still visible some not... if it matters

For example...


```
C:\>dir
 Volume in drive C has no label.
 Volume Serial Number is 0A20-79B9

 Directory of C:\

18.01.2019  14:36             1 024 .rnd
16.06.2022  12:53            12 288 DumpStack.log
21.03.2022  13:52    <DIR>          Intel
12.07.2022  15:35    <DIR>          opt
26.06.2022  15:25    <DIR>          PerfLogs
06.07.2022  16:38    <DIR>          Program Files
01.07.2022  10:37    <DIR>          Program Files (x86)
30.06.2022  23:41    <DIR>          Users
06.07.2022  03:41    <DIR>          Windows
               2 File(s)         13 312 bytes
               7 Dir(s)  15 891 099 648 bytes free

```
![image](https://user-images.githubusercontent.com/1778999/178523372-e61d7bdc-2e0d-41e0-895a-5a56fef84a68.png)



Fix patch:
diff --git a/src/renderer/atlas/AtlasEngine.cpp b/src/renderer/atlas/AtlasEngine.cpp
index 815537bc483..a5bd51b7eab 100644
--- a/src/renderer/atlas/AtlasEngine.cpp
+++ b/src/renderer/atlas/AtlasEngine.cpp
@@ -360,6 +360,40 @@ try
         _api.dirtyRect = til::rect{ 0, _api.invalidatedRows.x, _api.cellCount.x, _api.invalidatedRows.y };
     }
 
+    // This is an important block of code for our TileHashMap.
+    // We only process glyphs within the dirtyRect, but glyphs outside of the
+    // dirtyRect are still in use and shouldn't be discarded. This is critical
+    // if someone uses a tool like tmux to split the terminal horizontally.
+    // If they then print a lot of Unicode text on just one side, we have to
+    // ensure that the (for example) plain ASCII glyphs on the other half of the
+    // viewport are still retained. This bit of code "refreshes" those glyphs and
+    // brings them to the front of the LRU queue to prevent them from being reused.
+    {
+        const std::array<til::point, 2> ranges{ {
+            { 0, _api.dirtyRect.top },
+            { _api.dirtyRect.bottom, _api.cellCount.y },
+        } };
+        const auto stride = static_cast<size_t>(_r.cellCount.x);
+
+        for (const auto& p : ranges)
+        {
+            // We (ab)use the .x/.y members of the til::point as the
+            // respective [from,to) range of rows we need to makeNewest().
+            const auto from = p.x;
+            const auto to = p.y;
+
+            for (auto y = from; y < to; ++y)
+            {
+                auto it = _r.cellGlyphMapping.data() + stride * y;
+                const auto end = it + stride;
+                for (; it != end; ++it)
+                {
+                    _r.glyphs.makeNewest(*it);
+                }
+            }
+        }
+    }
+
     return S_OK;
 }
 catch (const wil::ResultException& exception)
@@ -883,10 +917,12 @@ void AtlasEngine::_recreateSizeDependentResources()
         // This buffer is a bit larger than the others (multiple MB).
         // Prevent a memory usage spike, by first deallocating and then allocating.
         _r.cells = {};
+        _r.cellGlyphMapping = {};
         // Our render loop heavily relies on memcpy() which is between 1.5x
         // and 40x faster for allocations with an alignment of 32 or greater.
         // (40x on AMD Zen1-3, which have a rep movsb performance issue. MSFT:33358259.)
         _r.cells = Buffer<Cell, 32>{ totalCellCount };
+        _r.cellGlyphMapping = Buffer<TileHashMap::iterator>{ totalCellCount };
         _r.cellCount = _api.cellCount;
         _r.tileAllocator.setMaxArea(_api.sizeInPixel);
 
@@ -1062,6 +1098,13 @@ AtlasEngine::Cell* AtlasEngine::_getCell(u16 x, u16 y) noexcept
     return _r.cells.data() + static_cast<size_t>(_r.cellCount.x) * y + x;
 }
 
+AtlasEngine::TileHashMap::iterator* AtlasEngine::_getCellGlyphMapping(u16 x, u16 y) noexcept
+{
+    assert(x < _r.cellCount.x);
+    assert(y < _r.cellCount.y);
+    return _r.cellGlyphMapping.data() + static_cast<size_t>(_r.cellCount.x) * y + x;
+}
+
 void AtlasEngine::_setCellFlags(u16r coords, CellFlags mask, CellFlags bits) noexcept
 {
     assert(coords.left <= coords.right);
@@ -1399,9 +1442,9 @@ void AtlasEngine::_emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, si
     attributes.cellCount = cellCount;
 
     AtlasKey key{ attributes, gsl::narrow<u16>(charCount), chars };
-    const AtlasValue* valueRef = _r.glyphs.find(key);
+    auto it = _r.glyphs.find(key);
 
-    if (!valueRef)
+    if (it == _r.glyphs.end())
     {
         // Do fonts exist *in practice* which contain both colored and uncolored glyphs? I'm pretty sure...
         // However doing it properly means using either of:
@@ -1440,26 +1483,24 @@ void AtlasEngine::_emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, si
             coords[i] = _r.tileAllocator.allocate(_r.glyphs);
         }
 
-        const auto it = _r.glyphs.insert(std::move(key), std::move(value));
-        valueRef = &it->second;
+        it = _r.glyphs.insert(std::move(key), std::move(value));
         _r.glyphQueue.emplace_back(&it->first, &it->second);
     }
 
-    // For some reason MSVC doesn't understand that valueRef is overwritten in the branch above, resulting in:
-    //   C26430: Symbol 'valueRef' is not tested for nullness on all paths (f.23).
-    __assume(valueRef != nullptr);
-
-    const auto valueData = valueRef->data();
+    const auto valueData = it->second.data();
     const auto coords = &valueData->coords[0];
-    const auto data = _getCell(x1, _api.lastPaintBufferLineCoord.y);
+    const auto cells = _getCell(x1, _api.lastPaintBufferLineCoord.y);
+    const auto cellGlyphMappings = _getCellGlyphMapping(x1, _api.lastPaintBufferLineCoord.y);
 
     for (u32 i = 0; i < cellCount; ++i)
     {
-        data[i].tileIndex = coords[i];
+        cells[i].tileIndex = coords[i];
         // We should apply the column color and flags from each column (instead
         // of copying them from the x1) so that ligatures can appear in multiple
         // colors with different line styles.
-        data[i].flags = valueData->flags | _api.bufferLineMetadata[static_cast<size_t>(x1) + i].flags;
-        data[i].color = _api.bufferLineMetadata[static_cast<size_t>(x1) + i].colors;
+        cells[i].flags = valueData->flags | _api.bufferLineMetadata[static_cast<size_t>(x1) + i].flags;
+        cells[i].color = _api.bufferLineMetadata[static_cast<size_t>(x1) + i].colors;
     }
+
+    std::fill_n(cellGlyphMappings, cellCount, it);
 }
diff --git a/src/renderer/atlas/AtlasEngine.h b/src/renderer/atlas/AtlasEngine.h
index 94e95f6a2a7..d1da93c9bc7 100644
--- a/src/renderer/atlas/AtlasEngine.h
+++ b/src/renderer/atlas/AtlasEngine.h
@@ -166,6 +166,12 @@ namespace Microsoft::Console::Render
         };
 
     private:
+        // I wrote `Buffer` instead of using `std::vector`, because I want to convey that these things
+        // explicitly _don't_ hold resizeable contents, but rather plain content of a fixed size.
+        // For instance I didn't want a resizeable vector with a `push_back` method for my fixed-size
+        // viewport arrays - that doesn't make sense after all. `Buffer` also doesn't initialize
+        // contents to zero, allowing rapid creation/destruction and you can easily specify a custom
+        // (over-)alignment which can improve rendering perf by up to ~20% over `std::vector`.
         template<typename T, size_t Alignment = alignof(T)>
         struct Buffer
         {
@@ -175,19 +181,24 @@ namespace Microsoft::Console::Render
                 _data{ allocate(size) },
                 _size{ size }
             {
+                std::uninitialized_default_construct_n(_data, size);
             }
 
             Buffer(const T* data, size_t size) :
                 _data{ allocate(size) },
                 _size{ size }
             {
-                static_assert(std::is_trivially_copyable_v<T>);
-                memcpy(_data, data, size * sizeof(T));
+                // Changing the constructor arguments to accept std::span might
+                // be a good future extension, but not to improve security here.
+                // You can trivially construct std::span's from invalid ranges.
+                // Until then the raw-pointer style is more practical.
+#pragma warning(suppress : 26459) // You called an STL function '...' with a raw pointer parameter at position '3' that may be unsafe [...].
+                std::uninitialized_copy_n(data, size, _data);
             }
 
             ~Buffer()
             {
-                deallocate(_data);
+                destroy();
             }
 
             Buffer(Buffer&& other) noexcept :
@@ -199,7 +210,7 @@ namespace Microsoft::Console::Render
 #pragma warning(suppress : 26432) // If you define or delete any default operation in the type '...', define or delete them all (c.21).
             Buffer& operator=(Buffer&& other) noexcept
             {
-                deallocate(_data);
+                destroy();
                 _data = std::exchange(other._data, nullptr);
                 _size = std::exchange(other._size, 0);
                 return *this;
@@ -288,6 +299,12 @@ namespace Microsoft::Console::Render
             }
 #pragma warning(pop)
 
+            void destroy() noexcept
+            {
+                std::destroy_n(_data, _size);
+                deallocate(_data);
+            }
+
             T* _data = nullptr;
             size_t _size = 0;
         };
@@ -553,21 +570,28 @@ namespace Microsoft::Console::Render
 
         struct TileHashMap
         {
+            using iterator = std::list<std::pair<AtlasKey, AtlasValue>>::iterator;
+
             TileHashMap() noexcept = default;
 
-            AtlasValue* find(const AtlasKey& key)
+            iterator end() noexcept
+            {
+                return _lru.end();
+            }
+
+            iterator find(const AtlasKey& key)
             {
                 const auto it = _map.find(key);
                 if (it != _map.end())
                 {
                     // Move the key to the head of the LRU queue.
-                    _lru.splice(_lru.begin(), _lru, *it);
-                    return &(*it)->second;
+                    makeNewest(*it);
+                    return *it;
                 }
-                return nullptr;
+                return end();
             }
 
-            std::list<std::pair<AtlasKey, AtlasValue>>::iterator insert(AtlasKey&& key, AtlasValue&& value)
+            iterator insert(AtlasKey&& key, AtlasValue&& value)
             {
                 // Insert the key/value right at the head of the LRU queue, just like find().
                 //
@@ -579,6 +603,11 @@ namespace Microsoft::Console::Render
                 return it;
             }
 
+            void makeNewest(const iterator& it)
+            {
+                _lru.splice(_lru.begin(), _lru, it);
+            }
+
             void popOldestTiles(std::vector<u16x2>& out) noexcept
             {
                 Expects(!_lru.empty());
@@ -602,7 +631,7 @@ namespace Microsoft::Console::Render
             // If you need a LRU hash-map, write a custom one with an intrusive
             // prev/next linked list (it's easier than you might think!).
             std::list<std::pair<AtlasKey, AtlasValue>> _lru;
-            std::unordered_set<std::list<std::pair<AtlasKey, AtlasValue>>::iterator, AtlasKeyHasher, AtlasKeyEq> _map;
+            std::unordered_set<iterator, AtlasKeyHasher, AtlasKeyEq> _map;
         };
 
         // TileAllocator yields `tileSize`-sized tiles for our texture atlas.
@@ -844,6 +873,7 @@ namespace Microsoft::Console::Render
         IDWriteTextFormat* _getTextFormat(bool bold, bool italic) const noexcept;
         const Buffer<DWRITE_FONT_AXIS_VALUE>& _getTextFormatAxis(bool bold, bool italic) const noexcept;
         Cell* _getCell(u16 x, u16 y) noexcept;
+        TileHashMap::iterator* _getCellGlyphMapping(u16 x, u16 y) noexcept;
         void _setCellFlags(u16r coords, CellFlags mask, CellFlags bits) noexcept;
         void _flushBufferLine();
         void _emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, size_t bufferPos2);
@@ -911,6 +941,7 @@ namespace Microsoft::Console::Render
             wil::com_ptr<IDWriteTypography> typography;
 
             Buffer<Cell, 32> cells; // invalidated by ApiInvalidations::Size
+            Buffer<TileHashMap::iterator> cellGlyphMapping; // invalidated by ApiInvalidations::Size
             f32x2 cellSizeDIP; // invalidated by ApiInvalidations::Font, caches _api.cellSize but in DIP
             u16x2 cellSize; // invalidated by ApiInvalidations::Font, caches _api.cellSize
             u16x2 cellCount; // invalidated by ApiInvalidations::Font|Size, caches _api.cellCount
diff --git a/src/renderer/atlas/AtlasEngine.r.cpp b/src/renderer/atlas/AtlasEngine.r.cpp
index 4af0a4728a8..f2b5cdfefb9 100644
--- a/src/renderer/atlas/AtlasEngine.r.cpp
+++ b/src/renderer/atlas/AtlasEngine.r.cpp
@@ -293,11 +293,16 @@ void AtlasEngine::_drawCursor()
     // At 150% scale lineWidth thus needs to be 1.33333... because at a zoom scale of 1.5 this results in a 2px wide line.
     const auto lineWidth = std::max(1.0f, static_cast<float>((_r.dpi + USER_DEFAULT_SCREEN_DPI / 2) / USER_DEFAULT_SCREEN_DPI * USER_DEFAULT_SCREEN_DPI) / static_cast<float>(_r.dpi));
     const auto cursorType = static_cast<CursorType>(_r.cursorOptions.cursorType);
-    D2D1_RECT_F rect;
-    rect.left = 0.0f;
-    rect.top = 0.0f;
-    rect.right = _r.cellSizeDIP.x;
-    rect.bottom = _r.cellSizeDIP.y;
+
+    // `clip` is the rectangle within our texture atlas that's reserved for our cursor texture, ...
+    D2D1_RECT_F clip;
+    clip.left = 0.0f;
+    clip.top = 0.0f;
+    clip.right = _r.cellSizeDIP.x;
+    clip.bottom = _r.cellSizeDIP.y;
+
+    // ... whereas `rect` is just the visible (= usually white) portion of our cursor.
+    auto rect = clip;
 
     switch (cursorType)
     {
@@ -328,6 +333,9 @@ void AtlasEngine::_drawCursor()
     }
 
     _r.d2dRenderTarget->BeginDraw();
+    // We need to clip the area we draw in to ensure we don't
+    // accidentally draw into any neighboring texture atlas tiles.
+    _r.d2dRenderTarget->PushAxisAlignedClip(&clip, D2D1_ANTIALIAS_MODE_ALIASED);
     _r.d2dRenderTarget->Clear();
 
     if (cursorType == CursorType::EmptyBox)
@@ -346,5 +354,6 @@ void AtlasEngine::_drawCursor()
         _r.d2dRenderTarget->FillRectangle(&rect, _r.brush.get());
     }
 
+    _r.d2dRenderTarget->PopAxisAlignedClip();
     THROW_IF_FAILED(_r.d2dRenderTarget->EndDraw());
 }


Test patch: