Skip to content

Commit

Permalink
AtlasEngine: Harden against empty target sizes (#15615)
Browse files Browse the repository at this point in the history
The WPF control has a minor bug where it initializes the renderer
when there isn't even a window yet. When it then calls `SetWindowSize`
it'll pass the result of `GetWindowRect` which is `0,0,0,0`.
This made AtlasEngine unhappy because it restricted the glyph atlas
size to some multiple of the window size. If the window size is `0,0`
then there couldn't be a glyph atlas and so it crashed.

## Validation Steps Performed
* Fixes WPF test control crash on startup ✅
  • Loading branch information
lhecker authored Jun 30, 2023
1 parent d628c46 commit a084834
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 12 deletions.
21 changes: 13 additions & 8 deletions src/renderer/atlas/AtlasEngine.api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -420,14 +420,19 @@ void AtlasEngine::SetWarningCallback(std::function<void(HRESULT)> pfn) noexcept

[[nodiscard]] HRESULT AtlasEngine::SetWindowSize(const til::size pixels) noexcept
{
u16x2 newSize;
RETURN_IF_FAILED(vec2_narrow(pixels.width, pixels.height, newSize));

// At the time of writing:
// When Win+D is pressed, `TriggerRedrawCursor` is called and a render pass is initiated.
// As conhost is in the background, GetClientRect will return {0,0} and we'll get called with {0,0}.
// This isn't a valid value for _api.sizeInPixel and would crash _recreateSizeDependentResources().
if (_api.s->targetSize != newSize && newSize != u16x2{})
// When Win+D is pressed, `GetClientRect` returns {0,0}.
// There's probably more situations in which our callers may pass us invalid data.
if (!pixels)
{
return S_OK;
}

const u16x2 newSize{
gsl::narrow_cast<u16>(clamp<til::CoordType>(pixels.width, 1, u16max)),
gsl::narrow_cast<u16>(clamp<til::CoordType>(pixels.height, 1, u16max)),
};

if (_api.s->targetSize != newSize)
{
_api.s.write()->targetSize = newSize;
}
Expand Down
5 changes: 3 additions & 2 deletions src/renderer/atlas/BackendD3D.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,6 @@ void BackendD3D::_resetGlyphAtlas(const RenderingPayload& p)

const auto minAreaByFont = cellArea * 95; // Covers all printable ASCII characters
const auto minAreaByGrowth = static_cast<u32>(_rectPacker.width) * _rectPacker.height * 2;
const auto min = std::max(minArea, std::max(minAreaByFont, minAreaByGrowth));

// It's hard to say what the max. size of the cache should be. Optimally I think we should use as much
// memory as is available, but the rendering code in this project is a big mess and so integrating
Expand All @@ -702,7 +701,9 @@ void BackendD3D::_resetGlyphAtlas(const RenderingPayload& p)
// we're locked into a state, where on every render pass we're starting with a half full atlas, drawing once,
// filling it with the remaining half and drawing again, requiring two rendering passes on each frame.
const auto maxAreaByFont = targetArea + targetArea / 4;
const auto area = std::min(maxArea, std::min(maxAreaByFont, min));

auto area = std::min(maxAreaByFont, std::max(minAreaByFont, minAreaByGrowth));
area = clamp(area, minArea, maxArea);

// This block of code calculates the size of a power-of-2 texture that has an area larger than the given `area`.
// For instance, for an area of 985x1946 = 1916810 it would result in a u/v of 2048x1024 (area = 2097152).
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/atlas/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,8 @@ namespace Microsoft::Console::Render::Atlas
til::generational<FontSettings> font;
til::generational<CursorSettings> cursor;
til::generational<MiscellaneousSettings> misc;
u16x2 targetSize{};
u16x2 cellCount{};
u16x2 targetSize{ 1, 1 };
u16x2 cellCount{ 1, 1 };
};

using GenerationalSettings = til::generational<Settings>;
Expand Down

0 comments on commit a084834

Please sign in to comment.