1002992 - use a bare frame tree walker for image visibility

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Building a display list for image visibility is a bit overkill. A bare frame tree walker should be just as good but much faster.

The attached pass needs a little more work and lots more testing but preliminary numbers say its about 3-4x faster.

Comment 1

[Timothy Nikkel (:tnikkel)]

Preserve3d needs a little special handling that hasn't been written just yet.

Comment 2

[Mike Lee [:mlee]]

Setting as a 2.0 blocker per Jet's bug 981899 comment 29:

    https://bugzilla.mozilla.org/show_bug.cgi?id=981899#c29

and blocking bug 982210 and bug 986681 per Milan's bug 981899 comment 31:

    https://bugzilla.mozilla.org/show_bug.cgi?id=981899#c31

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: wip

check point, more fixes

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: get the subdocument the same way as display list building

In order to compare the two approaches we need to get the subdoc the same way as display list building does.

Comment 5

[bhavana bajaj [:bajaj]]

Can you please help with the user impact here?

Comment 6

[Timothy Nikkel (:tnikkel)]

It will make the update image visibility about 4 times faster. On the contacts app scrolling on b2g update image visibility was about 3% of non-idle time. So it will improve that case by about 2% overall. I have no reason to believe that case is not representative.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Skip text-overflow processing of display lists for image visibility

I found it indispensable to be able to compare the results of the display list method and the new frame walker method in ironing out the bugs of the new frame walker. Furthermore I'd like to keep around the display list code to use to ensure that the results will continue to get the same results (ie if a new feature is added or painting is changed). I have a patch which automatically checks that they get the same result. The display list code solely for image visibility is relatively small and self contained.

I have a couple of patches to display list portion that make it easier to get the same results. This is the first. We just skip the text-overflow clipping, because in cases where it clips out images it would be somewhat more complicated to make the frame walker handle that. Making images clipped by text-overflow visible seems fine to me.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Update image visibility display list building code to be in sync with painting code for display ports.

This just syncs up some more recent changes to how we handle display ports in painting.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Don't build the caret for the display list for image visibility

If the caret is in an image frame, the caret display item will have underlying frame as the image frame. If the image frame is 0x0 then we won't build a display item for it otherwise, and the framewalker will not consider it visible.

This just disables the caret. There's no benefit. The caret being in an image frame makes no difference as to if the image is visible or not.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Factor out presshell getter for painting subdocs.

Turns out we need to get the subdoc to descend in to the same way as display list building does, otherwise we'll go into a different one. This only happens during page navigation. Otherwise we only have one document to choose from.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: Frame walker.

I think this should handle all properties of painting. Out of flow frames are much simpler because we don't have to worry about painting order so we can just traverse them directly when we come across them on Absolute and Fixed lists instead of marking them at that point and then descending into them when we follow their placeholders.

Anything I missed?

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8433187 [details] [diff] [review]
Skip text-overflow processing of display lists for image visibility

Don't forget to add the bug number to the commit message.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8433190 [details] [diff] [review]
Update image visibility display list building code to be in sync with painting code for display ports.

I think we need a more descriptive name for "ExpandRect".
It was sort of fine until now since it was only an internal
helper method, but I think the name is too generic for
a public interface.

I'd suggest something like ExpandRectToNearlyVisible or
ExpandToNearlyVisibleRect, to associate it with
IsRectNearlyVisible.  Renaming both methods to have
"ForImageVisibility" in the name might be an alternative.

(sorry for bike shedding)

Other than that, this patch looks fine.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8433191 [details] [diff] [review]
Don't build the caret for the display list for image visibility

There's a typo in the commit message: s/carey/caret/

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8433192 [details] [diff] [review]
Factor out presshell getter for painting subdocs.

>layout/generic/nsSubDocumentFrame.cpp
>+nsSubDocumentFrame::GetSubdocumentPresShellForPainting(uint32_t aFlags)
> ...
>+      if (!presShell)
>+        return nullptr;
>+    }
>+  }
>+
>+  return presShell;

The if-statement above can be removed.

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8433194 [details] [diff] [review]
Frame walker.

>layout/base/nsPresShell.cpp
>+PresShell::MarkImagesInSubtreeVisible(nsIFrame* aFrame, const nsRect& aRect)
>+{
>+  MOZ_ASSERT(aFrame->PresContext()->PresShell() == this, "wrong presshell");
>+
>+  nsCOMPtr<nsIImageLoadingContent> content(do_QueryInterface(aFrame->GetContent()));
>+  if (content && aFrame->StyleVisibility()->IsVisible()) {
>+    uint32_t count = mVisibleImages.Count();
>+    mVisibleImages.PutEntry(content);
>+    if (mVisibleImages.Count() > count) {
>+      // content was added to mVisibleImages, so we need to increment its visible count
>+      content->IncrementVisibleCount();
>+    }
>+  }

Can we return early in the outer if-statement above?
It looks like the rest of this method is a NOP for images.

>+      if (root) {
>+        rect = rect + aFrame->GetOffsetToCrossDoc(root);
>+      } else {
>+        rect = rect - aFrame->GetContentRectRelativeToSelf().TopLeft();
>+      }

I would prefer "rect.MoveBy(aFrame->GetOffsetToCrossDoc(root))".
It's clearer and it avoids assigning width/height.
(same for the else-branch)

>+    rect = rect.Intersect(scrollFrame->GetScrollPortRect());
>+    nsRect displayPort;
>+    bool usingDisplayport = nsLayoutUtils::GetDisplayPort(aFrame->GetContent(), &displayPort);
>+    if (usingDisplayport) {
>+      rect = displayPort;
>+    }

The value of first assignment above is not used when 'usingDisplayport'
is true, so it should be moved into an else-branch.

>+PresShell::RebuildImageVisibility(nsRect* aRect)
> ...
>+  for (uint32_t i = 0; i < beforeImageList.Length(); ++i) {

s/uint32_t/size_t/

>+#ifdef IMAGE_VISIBILITY_DISPLAY_LIST

Perhaps add a DEBUG_ prefix to the name to make it clear it's
not a configure option or something.  Also add a comment on how
one can use it for debugging?

Comment 17

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tn) from comment #11)
> Anything I missed?

Nope, it should be fine since we're traversing the whole frame tree
from the root.

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #12)
> Don't forget to add the bug number to the commit message.

Of course.

(In reply to Mats Palmgren (:mats) from comment #13)
> I'd suggest something like ExpandRectToNearlyVisible or
> ExpandToNearlyVisibleRect, to associate it with
> IsRectNearlyVisible.  Renaming both methods to have
> "ForImageVisibility" in the name might be an alternative.

Good idea. I added a patch to the end to rename ExpandRect -> ExpandRectToNearlyVisible.

(In reply to Mats Palmgren (:mats) from comment #14)
> There's a typo in the commit message: s/carey/caret/

Thanks.

(In reply to Mats Palmgren (:mats) from comment #15)
> The if-statement above can be removed.

Done.

(In reply to Mats Palmgren (:mats) from comment #16)
> Can we return early in the outer if-statement above?
> It looks like the rest of this method is a NOP for images.

nsImageLoadingContent is also the parent class of nsObjectLoadingContent, so we can still have child frames here. For example I came across the plugin block ui for an applet in the subtree of a frame whose content QI'd to nsImageLoadingContent.

> I would prefer "rect.MoveBy(aFrame->GetOffsetToCrossDoc(root))".
> It's clearer and it avoids assigning width/height.
> (same for the else-branch)

Done.

> The value of first assignment above is not used when 'usingDisplayport'
> is true, so it should be moved into an else-branch.

Done.


> s/uint32_t/size_t/

Done. Also another instance.

> >+#ifdef IMAGE_VISIBILITY_DISPLAY_LIST
> 
> Perhaps add a DEBUG_ prefix to the name to make it clear it's
> not a configure option or something.  Also add a comment on how
> one can use it for debugging?

Done.

Comment 19

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/397d00aa598f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fabb6ff29cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a95c4223dba7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3254fa7b1c51
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce548c7d8fa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/47fcf180a0b9

Comment 20

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/397d00aa598f
https://hg.mozilla.org/mozilla-central/rev/1fabb6ff29cd
https://hg.mozilla.org/mozilla-central/rev/a95c4223dba7
https://hg.mozilla.org/mozilla-central/rev/3254fa7b1c51
https://hg.mozilla.org/mozilla-central/rev/ce548c7d8fa2
https://hg.mozilla.org/mozilla-central/rev/47fcf180a0b9