Closed Bug 1435094 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- disabled
firefox61 --- fixed

People

(Reporter: Zaggy1024, Assigned: Gankra)

References

(Blocks 2 open bugs, )

Details

(Keywords: correctness, nightly-community)

Attachments

(3 files)

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.
Thank you! Confirmed in Nightly 60 x64 20180201100326 de_DE @ Debian Testing (KDE, Radeon RX480).
fresh profile: gfx.webrender.all
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: Untriaged → Graphics: WebRender
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Summary: [webrender] Characters of text in animated boxes on qt.io skip pixels rather than animating smoothly → Characters of text in animated boxes on qt.io skip pixels rather than animating smoothly
Version: 60 Branch → Trunk
Attached video 2018-02-02_01-07-00.mp4
Seems to be a general problem affecting all platforms, so probably something in our platform-independent code.
Assignee: nobody → a.beingessner
This seems to have been fixed somehow? Can anyone else still reproduce?
Oh no wait nevermind, I was looking at text that was part of the image, I think.
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?
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
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.
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?
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 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
Attachment #8971033 - Flags: review?(bugmail) → review+
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 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 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,
              ...);
Attachment #8971033 - Flags: review?(mstange) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f8ad5962fc89
wire up GlyphRasterSpace to nsDisplayTransform. r=kats,mstange
Keywords: checkin-needed
I am very confused that this line doesn't compile, since I built it locally several times! A mystery for tomorrow morning :|
Flags: needinfo?(ryanvm)
Hrm, my local build doesn't seem to actually implement the desired behaviour after rebasing :S

wr already broke the feature..?
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.
Depends on: 1457891
Attached video 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".
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65789d9edfe6
wire up GlyphRasterSpace to nsDisplayTransform. r=kats,mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65789d9edfe6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
QA Whiteboard: [good first verify]
Depends on: 1520176
You need to log in before you can comment on or make changes to this bug.