Add Visibility::IN_VIEWPORT to the frame visibility API

NEW
Assigned to

Status

()

Core
Layout
2 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

7.58 KB, patch
Details | Diff | Splinter Review
24.11 KB, patch
Details | Diff | Splinter Review
4.54 KB, patch
Details | Diff | Splinter Review
9.64 KB, patch
Details | Diff | Splinter Review
6.62 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
We want to get notified when frames enter or leave the viewport. This would allow us to do things like pause image animation when the image is not visible in the viewport, or throttle requestAnimationFrame in offscreen iframes which aren't visible. Currently we do both of those things, but we consider the displayport (or in the case of images, an even larger area) rather than the viewport. Constraining the work we do to things which are actually visible will help us reduce our energy and memory usage.
(Assignee)

Comment 1

2 years ago
Created attachment 8749060 [details] [diff] [review]
(Part 1a) - Add Visibility::IN_VIEWPORT to the frame visibility API.

OK, let's get this thing moving. A lot of this is quite similar to the
IN_DISPLAYPORT patch that landed quite recently, so I'll be a little bit less
verbose with my explanations. This patch adds IN_VIEWPORT to the Visibility enum
and exposes it on nsIFrame's frame visibility API.

(It's worth noting by the way that parts 1a-1d will be folded together before landing.)
Attachment #8749060 - Flags: review?(mstange)
(Assignee)

Comment 2

2 years ago
Created attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

This patch makes a lot of boilerplate changes to the pres shell code to support
IN_VIEWPORT visibility, but there's also some meat here.
UpdateInViewportFrameVisibilitySync() is the code that gets called from the
refresh driver when there's an APZ scroll event, but we don't paint. Just like
IN_DISPLAYPORT visibility, if we paint we get IN_VIEWPORT visibility more or
less for free - you'll see in an upcoming patch - so we don't need to call
UpdateInViewportFrameVisibilitySync() in that case. The basic ideas for
UpdateInViewportFrameVisibilitySync() are:

1. We only need to consider frames that are within the displayport, since we're
   only considering APZ scroll events that don't trigger a paint, and hence
   cannot have moved the displayport.

2. To tell if a frame is within the viewport, we just need to walk up all its
   scroll frame ancestors to the root of the frame tree and make sure that the
   intersection of all their scrollports with the frame's rect is nonempty.

3. To include <iframe>'s and the like in these calculations, we need to walk
   down the view tree and do this update in child pres shells as well.

All of that is implemented in this patch.
Attachment #8749064 - Flags: review?(mstange)
Attachment #8749064 - Flags: review?(botond)
(Assignee)

Comment 3

2 years ago
Created attachment 8749065 [details] [diff] [review]
(Part 1c) - Implement Visibility::IN_VIEWPORT.

This part actually implements Visibility::IN_VIEWPORT. It's simple once we have
the infrastructure from the previous parts in place. During painting, if a frame
is in the displayport we check if it's also in the viewport by checking the
cached ancestor scrollport information that we started tracking in bug 1263349.
When we get an APZ scroll event that doesn't trigger a paint, we tell the
refresh driver to call UpdateInViewportFrameVisibilitySync() on the next tick.
That's really it!
Attachment #8749065 - Flags: review?(mstange)
Attachment #8749065 - Flags: review?(botond)
(Assignee)

Comment 4

2 years ago
Created attachment 8749066 [details] [diff] [review]
(Part 1d) - Update various frame classes to use IN_VIEWPORT visibility.

This patch updates the various frame classes to use IN_VIEWPORT visibility. In
all but a few cases, this just boils down to using IN_VIEWPORT visibility
instead of IN_DISPLAYPORT visibile when we force a frame to be visible, because
when we do that we always want to use the "most visible" type of visibility.
Attachment #8749066 - Flags: review?(mstange)
(Assignee)

Comment 5

2 years ago
Created attachment 8749067 [details] [diff] [review]
(Part 2) - Add visualization of IN_VIEWPORT visibility to the APZ minimap.

Finally, we want to visualize IN_VIEWPORT visibility on the APZ minimap
visibility visualization. (Say that three times fast!)
Attachment #8749067 - Flags: review?(botond)
(Assignee)

Updated

2 years ago
Blocks: 1270394
Attachment #8749060 - Flags: review?(mstange) → review+
Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

This looks good except for the part that walks the presshell tree. I'd really prefer that to use EnumerateSubDocuments instead. And then you won't need the comment about the view tree structure - different documents are guaranteed to have different presshells.
(Assignee)

Comment 7

2 years ago
(In reply to Markus Stange [:mstange] from comment #6)
> This looks good except for the part that walks the presshell tree. I'd
> really prefer that to use EnumerateSubDocuments instead. And then you won't
> need the comment about the view tree structure - different documents are
> guaranteed to have different presshells.

Doh! I didn't know about EnumerateSubDocuments. That does seem perfect. I'll update the patch.
Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

::: layout/base/nsPresShell.h
@@ +821,4 @@
>      bool mSuppressingVisibility;
>    };
>  
>    VisibleFramesContainer mVisibleFrames;

Does this patch have a code dependency that's not on m-c and isn't marked as a dependency of this bug? I can't find the change that added this variable, for example.
(In reply to Botond Ballo [:botond] from comment #8)
> Does this patch have a code dependency that's not on m-c and isn't marked as
> a dependency of this bug? I can't find the change that added this variable,
> for example.

Looks like the missing dependency is bug 1259281 (and the things it depends on in turn); marking it.
Depends on: 1259281
Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

I'm not really familiar enough with this code to r+ this patch, but I've looked it over and it looks sensible to me, so I'll f+ it.

::: layout/base/nsPresShell.cpp
@@ +4628,5 @@
>                            mVisibleRegions->mInDisplayPort,
>                            VisibilityCounter::IN_DISPLAYPORT,
>                            layersId, presShellId);
> +
> +  SendUpdateVisibleRegion(compositorChild,

Really nit-picking here, but in a state where the Part 1 patches are applied but the Part 2 patch isn't, this would overwrite the compositor's IN_DISPLAYPORT regions with the IN_VIEWPORT regions, so technically, the Part 2 patch should be folded in with these as well.

@@ +5920,5 @@
>            mOldInDisplayPortFrames.emplace();
>            mPresShell->mVisibleFrames.mInDisplayPort.SwapElements(*mOldInDisplayPortFrames);
>            break;
> +
> +        case VisibilityCounter::IN_VIEWPORT:

Do you need to add an |mPresShell->mVisibleFrames.mInViewport.Clear()| line to the IsVisibilitySuppressed() block above?

::: layout/base/nsRefreshDriver.cpp
@@ +1928,5 @@
>  #ifndef ANDROID  /* bug 1142079 */
>    mozilla::Telemetry::AccumulateTimeDelta(mozilla::Telemetry::REFRESH_DRIVER_TICK, mTickStart);
>  #endif
>  
> +  if (mNeedToRecomputeInViewportFrameVisibility && !mThrottled &&

The comment above ScheduleInViewportFrameVisibilityUpdate() says "Unlike approximate frame visibility updates, these are not throttled". Does that imply something about whether we should be checking the "mThrottled" flag here? (In particular, I notice we are checking it for mNeedToRecomputeApproxFrameVisibility, so I'm not sure how the two are different).
Attachment #8749064 - Flags: review?(botond) → feedback+

Updated

2 years ago
Attachment #8749065 - Flags: review?(botond) → review+
Comment on attachment 8749067 [details] [diff] [review]
(Part 2) - Add visualization of IN_VIEWPORT visibility to the APZ minimap.

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

::: gfx/layers/composite/LayerManagerComposite.h
@@ +417,5 @@
> +    {
> +      case VisibilityCounter::MAY_BECOME_VISIBLE: return mApproximatelyVisibleRegions;
> +      case VisibilityCounter::IN_DISPLAYPORT:     return mInDisplayPortVisibleRegions;
> +      case VisibilityCounter::IN_VIEWPORT:        return mInViewportVisibleRegions;
> +    }

Should we MOZ_ASSERT or something in the default case?
Attachment #8749067 - Flags: review?(botond) → review+
Attachment #8749066 - Flags: review?(mstange) → review+
Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

r+ assuming you use EnumerateSubDocuments.
Attachment #8749064 - Flags: review?(mstange) → review+
Comment on attachment 8749065 [details] [diff] [review]
(Part 1c) - Implement Visibility::IN_VIEWPORT.

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

::: layout/generic/nsFrame.cpp
@@ +2774,5 @@
>        nsIPresShell* shell = child->PresContext()->PresShell();
>        shell->MarkFrameVisible(child, VisibilityCounter::IN_DISPLAYPORT);
> +
> +      // If the frame is in the critical displayport, it may also be in the
> +      // viewport. A frame is considered to be IN_VIEWPORT if it's within the

Fun fact: Sometimes you can have a display port that doesn't completely include the viewport. But that should only be the case during fast scrolling and will be corrected as soon as scrolling stops.
Attachment #8749065 - Flags: review?(mstange)
Attachment #8749065 - Flags: review?(botond)
Attachment #8749065 - Flags: review+
Sorry, bugzilla broke botond's r+ there.
Comment on attachment 8749065 [details] [diff] [review]
(Part 1c) - Implement Visibility::IN_VIEWPORT.

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

(In reply to Markus Stange [:mstange] from comment #14)
> Sorry, bugzilla broke botond's r+ there.

Restoring.
Attachment #8749065 - Flags: review?(botond) → review+
(Assignee)

Comment 16

2 years ago
(In reply to Botond Ballo [:botond] from comment #10)
> The comment above ScheduleInViewportFrameVisibilityUpdate() says "Unlike
> approximate frame visibility updates, these are not throttled". Does that
> imply something about whether we should be checking the "mThrottled" flag
> here? (In particular, I notice we are checking it for
> mNeedToRecomputeApproxFrameVisibility, so I'm not sure how the two are
> different).

The terminology is bad here. =( There are two notions of throttling. One is the fact that the overall refresh driver can be throttled, for example if the document is in a background tab. The other notion (which is being referred to in that comment) is that we will only update approximately frame visibility every N milliseconds, regardless of whether updates are requested at a higher frequency. (There are exceptions to that but I'm planning on removing them.) Search for mMinRecomputeVisibilityInterval in nsRefreshDriver.cpp to see how that's implemented.

I'll see if I can reword the comment a bit.
(Assignee)

Updated

2 years ago
Depends on: 1272554
(Assignee)

Comment 17

2 years ago
Created attachment 8752040 [details] [diff] [review]
(Part 1a) - Add Visibility::IN_VIEWPORT to the frame visibility API.

Thanks for the quick reviews, folks!

This is a rebased version of part 1a.
(Assignee)

Updated

2 years ago
Attachment #8749060 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Created attachment 8752041 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

This version of part 1b addresses all of the review comments.
(Assignee)

Updated

2 years ago
Attachment #8749064 - Attachment is obsolete: true
(Assignee)

Comment 19

2 years ago
Created attachment 8752042 [details] [diff] [review]
(Part 1c) - Implement Visibility::IN_VIEWPORT.

Rebased.
(Assignee)

Updated

2 years ago
Attachment #8749065 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8752043 [details] [diff] [review]
(Part 1d) - Update various frame classes to use IN_VIEWPORT visibility.

Rebased.
(Assignee)

Updated

2 years ago
Attachment #8749066 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
Created attachment 8752044 [details] [diff] [review]
(Part 1e) - Add visualization of IN_VIEWPORT visibility to the APZ minimap.

Rebased and renumbered to part 1e per review comments.
(Assignee)

Updated

2 years ago
Attachment #8749067 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6413e5415ed1
(Assignee)

Comment 23

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed38eb707406
(Assignee)

Comment 24

2 years ago
OK, now that bug 1263349 is ready to land (and indeed has already been pushed), this one *should* be ready to go, but it's worth one more try job to make sure.
You need to log in before you can comment on or make changes to this bug.