Open Bug 1263349 Opened 4 years ago Updated 3 years ago

When the frame visibility API is tracking which frames are in the displayport, it should clip to the critical display port

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

People

(Reporter: seth, Assigned: seth, NeedInfo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

7.86 KB, patch
Details | Diff | Splinter Review
2.21 KB, patch
Details | Diff | Splinter Review
30.84 KB, patch
Details | Diff | Splinter Review
We probably want to clip to the critical displayport when tracking frames which are logically visible in the displayport according to the frame visibility API, because not doing so will mean we lock many more images (using more memory), animate many more images, etc. I want to discuss the implications of this a little more once the dust settles on the frame visibility API changes that are happening now, though.

For efficiency purposes we probably want to make the test cheaper by passing down the nearest enclosing scrollframe on the nsDisplayListBuilder when walking the frame tree to build the display list.
Here's the patch. Passing down the enclosing scrollframe on the
nsDisplayListBuilder is pretty straightforward since we can just piggyback on
AutoCurrentScrollParentIdSetter.
Attachment #8741160 - Flags: review?(botond)
Re: the implications of doing this, I don't see any obvious reason not to. The two arguments against it are:

- We won't animate images in the low-res displayport. That's true, but since we only display low-res tiles when scrolling quickly, I think this is actually desirable. We don't want to trigger additional invalidations when we're already in a situation where we can't paint the displayport quickly enough due to the high scroll speed.

- We won't lock images in the low-res displayport. This is actually desirable (and the current image visibility code does this too), because on low memory devices we may not actually have enough memory to store all the images which are in the low-res displayport (which is often very large) simultaneously. In that case the ability to kick one image out of the SurfaceCache when we need to decode another is much preferable to decode failure or OOM.
Comment on attachment 8741160 [details] [diff] [review]
Only consider a frame IN_DISPLAYPORT visible if it's within the critical displayport.

Review of attachment 8741160 [details] [diff] [review]:
-----------------------------------------------------------------

The general approach here looks fine. I have a couple of thoughts:

  - I'm not sure how much you care about XUL documents, but those can have a
    displayport without having a root scroll frame. (That's what this code [1]
    in nsLayoutUtils::PaintFrame() handles).

    A related observation is that, except in the above case,
    nsDisplayListBuilder::mCurrentScrollParent corresponds to 
    nsDisplayListBuilder::mCurrentScrollParentId. The scroll frame can be
    recovered from the ID with nsLayoutUtils::FindScrollableFrameFor().

    So, if you do want to handle XUL documents, you could just use
    mCurrentScrollParentId and recover the scroll frame from it. In the case
    where there is no scroll frame, you can still use 
    nsLayoutUtils::FindContentFor() to get an nsIContent and get the XUL 
    document's displayport from that; for the other use of the scroll frame, 
    as  the target of the TransformRect() call, you could use the XUL 
    document's root frame.

  - That said, the above suggestion would be moving in the direction of *less*
    caching, which may not be desirable as I'm guessing 
    BuildDisplayListForChild() is a very hot code path.

    We could still handle XUL documents without doing less caching (by still
    storing mCurrentScrollParent, but handling the case where it's null).

    We could actually move in the direction of *more* caching, by storing the
    current scroll frame's display port rect in nsDisplayListBuilder, instead
    of recomputing it every time. This is just a suggestion - I'm curious to
    hear your thoughts on it.

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/layout/base/nsLayoutUtils.cpp#3386

::: layout/base/nsLayoutUtils.cpp
@@ +1171,5 @@
> +nsLayoutUtils::FrameIntersectsCriticalDisplayPort(nsIFrame* aFrame,
> +                                                  nsIScrollableFrame* aScrollableFrame)
> +{
> +  MOZ_ASSERT(aFrame);
> +  MOZ_ASSERT(aScrollableFrame);

I see this assertion failing a lot in the Try run. I guess it's because some frames get BuildDisplayListForChild() called on them before we get into BuildDisplayList() for the display root document's root scroll frame? I'm not sure how frames like that should be handled for visibility purposes, but they should be handled somehow.

@@ +1173,5 @@
> +{
> +  MOZ_ASSERT(aFrame);
> +  MOZ_ASSERT(aScrollableFrame);
> +  MOZ_ASSERT(aScrollableFrame ==
> +               GetNearestScrollableFrame(aFrame, SCROLLABLE_ALWAYS_MATCH_ROOT),

I expect this assertion will sometimes fail as well unless you also pass in SCROLLABLE_ONLY_ASYNC_SCROLLABLE.
(In reply to Botond Ballo [:botond] from comment #4)
>     So, if you do want to handle XUL documents, you could just use
>     mCurrentScrollParentId and recover the scroll frame from it. In the case
>     where there is no scroll frame, you can still use 
>     nsLayoutUtils::FindContentFor() to get an nsIContent and get the XUL 
>     document's displayport from that; for the other use of the scroll frame, 
>     as  the target of the TransformRect() call, you could use the XUL 
>     document's root frame.

Thanks for the explanation of how XUL documents should work. We track visibility in them, so we definitely need to implement a solution for them. I think I can figure things out based upon what you've told me.

>   - That said, the above suggestion would be moving in the direction of
> *less*
>     caching, which may not be desirable as I'm guessing 
>     BuildDisplayListForChild() is a very hot code path.

Yeah, I feel like maximum caching is the right thing as long as we can make it work.

>     We could actually move in the direction of *more* caching, by storing the
>     current scroll frame's display port rect in nsDisplayListBuilder, instead
>     of recomputing it every time. This is just a suggestion - I'm curious to
>     hear your thoughts on it.

I didn't think of this, but I like the idea. I'll experiment with it.

> I see this assertion failing a lot in the Try run. I guess it's because some
> frames get BuildDisplayListForChild() called on them before we get into
> BuildDisplayList() for the display root document's root scroll frame? I'm
> not sure how frames like that should be handled for visibility purposes, but
> they should be handled somehow.

That could be. I need to investigate this.

> 
> @@ +1173,5 @@
> > +{
> > +  MOZ_ASSERT(aFrame);
> > +  MOZ_ASSERT(aScrollableFrame);
> > +  MOZ_ASSERT(aScrollableFrame ==
> > +               GetNearestScrollableFrame(aFrame, SCROLLABLE_ALWAYS_MATCH_ROOT),
> 
> I expect this assertion will sometimes fail as well unless you also pass in
> SCROLLABLE_ONLY_ASYNC_SCROLLABLE.

Yeah, I have this fixed in my local version of the patch (which I should've uploaded). It still fails a lot, though. The issue I've been seeing is that when we walk down the frame tree calling BuildDisplayListForChild(), we pass through the usual sequence of frames for <html> and the like, and we hit one or more nsHTMLScrollFrame's on the way, but when we have out-of-flow frames we're walking down through the placeholder frames. When we walk back up in GetNearestScrollableFrame(), we walk through the real out-of-flow frames and never hit those nsHTMLScrollFrame's - we just end up at the viewport and terminate without hitting any scrollable frames at all. This seems sketchy to me - is this behavior intended?
(In reply to Seth Fowler [:seth] [:s2h] from comment #5)
> Yeah, I have this fixed in my local version of the patch (which I should've
> uploaded). It still fails a lot, though. The issue I've been seeing is that
> when we walk down the frame tree calling BuildDisplayListForChild(), we pass
> through the usual sequence of frames for <html> and the like, and we hit one
> or more nsHTMLScrollFrame's on the way, but when we have out-of-flow frames
> we're walking down through the placeholder frames. When we walk back up in
> GetNearestScrollableFrame(), we walk through the real out-of-flow frames and
> never hit those nsHTMLScrollFrame's - we just end up at the viewport and
> terminate without hitting any scrollable frames at all. This seems sketchy
> to me - is this behavior intended?

Assuming the out-of-flow frames are position:fixed, the SCROLLABLE_FIXEDPOS_FINDS_ROOT flag was introduced to address this. Does passing it help?
Comment on attachment 8741160 [details] [diff] [review]
Only consider a frame IN_DISPLAYPORT visible if it's within the critical displayport.

Review of attachment 8741160 [details] [diff] [review]:
-----------------------------------------------------------------

Based on comment 5, it looks like the patch is going to change a bit before landing, so I'm going to drop the review flag for now.
Attachment #8741160 - Flags: review?(botond)
Alright! I believe I've fixed all the problems here. Unfortunately there is more complexity than I initially anticipated, but I think the final result is not bad at all. Patches incoming.
In this part we're just adding some new functions to nsLayoutUtils that deal
with scrolling-related tasks. There is a convenience function to get the
displayport with fallback to the scrollport if there isn't a displayport,
there's a convenience function to transform two rects into the same space and
then intersect them, and there are multiple variations of "get the nearest async
scrollable frame".

We'll use all of these in parts 2 and 3.
Attachment #8749027 - Flags: review?(botond)
Attachment #8741160 - Attachment is obsolete: true
Alright, this part is the meat of the patch. It's very heavily commented so I'll
let the details speak for themselves, but let me make a few high level comments.

In addition to caching the current scroll parent, as in the previous version of
the patch, we are now also caching the critical displayport "considering
ancestors" and the scrollport "considering ancestors". As documented in the
comments, "considering ancestors" for the scrollport just means intersecting all
the scrollports as we go down the frame tree. For the displayport, "considering
ancestors" means that we use each new critical displayport as we go down the
tree, but *only if the new one intersects with the old one*. Note that we don't
actually take the intersection - we use the whole critical displayport, but we
essentially want to ensure that some part of each critical displayport is
visible in the ancestor critical displayport. What we're trying to capture here
is, more or less, things that APZ could scroll into view, excluding low-res
displayports.

All of that is handled in nsDisplayList.h/.cpp. The main interesting code
outside of that is in nsFrame.cpp. If we are traversing an out-of-flow frame
while building the displaylist, our scroll parent will be wrong, because it's
the scroll parent for the *placeholder* and not for the out-of-flow frame.
Imagine you have this DOM structure:

<div id="a" style="position: relative;">
  <div id="b" style="overflow: scroll; width: 100px; height: 100px;">
    <div id="c" style="width: 100px; height: 1000px;">
      <div id="d" style="position: absolute; top: -50px; left: -50px; width: 100px; height: 100px;">

Div D's placeholder frame has a scroll parent corresponding to div B, but the
out-of-flow frame itself should have water scroll parent div A has - in other
words, it *should not* scroll when we scroll div B.

To implement this, we have to walk from the out-of-frame up to the root frame
and recalculate the current scroll parent and our cached displayport and
scrollport information. You can see where we do this in nsFrame.cpp; the code
for the calculation lives in nsDisplayList.cpp/.h.

I think that covers the general ideas in the patch. Hopefully the rest is clear
from the comments!
Attachment #8749034 - Flags: review?(botond)
In part 3, which is tiny, we actually make the change that we set out to make in
this bug: we only consider a frame IN_DISPLAYPORT if it's within the critical
displayport, *considering ancestors*. If we didn't take the ancestors into
account, then even when a subtree is not considered visible because it's outside
the critical displayport, we might start considering descendants visible again
because a new scroll frame introduced a *new* critical displayport. We don't
want that. Fortunately, the displayport "considering ancestors" that we started
tracking in part 2 gives us exactly the properties we need.
Attachment #8749046 - Flags: review?(botond)
One more note: you might notice that we don't actually use the "scroll port considering ancestors" that we start tracking in these patches. Don't worry! We'll use them in my very next patch set to implement IN_VIEWPORT visibility. It seemed better to me from a review perspective to introduce these two things at the same time since they're so closely related and tied together implementation wise.
(In reply to Seth Fowler [:seth] [:s2h] from comment #10)
> Div D's placeholder frame has a scroll parent corresponding to div B, but the
> out-of-flow frame itself should have water scroll parent div A has - in other
> words, it *should not* scroll when we scroll div B.

*whatever* scroll parent div A has. Gotta love autocorrect.
Blocks: 1270389
Comment on attachment 8749027 [details] [diff] [review]
(Part 1) - Add some utility functions for dealing with scrolling-related tasks.

Review of attachment 8749027 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with extraneous hunks removed

::: layout/base/nsLayoutUtils.cpp
@@ +3500,5 @@
>      // it will be the scroll parent for display items that are built in the
>      // BuildDisplayListForStackingContext call below. We need to set the scroll
>      // parent on the display list builder while we build those items, so that they
>      // can pick up their scroll parent's id.
> +    nsIScrollableFrame* scrollFrame = nullptr;

This looks like a diff hunk from another patch.

@@ +3526,5 @@
>      }
>  #endif
>  
> +    nsDisplayListBuilder::
> +      AutoCurrentScrollParentIdSetter idSetter(&builder,

So does this.

::: layout/base/nsLayoutUtils.h
@@ +911,5 @@
>                                         nsRect& aRect);
>  
>    /**
> +   * Transforms @aFromRect from the space of @aFromFrame to the space of
> +   * @aToFrame, and intersects it with @aToRect.

nit: the parameter name references @aFromFrame and @aToFrame don't match the actual parameter names (aFrom and aTo)

@@ +2729,5 @@
>    /**
> +   * @return the nearest async scrollable ancestor frame of @aTarget, excluding
> +   * @aTarget itself.
> +   */
> +  static nsIScrollableFrame* GetAsyncScrollableAncestorFrameExcludingSelf(nsIFrame* aTarget);

I've seen the term "proper ancestor" used for this notion ("ancestor excluding self") elsewhere. Feel free to use it if you like.
Attachment #8749027 - Flags: review?(botond) → review+
Comment on attachment 8749034 [details] [diff] [review]
(Part 2) - Cache the current scroll parent and information about scrollports and displayports on nsDisplayListBuilder.

Review of attachment 8749034 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like the extraneous hunks from the last patch belong in this one; this review assumes they're included in this one.

This looks good to me, but I'd feel better if Timothy had a look as well, particularly at the parts related to out-of-flow frames. (And particular, on the effect of that change on the existing use of scroll parents.)

::: layout/base/nsDisplayList.cpp
@@ +1209,5 @@
> +  }
> +
> +  // Compute the new scroll parent ID.
> +  nsIContent* content = aScrollParent->GetContent();
> +  if (content) {

Looks to me like we should have a content whenever we have a scroll frame. We should probably assert that. Certainly, we don't want mCurrentScrollParent and mCurrentScrollParentId to become out of sync.

@@ +1278,5 @@
> +
> +void
> +nsDisplayListBuilder::UpdateCurrentScrollParentFromPathToRoot(nsIFrame* aScrollParent)
> +{
> +  AutoTArray<nsIFrame*, 100> ancestorScrollFrames;

Isn't 100 a bit excessive? (It may not be, I don't have much experience tuning these things.)

@@ +1296,5 @@
> +
> +  // Walk the ancestor scroll parents, starting from the root, and update the
> +  // current scroll parent for each of them. This will accumulate accurate
> +  // cached displayport and scrollport information.
> +  for (auto iterator = ancestorScrollFrames.rbegin();

We have fancy things like this now:

  for (const auto& scrollFrame : Reversed(ancestorScrollFrames)) {
    ...
  }

::: layout/base/nsDisplayList.h
@@ +160,5 @@
>    TRANSFORM_COMPUTATION,
>    GENERATE_GLYPH
>  };
>  
> +enum class NewScrollParentFrom : uint8_t

This deserves a comment, even if it's just "see the AutoCurrentScrollParentIdSetter constructor".

@@ +878,2 @@
>        : mBuilder(aBuilder)
> +      , mOldScrollParent(aBuilder->mCurrentScrollParent)

Maybe time to start moving these method bodies into nsDisplayList.cpp?

@@ +1224,5 @@
>    nsIFrame* FindAnimatedGeometryRootFrameFor(nsIFrame* aFrame);
>  
> +  /**
> +   * Updates cached information about the nearest ancestor scrollable frame.
> +   * @aScrollFrame should be a descendant of the previous nearest ancestor

s/@aScrollFrame/@aScrollParent?

@@ +1336,5 @@
>    // nsDisplayList class is defined below this class, so we can't use it here.
>    nsDisplayList*                 mScrollInfoItemsForHoisting;
> +  nsIFrame*                      mCurrentScrollParent;
> +  nsRect                         mDisplayPortConsideringAncestors;
> +  nsRect                         mScrollPortConsideringAncestors;

Can we put these beside mCurrentScrollParentId?
Attachment #8749034 - Flags: review?(botond)
Attachment #8749034 - Flags: review+
Attachment #8749034 - Flags: feedback?(tnikkel)
(In reply to Botond Ballo [:botond] from comment #16)
> We have fancy things like this now:
> 
>   for (const auto& scrollFrame : Reversed(ancestorScrollFrames)) {
>     ...
>   }

I guess that can be just

  for (auto scrollFrame : ...)

since the element is just a pointer.
Comment on attachment 8749046 [details] [diff] [review]
(Part 3) - Only consider a frame IN_DISPLAYPORT if it's within the critical displayport, considering ancestors.

Review of attachment 8749046 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I like how little work we're doing here now :)
Attachment #8749046 - Flags: review?(botond) → review+
Thanks for the quick reviews, Botond!

(In reply to Botond Ballo [:botond] from comment #16)
> @@ +1278,5 @@
> > +
> > +void
> > +nsDisplayListBuilder::UpdateCurrentScrollParentFromPathToRoot(nsIFrame* aScrollParent)
> > +{
> > +  AutoTArray<nsIFrame*, 100> ancestorScrollFrames;
> 
> Isn't 100 a bit excessive? (It may not be, I don't have much experience
> tuning these things.)

I used that number because nsLayoutUtils::FindNearestCommonAncestorFrame() uses it, but most similar functions in nsLayoutUtils use 32, so I'll drop it down to 32 for now. We can always bump it up later if it turns out to be a problem.
I've updated part 1 to address all of the review comments. Also, the hunks that
should've been in part 2 have been moved there.
Attachment #8749027 - Attachment is obsolete: true
This version of part 2 addresses all of the review comments so far.

The warnings on try led me to simplify the code that decides on a ViewID (now a
frame) to pass to AutoCurrentScrollParentId's constructor in
nsLayoutUtils::PaintFrame(). Matt, it'd be great if you could double check these
changes when you take a look at the patch.
Attachment #8751605 - Flags: feedback?(matt.woodrow)
Attachment #8749034 - Attachment is obsolete: true
Attachment #8749034 - Flags: feedback?(tnikkel)
Attachment #8749046 - Attachment is obsolete: true
Comment on attachment 8751605 [details] [diff] [review]
(Part 2) - Cache the current scroll parent and information about scrollports and displayports on nsDisplayListBuilder.

Review of attachment 8751605 [details] [diff] [review]:
-----------------------------------------------------------------

So, the way we've solved this in the past is by using nsDisplayListBuilder::GetOutOfFlowData.

When we initially walk down through the outer frame (that has the out-of-flow frames attached to it) we call MarkAbsoluteFramesForDisplayList -> MarkOutOfFlowFrameForDisplay and then that stores the current clip and dirty rect in the OutOfFlowDisplayDataProperty.

When we get to building the display list for this frame (after we've entered scroll frames and then jumped through our placeholder) we retrieve the cached data and temporarily override the clip and dirty rect to the correct values.

Could we store this information in that same structure?

Failing that, this approach looks good!

Could we detect if the new scroll parent is a child of the current one rather than expecting the caller to know (and then having to assert to make sure they didn't mess it up)?

It also would be nice if having multiple out-of-flow sibling frames didn't result in us having to recompute the full scroll frame ancestor tree each time. We could cache the previous scroll parent we encountered on the display list builder (as well as the stack based copy in AutoCurrentScrollParentIdSetter, so that creating another AutoCurrentScrollParentIdSetter with the same scroll parent was cheap.

::: layout/base/nsDisplayList.cpp
@@ +640,5 @@
> +{
> +  switch (aNewScrollParentFrom)
> +  {
> +    case NewScrollParentFrom::DESCENDANT_SCROLLFRAME:
> +      aBuilder->UpdateCurrentScrollParent(aScrollParent);

Can we debug assert that this is a descendant of the current one (and direct child too if applicable)?

@@ +650,5 @@
> +  }
> +
> +  // If this AutoCurrentScrollParentIdSetter has the same scrollId as the
> +  // previous one on the stack, then that means the scrollframe that
> +  // created this isn't actually scrollable and cannot participate in

Is it not scrollable, or not async scrollable?

@@ +1284,5 @@
> +  MOZ_ASSERT(mCurrentScrollParentId != FrameMetrics::NULL_SCROLL_ID,
> +             "Couldn't get a scroll ID for a scrollable frame?");
> +
> +  // If we didn't have a scroll parent before, but we do now, the displayport
> +  // and scrollport we're tracking are just those of the new scroll parent. 

Trailing whitespace :)

::: layout/base/nsDisplayList.h
@@ +867,5 @@
> +    /**
> +     * Temporarily update the cached scroll parent information on @aBuilder
> +     * from frame @aFrame.
> +     *
> +     * If @aNewScrollParentFrom is DESCENDANT_SCROLLFRAME, @aFrame is

Doesn't it need to be a direct child scrollframe of the current one, not an arbitrary descendant (like a grand-child)?

@@ +871,5 @@
> +     * If @aNewScrollParentFrom is DESCENDANT_SCROLLFRAME, @aFrame is
> +     * interpreted as a scrollframe that is a descendant of @aBuilder's
> +     * current scrollparent.
> +     *
> +     * If @aNewScrollParentFrom is SCROLLFRAMES_ON_PATH_TO_ROOT, @aFrame is

I'd personally prefer 'UNKNOWN_SCROLLFRAME' as a name for 'from', the current name is more of a description of what we need to do to handle that. It's probably not a big deal though.
Attachment #8751605 - Flags: feedback?(matt.woodrow)
OK, this version of part 2 addresses all of Matt's concerns. Most importantly,
we now cache the information about scrollports and displayports on OutOfFlowData
so we can trivially restore it when we get to the placeholder frame without
needing to walk up the tree. This should improve performance, and the resulting
code is simpler to boot.
Attachment #8752021 - Flags: review?(mstange)
Attachment #8751605 - Attachment is obsolete: true
Markus, I think you looked at Part 2 already, so I'll just note that the only substantial changes are to the way the constructors work in AutoCurrentScrollParentIdSetter, the addition of UpdateCurrentScrollParentForOutOfFlow(), the alterations to OutOfFlowData, and the code in nsIFrame::BuildDisplayListForChild() which uses AutoCurrentScrollParentIdSetter.
Attachment #8752021 - Flags: review?(mstange) → review+
Fixed a bad assert. When asserting that the old scroll parent in
nsDisplayListBuilder::UpdateScrollParent() is the nearest ancestor scrollable
frame of the new scroll parent, we need to allow for the fact that in some
circumstances we fall back to the root frame for a document, and the root frame
may not necessarily be a scrollframe. I've updated the assertion to allow for
both of these cases. This fixes the remaining issues on try.
Attachment #8752021 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8d93926a960ccec2d9e821c6e92005d1b6ee04
Bug 1263349 (Part 1) - Add some utility functions for dealing with scrolling-related tasks. r=botond

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cfdd1649464cf1b80de560ca972c8ed1633b251
Bug 1263349 (Part 2) - Cache the current scroll parent and information about scrollports and displayports on nsDisplayListBuilder. r=botond,mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae4cfe35d7157c4c0d3bc1b39224053e5cd84ba
Bug 1263349 (Part 3) - Only consider a frame IN_DISPLAYPORT if it's within the critical displayport, considering ancestors. r=botond
Hi, seems this caused crashes on android like https://treeherder.mozilla.org/logviewer.html#?job_id=28530854&repo=mozilla-inbound
Flags: needinfo?(seth)
(In reply to Carsten Book [:Tomcat] from comment #32)
> Hi, seems this caused crashes on android like
> https://treeherder.mozilla.org/logviewer.html#?job_id=28530854&repo=mozilla-
> inbound

pressed sent to fast :) also had to back this out because of the crashes
You need to log in before you can comment on or make changes to this bug.