1512010 - teamcoco zoom-in animated text jiggles

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

1. Enable WR
2. Go to https://teamcoco.com/japan
3. Hover mouse over the thumbnails on the timeline


Actual results:

when the text zooms-in on hover, it sort of jiggles.
Or maybe its just cjhanging the anialisaing type?

This will be noticable if you zoom on the page to make the text bigger, and then hover the mouse


Expected results:

not so

Comment 1

[Mayank Bansal]

Attachment: bandicam 2018-12-04 22-12-59-652.mp4

Comment 2

[Aria Beingessner [:Gankra]]

Looks like another case of us failing to properly pipe through MayBeAnimated into disabling subpixel AA/positioning.

Comment 3

[Mayank Bansal]

@kats: could you triage this please as well. this just might be a dupe of others.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Standalone version

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: don't snap text positions in local raster space

We don't want to snap text positions in raster space local as this is used to indicate that the text is animated. There are still legitimate non-transform cases where we want to snap (i.e. no scale/rotate, only translation), so that is why this patch goes through some pain to pass in the raster space and make sure we check it before skipping the snapping.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9035697 [details] [diff] [review]
don't snap text positions in local raster space

Review of attachment 9035697 [details] [diff] [review]:
-----------------------------------------------------------------

Dumping the review on kvark

Comment 8

[Dzmitry Malyshau [:kvark]]

Comment on attachment 9035697 [details] [diff] [review]
don't snap text positions in local raster space

Review of attachment 9035697 [details] [diff] [review]:
-----------------------------------------------------------------

Left some suggestions to improve this.
Unrelated, it's clear how the review experience here is inferior to Phabricator, even with all its warts.

::: gfx/wr/webrender/src/batch.rs
@@ +549,5 @@
>  
>          // Add each run in this picture to the batch.
>          for prim_instance in &pic.prim_list.prim_instances {
>              self.add_prim_to_batch(
> +                pic,

I think passing the whole `pic` here is not idiomatic, since the wrapping code accesses the `prim_list` it should extract whatever is needed from the picture and pass along separately (i.e. `pic.raster_config`)

@@ +830,5 @@
>                                  )
>                              }
>                          };
>  
> +                        let raster_space = RasterizationSpace::from(cur_pic.requested_raster_space);

Using `requested_raster_space` here is not correct. Notice the "hint" in the comments:

    /// Requested rasterization space for this picture. It is
    /// a performance hint only.

For example, having a compositing mode always forces the screen space rasterization.

To improve this, consider adding `raster_space` field to `RasterConfig`.

::: gfx/wr/webrender/src/gpu_types.rs
@@ +58,5 @@
>      Local = 0,
>      Screen = 1,
>  }
>  
> +impl From<RasterSpace> for RasterizationSpace {

I think it would be simpler to just pass `RasterSpace` along, and convert it to an `i32` with a match when writing to the primitive header.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: don't snap text positions in local raster space

After talking with Glenn, we found an alternate way to express this.

It makes far more sense to move the decision of whether to TextRunPrimitive::update_font_instance where we actually already have the intended raster space information. TextRunPrimitive can then store this decision and pass it down to the batching code through the primitive user data.

Comment 10

[Glenn Watson [:gw]]

Comment on attachment 9035794 [details] [diff] [review]
don't snap text positions in local raster space

Review of attachment 9035794 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but can we add a wrench reftest for this?

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: add wrench test for text snapping in raster spaces

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cf58f64e2c2
don't snap text positions in local raster space. r=gw
https://hg.mozilla.org/integration/mozilla-inbound/rev/58b0b6fba4c3
add wrench test for text snapping in raster spaces. r=gw

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3179fe251cc
fix png refs for wrench tests. r=me

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9cf58f64e2c2
https://hg.mozilla.org/mozilla-central/rev/58b0b6fba4c3
https://hg.mozilla.org/mozilla-central/rev/c3179fe251cc

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3989f48b153
backing out local raster space text snapping change. r=jrmuizel

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/2bf2c209f520
backing out local raster space text snapping change. r=jrmuizel

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2bf2c209f520

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/a3989f48b153

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Reduced test case for blurring with patch set, v1

Comment 20

[Jeff Muizelaar [:jrmuizel]]

I set up https://public.etherpad-mozilla.org/p/wr-text-snapping which can contain our brain storming.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

When we're drawing in RasterSpace::Local we rasterize with an identity transform: https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/prim_store/text_run.rs#254 it seems like this should mean that we should snap RasterSpace::Local glyph coords as though there was an identity transform and then scale to the actual transform instead of not snapping at all.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Attachment: part 2 - snap in local space for axis aligned transforms, v1

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Attachment: part 2 - snap in local space for axis aligned transforms, v2 -- complete hack

As requested by Jeff -- always adding the x coordinate snap bias to the snap offset seems to fix both cases. It is obviously not right, but maybe it will be useful for debugging :).

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81842621d427
snap text positions without device transform in local raster space. r=gw

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/81842621d427

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/965265642346
add wrench test for text snapping in raster spaces. r=gw

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/965265642346

Comment 28

[Jeff Muizelaar [:jrmuizel]]

Lee, can you request beta approval for this?

Comment 29

[Lee Salzman [:lsalzman]]

Comment on attachment 9035794 [details] [diff] [review]
don't snap text positions in local raster space

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Text animation in WR will cause broken hinting.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Only affects WR. Just changes appearance of text animation.

String changes made/needed

Comment 30

[Liz Henry (:lizzard)]

Just this patch? Or anything else as well (tests?)

Comment 31

[Lee Salzman [:lsalzman]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #30)

Just this patch? Or anything else as well (tests?)

Just this commit: https://hg.mozilla.org/integration/mozilla-inbound/rev/81842621d427

Comment 32

[Liz Henry (:lizzard)]

Comment on attachment 9035794 [details] [diff] [review]
don't snap text positions in local raster space

Support for WebRender experiments in 66. 
OK to uplift for beta 5 - only this patch.

Comment 33

[Bogdan Tara[:bogdan_tara | bogdant]]

I tried grafting 9cf58f64e2c2 from central to uplift to beta, but got "skipping ancestor revision 506869:9cf58f64e2c2".

I tried to qimport the patch but got:
applying no-local-round.diff
patching file gfx/wr/webrender/res/ps_text_run.glsl
Hunk #1 FAILED at 57
Hunk #2 FAILED at 172
Hunk #3 FAILED at 208
3 out of 3 hunks FAILED -- saving rejects to file gfx/wr/webrender/res/ps_text_run.glsl.rej
patching file gfx/wr/webrender/src/batch.rs
Hunk #1 FAILED at 823
1 out of 1 hunks FAILED -- saving rejects to file gfx/wr/webrender/src/batch.rs.rej
patching file gfx/wr/webrender/src/prim_store/text_run.rs
Hunk #1 FAILED at 59
Hunk #2 FAILED at 220
2 out of 3 hunks FAILED -- saving rejects to file gfx/wr/webrender/src/prim_store/text_run.rs.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh no-local-round.diff

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/25a0bd49c183

Comment 35

[Rares Doghi, Desktop QA]

Hi, After checking this issue with WebRender on and Off it seems that this issue still occurs, it is an improvement to previous versions and you can barely notice it on most sites but here is where you can still notice the text and image jiggle a bit: http://museumofwifi.com/

Comment 36

[Jeff Muizelaar [:jrmuizel]]

Are you talking about the snap at the end of the animation or do you see jiggle during the animation?

Comment 37

[Rares Doghi, Desktop QA]

Attachment: 2019-02-07_17h33_39.mp4

It jiggles the entire image a bit and the text looks like its loading textures, its not that noticeable, but without WebRender its a bit better. I hope the attachment shows what I mean, I tested this with Nvidia 730 on Windows 10 64bit.

Comment 38

[Rares Doghi, Desktop QA]

This issue can be noticed on Samsung.com as well with the thumbnails from the page.

Comment 39

[Lee Salzman [:lsalzman]]

I think it would be best to file a follow-up bug for these new testcases rather than keep adding to this bug.

Comment 40

[Rares Doghi, Desktop QA]

Hi Lee

We thought it's the same issue, same exact behavior since the images from the thumbnails were moving, the same goes with text, and there has been other defects like Bug 1491920 but they have been marked as duplicate for this one and in those comments you can see a few more websites like this: https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/variable-fonts/ where the text is acting a bit weird, granted is not jiggling as bad as before but theres still something weird happening with the text and if you turn WebRender off is a little better. (a little better in a way that its not moving as much as it loading its textures or something)

We can add defects for Samsung and the other website if you think its best but they might just end up duplicates for this bug.

Please let me know if I should add a separate defect for those cases.
Thanks

Comment 41

[Lee Salzman [:lsalzman]]

Please file them as a different bug.

Comment 42

[Rares Doghi, Desktop QA]

Here it is as requested, Bug 1526298. I will mark this issue accordingly since the main issues of this bug have been resolved.