teamcoco zoom-in animated text jiggles
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
People
(Reporter: mayankleoboy1, Assigned: lsalzman)
References
()
Details
Attachments
(8 files, 1 obsolete file)
1.06 MB,
video/mp4
|
Details | |
3.92 KB,
text/html
|
Details | |
7.34 KB,
patch
|
gw
:
review+
kvark
:
feedback+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
gw
:
review+
|
Details | Diff | Splinter Review |
201 bytes,
patch
|
Details | Diff | Splinter Review | |
2.12 KB,
patch
|
Details | Diff | Splinter Review | |
2.00 KB,
patch
|
Details | Diff | Splinter Review | |
2.42 MB,
video/mp4
|
Details |
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•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Comment 2•5 years ago
|
||
Looks like another case of us failing to properly pipe through MayBeAnimated into disabling subpixel AA/positioning.
Reporter | ||
Comment 3•5 years ago
|
||
@kats: could you triage this please as well. this just might be a dupe of others.
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 4•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years 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.
Comment 6•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 9•5 years 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.
Comment 10•5 years ago
|
||
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?
Assignee | ||
Comment 11•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 12•5 years 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 13•5 years ago
|
||
Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3179fe251cc fix png refs for wrench tests. r=me
Comment 14•5 years ago
|
||
bugherder |
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•5 years 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•5 years ago
|
Updated•5 years ago
|
Comment 16•5 years 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•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 18•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Comment 20•5 years ago
|
||
I set up https://public.etherpad-mozilla.org/p/wr-text-snapping which can contain our brain storming.
Comment 21•5 years ago
|
||
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•5 years ago
|
||
Comment 23•5 years ago
|
||
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•5 years 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•5 years ago
|
||
bugherder |
Comment 26•5 years 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
Comment 27•5 years ago
|
||
bugherder |
Assignee | ||
Comment 29•5 years 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
Comment 30•5 years ago
|
||
Just this patch? Or anything else as well (tests?)
Assignee | ||
Comment 31•5 years 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
Comment 32•5 years ago
|
||
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•5 years ago
|
||
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
Updated•5 years ago
|
Comment 34•5 years ago
|
||
bugherder uplift |
Comment 35•5 years 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/
Comment 36•5 years ago
|
||
Are you talking about the snap at the end of the animation or do you see jiggle during the animation?
Comment 37•5 years 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.
Comment 38•5 years ago
|
||
This issue can be noticed on Samsung.com as well with the thumbnails from the page.
Assignee | ||
Comment 39•5 years 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•5 years 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
Comment 42•5 years ago
|
||
Here it is as requested, Bug 1526298. I will mark this issue accordingly since the main issues of this bug have been resolved.
Description
•