Open Bug 1259593 Opened 4 years ago Updated 3 years ago

Displayport base rect can get clipped to empty

Categories

(Core :: Panning and Zooming, defect, P5)

48 Branch
defect

Tracking

()

REOPENED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fix-optional

People

(Reporter: kats, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

Attached file IRC conversation
If you go to https://github.com/mozilla/gecko-dev/blob/master/layout/base/FrameLayerBuilder.cpp, scroll the inner subframe so it layerizes, and then scroll the outer page fast, you can end up in a state where the inner frame gets an empty displayport base rect. It's transient but undesirable, as it can result in unnecessary checkerboarding.

I had a discussion with tnikkel about this (attached). We discovered that we get into this situation when the displayport we're painting doesn't intersect the main-thread's notion of what's visible. This can happen (currently) under two conditions: one is that velocity bias pushes the displayport out in the direction of scrolling so far that it no longer intersects the scroll position. The other is that the peek-messages code gets a future displayport that is far ahead of the current scroll position.

In both of these cases, the intersection of the dirty rect, scroll port, and transformed root composition bounds ends up empty, because the dirty rect (which derives from the displayport) uses these "future scroll positions" whereas the transformed root composition bounds does not.

Some possible approaches to fixing this are discussed at the end of the IRC conversation.
Just to add one last thought to the simple solution from the log.

Instead of transforming the root comp bounds and intersecting with displayPortBase, we should transform the root comp bounds, then limit displayPortBase to the same width/height of the transform root comp bounds, taking an equal amount from the top and bottom/left and right.
Assignee: nobody → bugmail.mozilla
Attached patch Patch (obsolete) — Splinter Review
Attachment #8736393 - Flags: review?(tnikkel)
Attachment #8736393 - Flags: review?(tnikkel) → review+
Same patch, with r=tnikkel
Attachment #8736393 - Attachment is obsolete: true
Attachment #8736881 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/abe2031a710e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I think patch actually made things worse somehow, leading to issues like bug 1259593. I'm going to back it out.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> I think patch actually made things worse somehow, leading to issues like bug
> 1259593. I'm going to back it out.

This should say "... like bug 1262151"
After thinking about this some more I think we definitely need to intersect mScrollPort with the transform root composition bounds. If we don't do that, then we don't account for the size change that occurs as the subframe moves out of the visible portion of the parent (e.g. scrolling down from y=0 to y=100 on [1] should increase the height of the subframe's base rect by 100).

Right now the end result that we compute is the intersection of the above with the dirty rect, which has the "future scroll position" applied. I think that intersection doesn't make sense because we are intersecting things with temporally different scroll positions; instead we want to do a MoveInsideAndClamp.

IOW aDirtyRect->MoveInsideAndClamp(mScrollPort.Intersect(rootCompBounds)) seems to fix this issue for me and as far as I can tell doesn't incur any badness.

[1] https://github.com/mozilla/gecko-dev/blob/master/layout/base/FrameLayerBuilder.cpp
That being said, I'm not confident that it doesn't incur any badness. It's true that this would result in the base rect including parts of the dirty rect that it wasn't before, so it's quite possible that we'll do something bad.

I'm going to let this bug sit for a while since there's no user-visible impact of it as far as I can tell, it's just something I noticed that results in inefficient behaviour.
Priority: -- → P5
Assignee: bugmail.mozilla → nobody
No longer blocks: 1280013
See Also: → 1280013
You need to log in before you can comment on or make changes to this bug.