Skip to content

Commit

Permalink
Restore simple text runs, correct for crashes (#6695)
Browse files Browse the repository at this point in the history 
Restores the simple text run analysis and skipping of most of the
shaping/layout steps. Corrects one of the fast-path steps to ensure that
offsets and clusters are assigned.

## References
- Bug #6488 
- Bug #6664
- Simple run PR #6206 
- Simple run revert PR #6665
- Recycle glyph runs PR #6483

The "correction" functions, by which box drawing analysis is one of
them, is dependent on the steps coming before it properly assigning the
four main vectors of the text layout glyphs: indices, advances, offsets,
and clusters. When the fast path is identified by the code from #6206,
only two of those are fully updated: indices and advances. The offsets
doesn't tend to cause a problem because offsets are rarely used so
they're pretty much always 0 already (but this PR enforces that they're
zero for the simple/fast path.) The clusters, however, were not mapped
for the fast path. This showed itself in one of two ways:
1. Before the recycled runs PR #6483, the cluster map had a 0 in every
   field for the stock initialized vector.
2. After the recycled runs PR #6483, the cluster map had the previous
   run's mapping in it.

This meant that when we reached the steps where glyph runs were
potentially split during the correction phase for box drawing
characters, unexpected values were present to map the glyph indices to
clusters and were corrected, adjusted, or split in an unexpected
fashion. 

For instance, the index out of range bug could appear when the default 0
values appended to the end of the clusters vector were decremented down
to a negative value during the run splitter as the true DWrite cluster
mapper doesn't generate that sort of pattern in the slow path case
without also breaking the run itself.

The resolution here is therefore to ensure that all of the fields
related to glyph layout are populated even in the fast path. This
doesn't affect the slow path because that one always populated all
fields by asking DWrite to do it. The fast path just skips a bunch of
DWrite steps because it can implicitly identify patterns and save a
bunch of time.

I've also identified a few vectors that weren't cleared on reset/reuse
of the layout. I'm clearing those now so the `.resize()` operations
performed on them to get to the correct lengths will fill them with
fresh and empty values instead of hanging on to ones that may have been
from the previous. This should be OK memory perf wise because the act of
`.clear()` on a vector shouldn't free anything, just mark it invalid.
And doing `.resize()` from an empty one should just default construct
them into already allocated space (which ought to be super quick).

## Validation
* [x] Far.exe doesn't crash and looks fine
* [x] "\e[30;47m\u{2500} What \u{2500}\e[m" from #6488 appears
  appropriately antialiased
* [x] Validate the "\e[30;47m\u{2500} What \u{2500}\e[m" still works
  when `FillGeometry` is nerfed as a quick test that the runs are split
  correctly.
* [x] Put `u{fffd} into Powershell Core to make a replacement char in
  the output. Then press enter a few times and see that shrunken initial
  characters on random rows. Verify this is gone.

Closes #6668
Closes #6669

Co-Authored-By: Chester Liu <[email protected]>
  • Loading branch information
@miniksa @skyline75489
miniksa and skyline75489 authored Jun 29, 2020
1 parent aa1ed0a commit c4885f1
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 11 deletions.
107 changes: 96 additions & 11 deletions src/renderer/dx/CustomTextLayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ CustomTextLayout::CustomTextLayout(gsl::not_null<IDWriteFactory1*> const factory
_runs{},
_breakpoints{},
_runIndex{ 0 },
_width{ width }
_width{ width },
_isEntireTextSimple{ false }
{
// Fetch the locale name out once now from the format
_localeName.resize(gsl::narrow_cast<size_t>(format->GetLocaleNameLength()) + 1); // +1 for null
Expand All @@ -58,8 +59,15 @@ try
_runs.clear();
_breakpoints.clear();
_runIndex = 0;
_isEntireTextSimple = false;
_textClusterColumns.clear();
_text.clear();
_glyphScaleCorrections.clear();
_glyphClusters.clear();
_glyphIndices.clear();
_glyphDesignUnitAdvances.clear();
_glyphAdvances.clear();
_glyphOffsets.clear();
return S_OK;
}
CATCH_RETURN()
Expand Down Expand Up @@ -105,6 +113,7 @@ CATCH_RETURN()
RETURN_HR_IF_NULL(E_INVALIDARG, columns);
*columns = 0;

RETURN_IF_FAILED(_AnalyzeTextComplexity());
RETURN_IF_FAILED(_AnalyzeRuns());
RETURN_IF_FAILED(_ShapeGlyphRuns());

Expand Down Expand Up @@ -135,6 +144,7 @@ CATCH_RETURN()
FLOAT originX,
FLOAT originY) noexcept
{
RETURN_IF_FAILED(_AnalyzeTextComplexity());
RETURN_IF_FAILED(_AnalyzeRuns());
RETURN_IF_FAILED(_ShapeGlyphRuns());
RETURN_IF_FAILED(_CorrectGlyphRuns());
Expand All @@ -148,6 +158,44 @@ CATCH_RETURN()
return S_OK;
}

// Routine Description:
// - Uses the internal text information and the analyzers/font information from construction
// to determine the complexity of the text. If the text is determined to be entirely simple,
// we'll have more chances to optimize the layout process.
// Arguments:
// - <none> - Uses internal state
// Return Value:
// - S_OK or suitable DirectWrite or STL error code
[[nodiscard]] HRESULT CustomTextLayout::_AnalyzeTextComplexity() noexcept
{
try
{
const auto textLength = gsl::narrow<UINT32>(_text.size());

BOOL isTextSimple = FALSE;
UINT32 uiLengthRead = 0;

// Start from the beginning.
const UINT32 glyphStart = 0;

_glyphIndices.resize(textLength);

const HRESULT hr = _analyzer->GetTextComplexity(
_text.c_str(),
textLength,
_font.Get(),
&isTextSimple,
&uiLengthRead,
&_glyphIndices.at(glyphStart));

RETURN_IF_FAILED(hr);

_isEntireTextSimple = isTextSimple && uiLengthRead == textLength;
}
CATCH_RETURN();
return S_OK;
}

// Routine Description:
// - Uses the internal text information and the analyzers/font information from construction
// to determine the complexity of the text inside this layout, compute the subsections (or runs)
Expand All @@ -170,19 +218,13 @@ CATCH_RETURN()
// This result will be subdivided by the analysis processes.
_runs.resize(1);
auto& initialRun = _runs.front();
initialRun.nextRunIndex = 0;
initialRun.textStart = 0;
initialRun.textLength = textLength;
initialRun.bidiLevel = (_readingDirection == DWRITE_READING_DIRECTION_RIGHT_TO_LEFT);

// Allocate enough room to have one breakpoint per code unit.
_breakpoints.resize(_text.size());

BOOL isTextSimple = FALSE;
UINT32 uiLengthRead = 0;
RETURN_IF_FAILED(_analyzer->GetTextComplexity(_text.c_str(), textLength, _font.Get(), &isTextSimple, &uiLengthRead, NULL));

if (!(isTextSimple && uiLengthRead == _text.size()))
if (!_isEntireTextSimple)
{
// Call each of the analyzers in sequence, recording their results.
RETURN_IF_FAILED(_analyzer->AnalyzeLineBreakpoints(this, 0, textLength, this));
Expand Down Expand Up @@ -303,6 +345,42 @@ CATCH_RETURN()
_glyphIndices.resize(totalGlyphsArrayCount);
}

if (_isEntireTextSimple)
{
// When the entire text is simple, we can skip GetGlyphs and directly retrieve glyph indices and
// advances(in font design unit). With the help of font metrics, we can calculate the actual glyph
// advances without the need of GetGlyphPlacements. This shortcut will significantly reduce the time
// needed for text analysis.
DWRITE_FONT_METRICS1 metrics;
run.fontFace->GetMetrics(&metrics);

// With simple text, there's only one run. The actual glyph count is the same as textLength.
_glyphDesignUnitAdvances.resize(textLength);
_glyphAdvances.resize(textLength);

USHORT designUnitsPerEm = metrics.designUnitsPerEm;

RETURN_IF_FAILED(_font->GetDesignGlyphAdvances(
textLength,
&_glyphIndices.at(glyphStart),
&_glyphDesignUnitAdvances.at(glyphStart),
run.isSideways));

for (size_t i = glyphStart; i < _glyphAdvances.size(); i++)
{
_glyphAdvances.at(i) = (float)_glyphDesignUnitAdvances.at(i) / designUnitsPerEm * _format->GetFontSize() * run.fontScale;
}

// Set all the clusters as sequential. In a simple run, we're going 1 to 1.
// Fill the clusters sequentially from 0 to N-1.
std::iota(_glyphClusters.begin(), _glyphClusters.end(), gsl::narrow_cast<unsigned short>(0));

run.glyphCount = textLength;
glyphStart += textLength;

return S_OK;
}

std::vector<DWRITE_SHAPING_TEXT_PROPERTIES> textProps(textLength);
std::vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProps(maxGlyphCount);

Expand Down Expand Up @@ -400,6 +478,12 @@ CATCH_RETURN()
{
try
{
// For simple text, there is no need to correct runs.
if (_isEntireTextSimple)
{
return S_OK;
}

// Correct each run separately. This is needed whenever script, locale,
// or reading direction changes.
for (UINT32 runIndex = 0; runIndex < _runs.size(); ++runIndex)
Expand Down Expand Up @@ -513,6 +597,10 @@ CATCH_RETURN()
// 1 1 .8 1 1
}

// Dump the glyph scale corrections now that we're done with them.
_glyphScaleCorrections.clear();

// Order the runs.
_OrderRuns();
}
CATCH_RETURN();
Expand All @@ -537,9 +625,6 @@ try

// We're going to walk through and check for advances that don't match the space that we expect to give out.

DWRITE_FONT_METRICS1 metrics;
run.fontFace->GetMetrics(&metrics);

// Glyph Indices represents the number inside the selected font where the glyph image/paths are found.
// Text represents the original text we gave in.
// Glyph Clusters represents the map between Text and Glyph Indices.
Expand Down
7 changes: 7 additions & 0 deletions src/renderer/dx/CustomTextLayout.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ namespace Microsoft::Console::Render
[[nodiscard]] HRESULT STDMETHODCALLTYPE _AnalyzeBoxDrawing(gsl::not_null<IDWriteTextAnalysisSource*> const source, UINT32 textPosition, UINT32 textLength);
[[nodiscard]] HRESULT STDMETHODCALLTYPE _SetBoxEffect(UINT32 textPosition, UINT32 textLength);

[[nodiscard]] HRESULT _AnalyzeTextComplexity() noexcept;
[[nodiscard]] HRESULT _AnalyzeRuns() noexcept;
[[nodiscard]] HRESULT _ShapeGlyphRuns() noexcept;
[[nodiscard]] HRESULT _ShapeGlyphRun(const UINT32 runIndex, UINT32& glyphStart) noexcept;
Expand Down Expand Up @@ -183,6 +184,9 @@ namespace Microsoft::Console::Render

// Glyph shaping results

// Whether the entire text is determined to be simple and does not require full script shaping.
bool _isEntireTextSimple;

std::vector<DWRITE_GLYPH_OFFSET> _glyphOffsets;

// Clusters are complicated. They're in respect to each individual run.
Expand All @@ -194,6 +198,9 @@ namespace Microsoft::Console::Render
// This appears to be the index of the glyph inside each font.
std::vector<UINT16> _glyphIndices;

// This is for calculating glyph advances when the entire text is simple.
std::vector<INT32> _glyphDesignUnitAdvances;

std::vector<float> _glyphAdvances;

struct ScaleCorrection
Expand Down

0 comments on commit c4885f1

Please sign in to comment.