round off subpixel offsets from scroll origins

[lsalzman]

This is to address downstream Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1458921

The problem is that when both the glyphs and the transform contain a subpixel offset, they can combine together to produce visual artifacts. If you imagine two glyphs, one with offset 0.3, and one with offset, 0.2, if positioned at scroll offset 0, they both round to device pixel 0. If the scroll offset becomes 0.2, suddenly the 0.3 rounds up to device pixel 1, while the 0.2 glyph still rounds to 0. So this can contribute to the appearance of a wavefront of messed up hinting as you slowly scroll along.

We're not necessarily doing things differently in the text shader itself, as both Cairo and Skia round the position after the transform is applied. However, in Gecko, we ensure that never let Cairo or Skia see a subpixel offset from scrolling so this issue never arises, despite them handling rounding of glyph positioning at a similar time during rasterization.

Thus, if we just ensure the scroll offset is rounded up front like in Gecko, the problem disappears.

---

This change is 

[kvark]

Looks reasonable! Could you start a try push with this change?

[mstange]

Is the rounded value in screen space or in local space? The rounding needs to happen in screen space.

[gw3583]

This does seem like the right place to do it, I think. I'm fairly certain at some point in the past WR used to do this already, maybe it got lost somewhere along the line. You should probably scale it by the device pixel ratio (should be available in a parent function somewhere), which will make the rounding to a single device pixel.

[lsalzman]

Okay, this is v2. As the new commit message implies, there was a lot of messy uncertainty about glyph subpixel positioning going on, because we were treating them in local space. That really doesn't work well under transforms. Instead, I made GlyphKeys record subpixel positions in device-space. In the process I moved the nastiness of subpixel directions out of the API (as Skia does - you can only toggle it on or off, but the direction is automatic). Likewise, I made the axis handling of subpixel directions more like how Skia does it - unless the transform is axis-aligned, you get subpixel precision on all axes.

Finally, the cherry on top, is that I am now handling snapping of the scroll node transform in the text shader itself. Any other approach that tried to modify the transform up front affected the other shaders in a way that made Gecko try very very unhappy with regressions. With this approach, all we have is some non-threatening fuzzable differences on a couple tests, but otherwise it still fixes the original bug in question here.

[bors-servo]

☔ The latest upstream changes (presumably #2763) made this pull request unmergeable. Please resolve the merge conflicts.

[gw3583]

Reviewed 28 of 28 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

webrender/src/prim_store.rs, line 817 at r1 (raw file):

            for (i, src) in src_glyphs.enumerate() {
                let layout_offset = src.point + self.offset;
                let world_offset = font.transform.transform(&layout_offset);

It seems like if there is a property animation occurring, this would never get the new transform, since this branch would be skipped by the glyph_keys.is_empty() check above. I guess we need to detect if this has changed since we last calculated the glyph keys to invalidate them.

---

webrender/src/glyph_rasterizer/mod.rs, line 142 at r1 (raw file):

    pub fn get_subpx_dir(&self) -> SubpixelDirection {
        if self.skew_y == 0.0 {

Should these be epsilon checks due to float precision?

---

Comments from Reviewable

[gw3583]

This generally looks great to me. There's a couple of issues noted above, and then we'll need to get this rebased + do a gecko try run. Then this should be ready to get merged.

[lsalzman]

Review status: 24 of 28 files reviewed at latest revision, 2 unresolved discussions.

---

webrender/src/prim_store.rs, line 817 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

It seems like if there is a property animation occurring, this would never get the new transform, since this branch would be skipped by the glyph_keys.is_empty() check above. I guess we need to detect if this has changed since we last calculated the glyph keys to invalidate them.

I made it remember the glyph transform and then compare that for invalidation.

---

webrender/src/glyph_rasterizer/mod.rs, line 142 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Should these be epsilon checks due to float precision?

Modified these to use approx_eq

---

Comments from Reviewable

[lsalzman]

Okay, fixed the mentioned issues. Gecko try run was already done. Only some things I need to fuzz on the Gecko-side, but otherwise everything is okay.

---

Review status: 24 of 28 files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[gw3583]

Looks like there are some CI failures (I haven't looked into them).

It would be good to paste the try run results here, so that we know which tests are awaiting annotation updates while landing other PRs until the next WR update.

[lsalzman]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=279b976749c47591480a46e4dd31bffcf1feb77e

[lsalzman]

It looks like the failing test is wrench rawtest which does not fail locally at all. The rawtest failure hypothetically seems unrelated to this, but I am not sure exactly what rawtest does?

[gw3583]

This is the rawtest failure:

hidpi factor: 1 (native 1)
Shader override path: None
	hit testing test...
	retained blob images test...
thread 'main' panicked at 'assertion failed: called.load(Ordering::SeqCst) == 1', wrench/src/rawtest.rs:261:9
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:207
   3: std::panicking::default_hook
             at libstd/panicking.rs:223
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:402
   5: <unknown>
             at /checkout/src/libstd/panicking.rs:365
Traceback (most recent call last):
  File "script/headless.py", line 75, in <module>
    subprocess.check_call(['../target/release/wrench', '--no-scissor', '-h'] + sys.argv[1:])
  File "/usr/lib/python2.7/subprocess.py", line 541, in check_call
    raise CalledProcessError(retcode, cmd)

I ran your branch locally on my machine, which also does not fail locally. @jrmuizel any ideas what the cause of this might be?

[lsalzman]

Did a rebase and pushed again, and the rawtest didn't fail this time. Maybe that blob image rawtest is one of our first intermittent failures? :(

Either way, I don't think I am necessarily the cause of it.

[lsalzman]

Jeff made PR #2775 to hopefully catch the intermittent sooner so it can be investigated.

[gw3583]

@bors-servo r+ 🚀

[bors-servo]

📌 Commit 58ccf52 has been approved by gw3583

[bors-servo]

⌛ Testing commit 58ccf52 with merge 4b92630...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing 4b92630 to master...