Glyph snapping - revert of #1441

[kvark]

Fixes #1451
@staktrace I didn't see much of a difference locally. Would you be able to verify if this works?

---

This change is 

[glennw]

We definitely need to snap text glyphs. Otherwise fractional components in the text position result in blurry text.

[jrmuizel]

Do you have an example page that needs snapping?

…

On Jul 5, 2017 11:41 PM, "Glenn Watson" ***@***.***> wrote: We definitely need to snap text glyphs. Otherwise fractional components in the text position result in blurry text. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1455 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUTbRL8HxaDFP0GOVfno38hAJCOOzpWks5sLFdkgaJpZM4OPHgf> .

[kvark]

@glennw Here is how I see it, sorry if some parts are obvious :)

Glyph rasterization is hard, and we don't do it. Instead, we ask a platform-specific library to do the actual rasterization. That library works within an assumption that it renders on screen, and it takes into account all the details on how glyphs are placed, including sub-pixel offsets. If we do any sort of scaling, rotation, or translation by a fractional offset, after rasterization, we'll ruin the quality of rendered glyph.
Thus, we need to map the rasterized glyphs 1:1 to screen. This means that any transformations (#1453) should be applied prior to rasterization. Snapping, if any, should only be applied on the text run as a whole, and not ever try to scale it.

There is a simple way to ensure that snapping doesn't scale - just setting the snap rectangle size to 0. This PR has been updated to do it.

[staktrace]

@kvark Sadly it doesn't look like this PR fixes the problem. The reftest looks about the same as without the patch. Try push not fully done yet but R7 is failing still: Reftest analyzer link

[kvark]

Thanks @staktrace ! I assume the test is done with the latest commit 63ee6f4

[staktrace]

@kvark yeah: https://hg.mozilla.org/try/rev/b79323ee0c9b765f79a075dbcdde70b385d8dde4

[kvark]

I just noticed why this change doesn't produce desired effect:

    // Ensure that the snap rect is at *least* one device pixel in size.
    // TODO(gw): It's not clear to me that this is "correct". Specifically,
    //           how should it interact with sub-pixel snap rects when there
    //           is a layer transform with scale present? But it does fix
    //           the test cases we have in Servo that are failing without it
    //           and seem better than not having this at all.
    snap_rect.size = max(snap_rect.size, vec2(1.0 / uDevicePixelRatio));

@glennw what tests exactly are affected? As you mention in the comments, it doesn't make sense to adjust the size in local space anyway.

[glennw]

I think that the simplest option for now is to revert that previous change which snaps to the primitive rect, and return it to how it was (snapping to the glyph rect). Then we can work out the "proper" solution after that, which will allow us to update WR in Gecko / Servo without reftest regressions?

[glennw]

@bors-servo r+

[bors-servo]

📌 Commit 44e22da has been approved by glennw

[bors-servo]

⌛ Testing commit 44e22da with merge 8ca0041...

[bors-servo]

☀️ Test successful - status-travis
Approved by: glennw
Pushing 8ca0041 to master...

[staktrace]

Verified that the revert fixed the reftest failure - try push. Thanks!