GlyphSerializer rounding fix

[miloush]

Fixes #7499

Related: #6295

Description

Previous rounding strategy

1. did not calculate the error according to XPS specification
2. updated error with shaping value when font value is used by the client

The PR fixes 1. by keeping track of total advance width (desired and rounded) and 2. by using the font advance width when no value is output to XPS.

Customer Impact

Customers not taking this fix will see either gaps or overlapped characters when producing XPS with text. See #7499 for an example.

Regression

Yes. Introduced by #6295 trying to fix #6525. This PR addresses both issues.

Testing

Tested manually using repro projects for #6525 and #7499.

Risk

Same risks as with #6295. This PR changes relative positioning of glyphs in a GlyphRun to match the bounding box, so it should not cause reflow of documents.

Microsoft Reviewers: Open in CodeFlow

[miloush]

@campeters try this

[campeters]

This works for my test cases. Thank you so much!

[campeters]

When initializing the doubles should we set them to 0.0? Either for style convention or prevent the compiler from needing to do the implicit conversion from int to double?

Also, I was able to get the same result using only a single double to aggregate the error.... the other changes to your logic are what fixed the problem. This works for me:

        // advance width
        // #7499 Advance width needs to be specified if it differs from what is in the font tables. [ECMA-388 O5.5]
        // Most commonly it differs after shaping, e.g. when kerning is applied. (Ex. 12-15)
        // XPS supports floating point values, but in the interest of file size, we want to specify integers.

        double shapingAdvance = _advances[glyph] * _milToEm;
        double fontAdvance = _sideways ? _glyphTypeface.AdvanceHeights[fontIndex] : _glyphTypeface.AdvanceWidths[fontIndex];

        // To minimize rounding errors, we keep track of the unrounded advance total as required by [M5.6].
        int roundedShapingAdvance = (int)Math.Round(_idealAdvanceDelta + shapingAdvance);
        int roundedFontAdvance = (int)Math.Round(fontAdvance);

        if (roundedShapingAdvance != roundedFontAdvance)
        {
            _glyphStringBuider.Append(roundedShapingAdvance.ToString(CultureInfo.InvariantCulture));
            _idealAdvanceDelta += (shapingAdvance - roundedShapingAdvance);
        }
        else
        {
            // when the value comes from the font tables, the specification does not mandate clients to do any rounding
        }

[miloush]

I was able to get the same result using only a single double to aggregate the error

Yes that was the approach of the original fix. However, the XPS specification puts a hard requirement on keeping the total and subtracting the sum of advance to avoid error accumulation.

[singhashish-wpf]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[campeters]

@dipeshmsft @singhashish-wpf - Thanks for the help on this. Would you be able to share any details on when we will see this fix in a public release?

[dipeshmsft]

Thanks @miloush . Looks good to me. We will take it up for the next test pass. As far as I remember, we may need to make some changes in the test code as well. I am on it.

[campeters]

Thanks @miloush . Looks good to me. We will take it up for the next test pass. As far as I remember, we may need to make some changes in the test code as well. I am on it.

@dipeshmsft - Is this something that will be merged into a dotnet 6 maintenance release as well?

[wstaelens]

Thanks @miloush . Looks good to me. We will take it up for the next test pass. As far as I remember, we may need to make some changes in the test code as well. I am on it.

@dipeshmsft - Is this something that will be merged into a dotnet 6 maintenance release as well?

yes @dipeshmsft , how can we get this fix? our customers experience also this XPS issue.

[dipeshmsft]

@campeters @wstaelens, we will be taking this PR for the next round of test pass and once that is complete, we will take this PR for upcoming .NET servicing releases.

[wstaelens]

@campeters @wstaelens, we will be taking this PR for the next round of test pass and once that is complete, we will take this PR for upcoming .NET servicing releases.

@dipeshmsft great! just so that I can inform our customers: are we talking about days, weeks or months?

[dipeshmsft]

@wstaelens, I will get back on this, once the test pass is complete.

[wstaelens]

@wstaelens, I will get back on this, once the test pass is complete.

will this be in regular Windows Updates for customers or OOB?

[dipeshmsft]

The fix causing regression was not included in .NET Framework, so it will not be present in either. This fix will be shipped with .NET SDK servicing release.

[dipeshmsft]

/backport to release/7.0

[singhashish-wpf]

/backport to release/7.0

[singhashish-wpf]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/wpf/actions/runs/4687438204

[wstaelens]

@singhashish-wpf I guess this will be backported to 6.0 also, as this is Long term release (and our products affected are not yet on .net 7)

[dipeshmsft]

@wstaelens, yes this will be backported to 6.0 also.

[dipeshmsft]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/wpf/actions/runs/4695602743