Closed Bug 1002992 Opened 5 years ago Closed 5 years ago

use a bare frame tree walker for image visibility

Categories

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

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
feature-b2g 2.0

People

(Reporter: tnikkel, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

Attached patch wip (obsolete) — Splinter Review
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.
Preserve3d needs a little special handling that hasn't been written just yet.
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
Blocks: 982210, 986681
Status: NEW → ASSIGNED
blocking-b2g: --- → 2.0+
Attached patch wip (obsolete) — Splinter Review
check point, more fixes
Attachment #8414256 - Attachment is obsolete: true
In order to compare the two approaches we need to get the subdoc the same way as display list building does.
Can you please help with the user impact here?
Flags: needinfo?(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.
Flags: needinfo?(tnikkel)
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.
Attachment #8424210 - Attachment is obsolete: true
Attachment #8424212 - Attachment is obsolete: true
Attachment #8433187 - Flags: review?(matspal)
Attachment #8433187 - Attachment description: textoverflowimgvis → Skip text-overflow processing of display lists for image visibility
This just syncs up some more recent changes to how we handle display ports in painting.
Attachment #8433190 - Flags: review?(matspal)
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.
Attachment #8433191 - Flags: review?(matspal)
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.
Attachment #8433192 - Flags: review?(matspal)
Attached patch Frame walker.Splinter Review
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?
Attachment #8433194 - Flags: review?(matspal)
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.
Attachment #8433187 - Flags: review?(matspal) → review+
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.
Attachment #8433190 - Flags: review?(matspal) → review+
Attachment #8433191 - Flags: review?(matspal) → review+
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 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.
Attachment #8433192 - Flags: review?(matspal) → review+
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?
Attachment #8433194 - Flags: review?(matspal) → review+
(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.
blocking-b2g: 2.0+ → ---
feature-b2g: --- → 2.0
(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.
Depends on: 1070775
Depends on: 1241371
Depends on: 1350463
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.