1435094 - Characters of text in animated boxes on qt.io skip pixels rather than animating smoothly

Status: RESOLVED FIXED

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

Description

[Zaggy1024]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180129220114

Steps to reproduce:

Opened www.qt.io with gfx.webrender.all enabled, and hovered over one of the sliding rotated boxes on the right.


Actual results:

The text that appears in the boxes when they are hovered skips whole pixels rather than moving smoothly, presumably due to pixel snapping of the characters.


Expected results:

The text moves smoothly between pixels while staying sharp enough to read.

Comment 1

[Darkspirit]

Thank you! Confirmed in Nightly 60 x64 20180201100326 de_DE @ Debian Testing (KDE, Radeon RX480).
fresh profile: gfx.webrender.all

Comment 2

[Darkspirit]

Attachment: 2018-02-02_01-07-00.mp4

Comment 3

[Aria Beingessner [:Gankra]]

Seems to be a general problem affecting all platforms, so probably something in our platform-independent code.

Comment 4

[Aria Beingessner [:Gankra]]

This seems to have been fixed somehow? Can anyone else still reproduce?

Comment 5

[Aria Beingessner [:Gankra]]

Oh no wait nevermind, I was looking at text that was part of the image, I think.

Comment 6

[Aria Beingessner [:Gankra]]

Hey jfkthame, we seem to be getting ugly results in webrender from applying vertical text snapping to rotated glyphs, and the result is extra ugly when the text is animated. Vanilla gecko seems to handle this well, suggesting it has some heuristic/method for disabling vertical snapping. Ever stumbled across that before?

Comment 7

[Jonathan Kew [:jfkthame]]

IIRC, we basically turn off pixel-snapping if there's a transform (other than just a translation) in effect. See for example gfxContext::UserToDevicePixelSnapped, and its callers such as nsLayoutUtils::GetSnappedBaselineY.

Comment 8

[Glenn Watson [:gw]]

Right now, WR *always* rasterizes glyphs on transformed elements in screen-space. We want to support this for non-animated transformed elements, to get high quality AA in this cases.

This is not great for performance, and also not wanted for animated elements (causing bugs such as this one).

What we need to do is:
 * Add an API to push_stacking_context that allows the caller to specify whether glyphs should be screen-space or local-space rasterized.
 * Push that through the various frame building functions to where glyphs are requested.

WR already disables pixel snapping on elements with a non-axis-aligned transform, so the above should hopefully be all that is needed to resolve this bug.

Comment 9

[Aria Beingessner [:Gankra]]

Ok hashed this out with glenn on irc, the backend work does indeed seem trivial here. I got the desirable rendering results by just modifying TextRunPrimitiveCpu::get_font to never feed through the transform to the font (but still check it for AA selection).

I think we could potentially just ship that, since that's the only logic gecko seems to be using (per jfkthame's comment)? However it's *probably* a better decision to "do it right" and do what glenn suggested and add a flag that is set by gecko and piped through to get_font?

Comment 10

[Aria Beingessner [:Gankra]]

Ok no wait gecko does actually subpixel rotated text, so now I'm confused and should just go to bed and look at this with fresh eyes.

Comment 11

[Aria Beingessner [:Gankra]]

Hooking up GlyphRasterSpace to try to fix this, WIP try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b90b7275169c9a8fdfaa96590948e54d7a69fd3

Comment 12

[Aria Beingessner [:Gankra]]

Attachment: Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

When a transform thinks it's animated we should abandon screen rasterization
and instead favour local rasterization. This produces a more visually
pleasant rendering, as pixel-snapping "wobbles" the text between
frames.

The float scale of GlyphRasterSpace::Local is currently unused, but this
PR tries its best to set it to a reasonable value, based on discussion
with glennw about the intended semantics. We agreed it should specify
the scale *relative* to the parent stacking context, which means it's
just whatever scaling the stacking context's transform applies. It's
possible we'll need to clamp this value or make it properly 2-dimensional
later on.

Some book-keeping is added to StackingContextHelper and DisplayListBuilder
to ensure that GlyphRasterSpace::Screen is never requested by a descendent
of a stacking context using GlyphRasterSpace::Local.

In addition, this is the first commit making use of cbindgen's new support
for bridging Rust enums natively into C++! This bumps our minimum cbindgen
to 6.0.0 (just released).

Review commit: https://reviewboard.mozilla.org/r/239762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239762/

Comment 13

[Aria Beingessner [:Gankra]]

Whoops, submitted the wrong commit to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e073967187b73c89d983393ac023d1f5e0928c

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

https://reviewboard.mozilla.org/r/239762/#review245498

r+ with comments addressed, but mstange should at least review the nsDisplayList.cpp change since I can't really comment on that. Note that the missing initializer in the StackingContextHelper::StackingContextHelper() constructor is a bug because now you'll randomly do local rasterization of the entire display list because you're using a garbage value. Your try push might have random failures as a result.

Also, we push stacking contexts "manually" (without using StackingContextHelper) in two other places - [1] and [2]. [1] is probably fine since it's a separate pipeline but I'm not sure if you want to want local space at [2] in some cases. I guess mstange should take a look at that?

[1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/gfx/layers/wr/AsyncImagePipelineManager.cpp#321
[2] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/layout/painting/nsDisplayList.cpp#9707

::: gfx/layers/wr/StackingContextHelper.cpp:60
(Diff revision 1)
>  
> +  auto rasterSpace = wr::GlyphRasterSpace::Screen();
> +  if (aRasterizeLocally) {
> +    rasterSpace = wr::GlyphRasterSpace::Local(std::max(mScale.width, mScale.height));
> +  }
> +  mRasterizeLocally = aRasterizeLocally;

Move the mRasterizeLocally assignment to the initializer list of this function. It also needs to be initialized in the other constructor.

::: gfx/webrender_bindings/WebRenderAPI.h:509
(Diff revision 1)
>    std::unordered_map<const DisplayItemClipChain*, std::vector<wr::WrClipId>> mCacheOverride;
>  
> +  // Track how many nested animations which desire local glyph rasterization
> +  // we are currently in. If > 0, stacking contexts should default to requesting
> +  // GlyphRasterSpace::Local(1.0).
> +  uint32_t mLocallyRasterizedStackingContextDepth = 0;

nit: drop the `= 0` from here and initialize the field along with the rest of the fields at https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/gfx/webrender_bindings/WebRenderAPI.h#215

::: gfx/webrender_bindings/WebRenderAPI.cpp:804
(Diff revision 1)
> +
> +  wr::GlyphRasterSpace rasterSpace = aRasterSpace;
> +  if (aRasterSpace.IsLocal()) {
> +    mLocallyRasterizedStackingContextDepth += 1;
> +  } else if (mLocallyRasterizedStackingContextDepth > 0) {
> +    rasterSpace = wr::GlyphRasterSpace::Local(1.0); 

nit: trailing whitespace

::: gfx/webrender_bindings/WebRenderAPI.cpp:813
(Diff revision 1)
>    const size_t* maybeClipNodeId = aClipNodeId ? &aClipNodeId->id : nullptr;
>    WRDL_LOG("PushStackingContext b=%s t=%s\n", mWrState, Stringify(aBounds).c_str(),
>        aTransform ? Stringify(*aTransform).c_str() : "none");
> -  wr_dp_push_stacking_context(mWrState, aBounds, maybeClipNodeId, aAnimation, aOpacity,
> -                              maybeTransform, aTransformStyle, maybePerspective,
> -                              aMixBlendMode, aFilters.Elements(), aFilters.Length(), aIsBackfaceVisible);
> +  wr_dp_push_stacking_context(mWrState, aBounds, maybeClipNodeId, aAnimation,
> +                              aOpacity, maybeTransform, aTransformStyle,
> +                              maybePerspective, aMixBlendMode, 

nit: trailing whitespace

::: gfx/webrender_bindings/webrender_ffi_generated.h:995
(Diff revision 1)
> +extern void ClearBlobImageResources(WrIdNamespace aNamespace);
> +

FYI this will disappear when you rebase, I landed https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b58d542d97 with this change

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

https://reviewboard.mozilla.org/r/239762/#review245746

::: gfx/layers/wr/StackingContextHelper.cpp:24
(Diff revision 1)
>    , mIsPreserve3D(false)
>  {
>    // mOrigin remains at 0,0
>  }
>  
>  StackingContextHelper::StackingContextHelper(const StackingContextHelper& aParentSC,

We have access to the parent StackingContextHelper here. Instead of tracking the nestedness in the DisplayListBuilder, could we make use of the parent SC instead?

E.g. initialize mRasterizeLocally as

, mRasterizeLocally(aRasterizeLocally || aParentSC.mRasterizeLocally)

Comment 16

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239762/diff/1-2/

Comment 17

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239762/diff/2-3/

Comment 18

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239762/diff/3-4/

Comment 19

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

https://reviewboard.mozilla.org/r/239762/#review245746

> We have access to the parent StackingContextHelper here. Instead of tracking the nestedness in the DisplayListBuilder, could we make use of the parent SC instead?
> 
> E.g. initialize mRasterizeLocally as
> 
> , mRasterizeLocally(aRasterizeLocally || aParentSC.mRasterizeLocally)

Done; commit rewritten.

Comment 20

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239762/diff/4-5/

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

https://reviewboard.mozilla.org/r/239762/#review245870

Thanks! Looks a lot cleaner to me.

::: gfx/layers/wr/AsyncImagePipelineManager.cpp:331
(Diff revision 5)
>                                  wr::TransformStyle::Flat,
>                                  nullptr,
>                                  pipeline->mMixBlendMode,
>                                  nsTArray<wr::WrFilterOp>(),
> -                                true);
> +                                true,
> +                                wr::GlyphRasterSpace::Screen());

Should probably add a comment here that says that this stacking context only renders an image, no glyphs, and thus does not care about the glyph raster space.

::: layout/painting/nsDisplayList.cpp:9716
(Diff revision 5)
>  
>      // The stacking context shouldn't have any offset.
> -    layoutBounds.origin.x = 0;
> +    bounds.MoveTo(0, 0);
> -    layoutBounds.origin.y = 0;
>  
> -    aBuilder.PushStackingContext(/*aBounds: */ layoutBounds,
> +    layer = Some(StackingContextHelper(aSc,

You could use emplace here:

layer.emplace(aSc,
              aBuilder,
              ...);

Comment 22

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239762/diff/5-6/

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

https://reviewboard.mozilla.org/r/239762/#review245884

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8ad5962fc89
wire up GlyphRasterSpace to nsDisplayTransform. r=kats,mstange

Comment 25

[Noemi Erli[:noemi_erli]]

Backed out changeset f8ad5962fc89 (bug 1435094) bustage in /builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f8ad5962fc897cbec562226fb23aa361dadcfa0a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=175869853

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=175869859&repo=autoland&lineNumber=22025

Backout: https://hg.mozilla.org/integration/autoland/rev/ab26e8e09fe50465702fd4f08fee67edf16a0e6b

Comment 26

[Aria Beingessner [:Gankra]]

I am very confused that this line doesn't compile, since I built it locally several times! A mystery for tomorrow morning :|

Comment 27

[Aria Beingessner [:Gankra]]

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

Comment 28

[Aria Beingessner [:Gankra]]

better try (actually rebased over the revert): https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6512027d93d8f3fcd4c1be23caf25ad785c9284

Comment 29

[Aria Beingessner [:Gankra]]

Hrm, my local build doesn't seem to actually implement the desired behaviour after rebasing :S

wr already broke the feature..?

Comment 30

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239762/diff/6-7/

Comment 31

[Aria Beingessner [:Gankra]]

ok so did some more investigation with the try builds:

windows and mac: look good
linux: looks better but has some nasty artifacting; this is new

With this alone I'd be inclined to move forward. However there were also two regressions on windows:




REFTEST TEST-UNEXPECTED-FAIL | file:///C:/Users/task_1524850541/build/tests/reftest/tests/layout/reftests/bugs/1375674.html == file:///C:/Users/task_1524850541/build/tests/reftest/tests/layout/reftests/bugs/1375674-ref.html | image comparison, max difference: 95, number of differing pixels: 619

This seems to be testing `will-change:opacity`. For some reason this patch is making one of these cases stop using subpixel-aa, even though no masks, images, or transforms are involved. I can reproduce it locally with the try artifact.





Assertion failure: ret, at z:/build/build/src/gfx/webrender_bindings/Moz2DImageRenderer.cpp:324
Assertion failure: ret, at z:/build/build/src/gfx/webrender_bindings/Moz2DImageRenderer.cpp:324
Assertion failure: !mTarget, at z:/build/build/src/gfx/layers/wr/WebRenderLayerManager.cpp:206
TEST-UNEXPECTED-FAIL | file:///C:/Users/task_1524849120/build/tests/reftest/tests/layout/reftests/border-image/multicolor-image-1.html | application terminated with exit code 1
REFTEST PROCESS-CRASH | file:///C:/Users/task_1524849120/build/tests/reftest/tests/layout/reftests/border-image/multicolor-image-1.html | application crashed [@ static bool mozilla::wr::Moz2DRenderCallback(const class mozilla::Range<unsigned char const >, struct mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::SurfaceFormat, const unsigned short *, const struct mozilla::wr::TypedPoint2D<unsigned short,mozilla::wr::Tiles> *, const struct mozilla::wr::Type
REFTEST PROCESS-CRASH | file:///C:/Users/task_1524849120/build/tests/reftest/tests/layout/reftests/border-image/multicolor-image-1.html | application crashed [@ mozilla::layers::WebRenderLayerManager::EndEmptyTransaction(mozilla::layers::LayerManager::EndTransactionFlags)]

I can't reproduce this one.

Comment 32

[Aria Beingessner [:Gankra]]

blocked on https://github.com/servo/webrender/pull/2708

Comment 33

[Aria Beingessner [:Gankra]]

Comment on attachment 8971033 [details]
Bug 1435094 - wire up GlyphRasterSpace to nsDisplayTransform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239762/diff/7-8/

Comment 34

[Aria Beingessner [:Gankra]]

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

Comment 35

[Darkspirit]

Attachment: 2018-05-04_20-23-58.mp4

Debian Testing, KDE, Xorg, Radeon RX480, 2560x1440 (Dell U2515H)
mozregression --repo try --launch cde4401695e7915cf23788372c365e3e5752feed -B debug --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://www.qt.io/'

Much better! Thanks!
The rest might be something like bug 1458921? Focus on "A" and "D".

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65789d9edfe6
wire up GlyphRasterSpace to nsDisplayTransform. r=kats,mstange

Comment 37

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/65789d9edfe6