teamcoco zoom-in animated text jiggles

VERIFIED FIXED in Firefox 66

Status

()

P2
normal
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: mayankleoboy1, Assigned: lsalzman)

Tracking

(Blocks: 1 bug)

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 verified, firefox67 verified)

Details

(URL)

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

4 months ago
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
(Reporter)

Comment 1

4 months ago
(Reporter)

Updated

4 months ago
Looks like another case of us failing to properly pipe through MayBeAnimated into disabling subpixel AA/positioning.
(Reporter)

Comment 3

3 months ago
@kats: could you triage this please as well. this just might be a dupe of others.
Flags: needinfo?(kats)
Blocks: 1386669
Priority: -- → P3
Summary: zoom-in animated text jiggles → teamcoco zoom-in animated text jiggles
(Reporter)

Updated

3 months ago
Flags: needinfo?(kats)
Assignee: nobody → jmuizelaar
Assignee: jmuizelaar → lsalzman
(Assignee)

Comment 5

3 months ago

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.

Attachment #9035697 - Flags: review?(jmuizelaar)
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
Attachment #9035697 - Flags: review?(jmuizelaar) → review?(dmalyshau)
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1491920
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.
Attachment #9035697 - Flags: review?(dmalyshau) → review-
(Assignee)

Comment 9

3 months ago

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.

Attachment #9035697 - Attachment is obsolete: true
Attachment #9035794 - Flags: review?(gwatson)
Attachment #9035794 - Flags: feedback?(dmalyshau)
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?
Attachment #9035794 - Flags: review?(gwatson) → review+
Attachment #9035794 - Flags: feedback?(dmalyshau) → feedback+

Updated

2 months ago
Attachment #9035843 - Flags: review?(gwatson) → review+

Comment 12

2 months ago
Pushed by lsalzman@mozilla.com:
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 14

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Updated

2 months ago
Depends on: 1520126

Comment 15

2 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3989f48b153
backing out local raster space text snapping change. r=jrmuizel
(Assignee)

Updated

2 months ago
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
status-firefox66: fixed → ---
Keywords: leave-open
Target Milestone: mozilla66 → ---

Comment 16

2 months ago
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/2bf2c209f520
backing out local raster space text snapping change. r=jrmuizel

Comment 17

2 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
(Assignee)

Updated

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox66: fixed → affected

Comment 18

2 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
(Assignee)

Updated

2 months ago
Status: RESOLVED → REOPENED
status-firefox66: fixed → affected
Resolution: FIXED → ---
Priority: P3 → P2

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

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.

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

2 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81842621d427
snap text positions without device transform in local raster space. r=gw

Comment 25

2 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
status-firefox67: --- → fixed
Resolution: --- → FIXED

Comment 26

2 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/965265642346
add wrench test for text snapping in raster spaces. r=gw

Lee, can you request beta approval for this?

Flags: needinfo?(lsalzman)
(Assignee)

Comment 29

2 months ago

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

Flags: needinfo?(lsalzman)
Attachment #9035794 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 months ago
See Also: → bug 1491942

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

Flags: needinfo?(lsalzman)
(Assignee)

Comment 31

2 months ago

(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

Flags: needinfo?(lsalzman)
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.
Attachment #9035794 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: needinfo?(lsalzman)

Comment 35

2 months ago

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/

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

Flags: needinfo?(rares.doghi)

Comment 37

2 months ago

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.

Flags: needinfo?(rares.doghi)

Comment 38

2 months ago

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

(Assignee)

Comment 39

2 months ago

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

2 months ago

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

Flags: needinfo?(lsalzman)
(Assignee)

Comment 41

2 months ago

Please file them as a different bug.

Flags: needinfo?(lsalzman)

Comment 42

2 months ago

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

Status: RESOLVED → VERIFIED
status-firefox66: fixed → verified
status-firefox67: fixed → verified
You need to log in before you can comment on or make changes to this bug.