1669865 - main thread paint causes picture caching to be invalidated

Status: RESOLVED FIXED

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

Description

[Timothy Nikkel (:tnikkel)]

This comes from bug 1669625. Scroll the url testcase from that bug with debug picture caching enabled and change your full zoom (app units per dev pixel ratio). Change the aRepaint argument of SetVisualViewportOffset of this call

https://searchfox.org/mozilla-central/rev/f82d5c549f046cb64ce5602bfd894b7ae807c8f8/gfx/layers/apz/util/APZCCallbackHelper.cpp#175

to true. You should then be able to reproduce. A main thread paint shouldn't invalidate all picture caches during scrolling.

Comment 1

[Timothy Nikkel (:tnikkel)]

Alternate steps to reproduce: add a SchedulePaint call in PresShell::SetVisualViewportOffset.

Comment 2

[Timothy Nikkel (:tnikkel)]

This means that we are very likely throwing away the picture cache in many situations for no reason.

Comment 3

[Timothy Nikkel (:tnikkel)]

And I do not believe this is fixed by the patch in bug 1642308.

Comment 4

[Glenn Watson [:gw]]

Even with the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1642308 applied, and without any of the local changes listed above, I still see lots of invalidation scrolling on pages when zoomed in. I'll investigate that as a start point - it may be that the SchedulePaint call just makes the same underlying issue more likely to occur.

Comment 5

[Timothy Nikkel (:tnikkel)]

That's because you don't have the fix for bug 1669625 in your tree yet (I've triggered it to land). Once that is in your tree you'll need to take some of the steps above to reproduce.

Comment 6

[Glenn Watson [:gw]]

I found the primary issue causing these extra invalidations when scrolling while zoomed in.

The external scroll offsets supplied in the spatial nodes are sometimes not quite a round number - it's common to see Gecko switch between 60.0 and 59.99998 on different display lists, for example.

This external offset is applied to normalize the position of glyphs that are supplied by Gecko for interning / hashing purposes (ideally Gecko would already supply glyphs without the external scroll offset, that's a long story for another time!).

Two questions:

• In the short term, is it reasonable to round or quantize this external scroll offset value? Is it ever expected to be fractional and if so, is there an expected accuracy we can quantize it to? I confirmed in my local tests that just applying round() to that external scroll offset seems to fix all the extra invalidation logic.

• In the longer term, perhaps this API (and others) should be moved to natively supply app-units in the display list. Would that make sense from a Gecko perspective?

Comment 7

[Glenn Watson [:gw]]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)

That's because you don't have the fix for bug 1669625 in your tree yet (I've triggered it to land). Once that is in your tree you'll need to take some of the steps above to reproduce.

Ah, that makes sense. I haven't confirmed but I think the explanation above in comment 6 will explain what is happening here.

When a main thread paint is triggered, that causes the external scroll offset to be updated (I believe). In turn, if the external scroll offset is not quite accurate in these zoomed in scenarios, the main thread scheduled paint would result in invalidations in this scenario.

Comment 8

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

The quick summary is: yes, I think round()ing the external scroll offset should be ok.

The longer version: the external scroll offset is computed here - the layout scroll scroll offset on the metrics is computed here during the scroll metadata computation here. So basically we're taking scrollableFrame->GetPosition(), converting that to CSS pixels, and then converting that to LayoutDevice pixels. The key thing to note here is that scrollableFrame->GetPosition() itself is supposed to always be a multiple of LayoutDevice pixels. So if we are getting an external scroll offset that is slightly off from an integer value, it's almost certainly due to floating point error during the conversion from app units to CSS pixels and then to LayoutDevice pixels.

In terms of possible fixes:

1. We could just apply a rounding to this quantity
2. Instead of doing the CSS-to-LD conversion there, we can read scrollableFrame->GetPosition() directly there, which gives us the value in app units and is not subject to rounding. Or we can convert it directly from app units to LayoutDevice pixels using LayoutDeviceIntPoint::FromAppUnitsRounded which will take care of the rounding for us. This is similar to what we are doing for the clip bounds a few lines up.
3. Do both 1 and 2 and MOZ_ASSERT they give the same result

Comment 9

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

Oh actually I think there might be cases where if the scrolled content itself is not an integer number of device pixels then it's possible to end up at a scroll offset that's not an integer value in device pixels. So maybe just using the appunit value directly is best.

Comment 10

[Glenn Watson [:gw]]

scrollableFrame in that context doesn't have a method called GetPosition. Is there a different method I should be using, or calling on a different object?

Comment 11

[Glenn Watson [:gw]]

botond suggested I should be using GetScrollPosition which seems to be working.

Comment 12

[Glenn Watson [:gw]]

Attachment: Bug 1669865 - Fix invalidations from external scroll offset inaccuracy.

Ensure that the external scroll offset is rounded from app units
into whole device pixels. This means that the primitive content id
matches in cases with non-round device pixel ratios, avoiding
unnecessary invalidations in these cases.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/265e51a04c52
Fix invalidations from external scroll offset inaccuracy. r=kats

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/265e51a04c52