1635406 - Font rendering issue caused by webrender change

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Ali]

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

Steps to reproduce:

Visited this page and clicked on "In stock at PB Tech"
https://www.pbtech.co.nz/product/CPUAMD03600/AMD-Ryzen-5-3600-6-Core12-Threads-up-to-42-GHz-Max

System specs:
i5 2400
8gb
GTX 970 - 445.87
Win 10 18363.815

Actual results:

Font was rendered incorrectly.
http://puu.sh/FGwe3/d92ecf1a1c.png

Expected results:

Correct font rendering, as was the case in 75.

I have used mozregression and found the change which caused the issue.
https://pastebin.com/BDd8mTQC

Confirmed the issue persisted after a Firefox refresh, but is corrected with hardware acceleration disabled.

Comment 1

[Ali]

about:support details:
https://pastebin.com/WytRqam1

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 4

[Firefox Bug Husbandry Bot]

Because this bug's Severity is normal and has not been changed, and this bug's priority is -- (none,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Comment 5

[Jamie Nicol [:jnicol]]

Thanks for the report, Ali.

I can reproduce this. It looks a lot more subtle on my computer, but by opening the devtools and toggling the overflow-y: auto; on the <div class=storeScrollArea> element it becomes pretty clear.

What I have figured out so far, is that in update_transform() for the scrollable list of stores, state.coordinate_system_relative_scale_offset.y ends in .5. As of bug 1620014, we now snap that here, whereas previously it was not being snapped.

state.coordinate_system_relative_scale_offset comes from the parent node's content_transform, which is the combination of a few different things, but the .5 is coming from here. (All the other contributing offsets are whole numbers, and there is no scale.)

info.source_transform is in this case a PropertyBinding::Value rather than a PropertyBinding::Binding, so I guess the value is coming from the display list gecko sends?

Andrew, any ideas what the problem is here? The intention of bug 1620014 was to snap animated tranforms (specifically scrolling, but css animations would probably be okay too), not non-animated ones. Is it surprising that we get this result when we snap non-animated transforms?

Comment 6

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

So, there are many things going on with scrolling, but presumably when you say animated scrolling transforms, you are referring to the sampling we do in APZCTreeManager::SampleForWebRender. Ultimately they all boil down to a dynamic transform via a PropertyBinding::Binding. Only ReferenceFrame spatial nodes can have an explicit transform. Hopefully I got this right.

The patch referenced from bug 1620014 changes the behaviour of non-ReferenceFrame spatial nodes -- specifically StickyFrame and ScrollFrame. They cannot be directly animated. But they may have an ancestor ReferenceFrame that is animated -- I presume what we actually want to make a distinction about. I didn't actually realize this was an important distinction when I did the review :).

We don't track if a spatial node has an ancestor which is animated, but that is fairly easy to do. Add a new bool to SpatialNode, is_ancestor_or_self_animated, defaulting to false. In SpatialTree::add_reference_frame, set node.is_ancestor_or_self_animated to true if the transform is PropertyBinding::Binding. In SpatialTree::add_spatial_node, if the node has a parent, we need to || in the parent's animated value. Finally then in SpatialNode::update_transform, you can snap/not snap the Sticky/ScrollFrame offsets based on that.

You are correct that this will also apply to CSS animations. But that should be fairly easy to work around if need by. We already mark the async zoom animations specially, and we can add a similar property for the other types too. And then only do this for zoom animations specifically. I don't really know what the right thing to do in that case is. I assume CSS animations can benefit from the same optimization / better picture caching behaviour.

Comment 7

[Jamie Nicol [:jnicol]]

Tracking whether the ancestor is animated, and only snapping if it is seems reasonable to me. As the problem in this case is that the ancestor is not animated, yet has a fractional offset transform, meaning that we are snapping it incorrectly.

I wonder though, would it not make more sense to snap the transform when we sample it? Do we actually only want to apply the snapping for sticky and scroll frames, or should child reference frames not be snapped too?

My hunch would be that yes CSS animations would also benefit from being snapped in the same way. So no point making a distinction now, and we can always re-evaluate later.

Comment 8

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

I don't think we can snap the transform in the APZ code. You raise a good point though -- you may have success snapping in SpatialNode::update_transform for only scroll frames.

https://searchfox.org/mozilla-central/rev/4166c15e2a99a23a9b38ad62c9fdfe8e5448b354/gfx/wr/webrender/src/spatial_node.rs#371

Mark the reference frames as for scrolling, and if so, and snap the accumulated scale/offset here and remove the snapping of the scale/offset in the scroll/sticky frame path. Now that I am looking at the code again, it would actually fix an inconsistency between the reference frame and the scroll/sticky frames, which may on its own resolve the overly aggressive clipping. Since the snapping is moving to the reference frame, you don't need to know if your ancestor is animated, you can just check the property binding here...

Comment 9

[Pascal Chevrel:pascalc]

S3 and we ship our last beta this Friday before RC week, marking as wontfix for our 77 release.

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Bug 1635406 - Snap reference frame transforms if animated or zooms. r?aosmond

Bug 1620014 attempted to fix an issue where an animated visual
viewport offset (eg due to scrolling while being zoomed in) was
causing the fractional offset of a descendant scroll frame's content
transform to change, causing too much picture cache invalidation.

It did so by snapping the coordinate-system-relative offset when using
it to calculate the content_transform. This value of course includes
the animated visual viewport offset (as the axis-aligned zoom
transform cannot reset the coordinate system). However, it also
includes non-animated offsets, which were now being incorrectly
snapped, causing blurry/clipped text.

This change reverts that original fix. And instead, it snaps the
source_transform of the reference frame itself when it is sampled,
rather than the accumulated coordinate-system-relative scale_offset of
the scroll frame. Additionally, it only snaps the offset if it is an
animation (including zoom), and static offsets are left unsnapped.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8499f9c0d02
Snap reference frame transforms if animated or zooms. r=aosmond

Comment 12

[Razvan Maries]

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