Closed Bug 1353000 Opened 4 years ago Closed 4 years ago

Shaped-word cache lookups should depend on the values returned by gfxFontShaper::GetRoundOffsetsToPixels

Categories

(Core :: Graphics: Text, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(5 files)

See bug 1352528 comment 1. When we do glyph shaping, the precise glyph positioning is dependent on the results of gfxFontShaper::GetRoundOffsetsToPixels; this is necessary to maintain proper inter-glyph spacing where "ideal" metrics would result in fractional-pixel positioning, but the glyph rasterizer will ultimately snap to the pixel grid.

However, we don't currently respect the roundToPixels values when caching gfxShapedWord records, which means that we can end up using cached glyph data that was generated for a different rounding scenario, leading to inferior glyph spacing.

To fix this, we need to check the GetRoundOffsetsToPixels earlier and use the results as part of the shaped-word cache key.
Attached file 8984055.html
This is a testcase that demonstrates the bug on Windows, at least (results may vary on other platforms, depending on font metrics and rasterizer rounding behavior).

In both canvases, the text is painted twice, first in red and then in green. The green text should exactly overprint the red. However, in the second canvas, we first render the red text (out of view) with a rotation applied, which changes the roundOffsetsToPixels result and causes the shaped text to be cached without the usual rounding rules applied. This alters the glyph spacing at the "To" kern pair, with the result that the green text no longer exactly matches.

(We can show that this is a result of incorrect caching behavior by setting the pref "gfx.font_rendering.wordcache.charlimit" to a very low value (e.g. 1) so that the strings here do not get cached; then, the rendering glitch no longer happens.)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
This builds on the patch in bug 1352528, and fixes things so that we correctly respect the rounding flags when caching shaped words. This resolves the correctness problem shown by the testcase here.
Attachment #8855017 - Flags: review?(jmuizelaar)
No longer blocks: 1352528
Depends on: 1352528
Attachment #8855016 - Flags: review?(jmuizelaar) → review+
Attachment #8855017 - Flags: review?(jmuizelaar) → review+
Jeff - r? ping for the reftest patch (or rubber-stamp? if you're happy for the test to land without a lot of review)... thanks!
Flags: needinfo?(jmuizelaar)
Attachment #8855019 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/10e777bb90dc60f810fb463b829c941adf7a5808
Bug 1353000 - Preparatory patch, explicitly make the Script enum a 16-bit type, so we can add a field to gfxShapedWord without making it bigger. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd127ce305c7527cda20e221fae8e80b63b53a29
Bug 1353000 - Respect the round-to-pixels flags when caching shaped-word data. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef3333cedf5eff2e152770981cb5fc3c5a8d7e5
Bug 1353000 - Reftest to check that shaping rotated text does not disrupt non-rotated text due to cache pollution. r=jrmuizel
Flags: needinfo?(jmuizelaar)
The devtools test failure appears to occur because when the inspector generates the font preview tooltip, it aims to render to a canvas at 2x size, and then downscale it to get better rendering on hi-dpi screens. But unfortunately, it doesn't quite ensure we get exactly integer scaling, and this seems to be at the root of the 1-pixel discrepancy in the test. Fixing the canvas size/scaling to be exactly 2x prevents the test failure. Try run to double-check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c83502f700a1c7bbd3b97520ceecfed4343e15.
Flags: needinfo?(jfkthame)
Attachment #8862413 - Flags: review?(bgrinstead)
Attachment #8862413 - Flags: review?(bgrinstead) → review?(gl)
Attachment #8862413 - Flags: review?(gl) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecddb82a88d8f49b6c4d58871556fcc037ed2d4d
Bug 1353000 - Preparatory patch, explicitly make the Script enum a 16-bit type, so we can add a field to gfxShapedWord without making it bigger. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/795219dcd057dccf452e19a9d095268fc7ef8443
Bug 1353000 - Fix canvas size used for font inspector preview to avoid risk of non-integer scaling that may lead to rounding discrepancies. r=gl

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb34b24b4cd63f732bcb001dbda047dd62044f04
Bug 1353000 - Respect the round-to-pixels flags when caching shaped-word data. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd87238aa41ab662f6fa2c069510da449ffcef0
Bug 1353000 - Reftest to check that shaping rotated text does not disrupt non-rotated text due to cache pollution. r=jrmuizel
You need to log in before you can comment on or make changes to this bug.