Open Bug 1261554 Opened 4 years ago Updated 3 years ago

Track whether frames are visible in the displayport as the part of the new frame visibility API

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

REOPENED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 5 open bugs)

Details

Attachments

(6 files, 1 obsolete file)

Right now, because approximate frame visibility is only updated on a 1hz timer, there is no guarantee that a frame is marked as visible even if it's within the displayport. We should fix that by adding all frames in the display list we use for painting the displayport to the visible frames list.

Some notes:

- This won't work for removing frames, because the area we track for frame visibility is bigger than the displayport. (Except in the case where we're painting a low-res displayport!) So we'll still need the 1hz update to remove frames.

- In a low memory situation (i.e., during memory pressure) we'll do a synchronous remove-only pass over the visible frames list, so if this causes too many images to be locked in low-memory situations, we should still survive as before.

- When we're painting the low-res displayport, we probably want to clip to the critical displayport, because pulling the entire low-res displayport in will likely pull in too many frames.
The process of writing this patch changed my views about how to approach the problem somewhat.

Instead of adding frames in the displayport to the same approximately visible frame set, it makes sense to keep a separate set of frames in the displayport. This means that we *can* remove frames from that set, which should prevent us from ending up with a massive number of frames between frame visibility updates.

There's a second advantage, too, and this one is possible more significant. By doing this, frames know with total certainty when they're within the displayport, and this lets us be smarter about resource usage. The most obvious application is animated images: we currently animate images within the approximate frame visibility area (which is much bigger than the displayport), but with this change we can easily make it so we only animate images in the displayport, while still retaining the ability to lock images or predictively decode images within the larger approximate frame visibility area. There are likely other refresh driver-driven things that could benefit from this. (<iframe> throttling would've been trivial if we had this when I was implementing it, for example.)

The try job in comment 1 is testing an implementation of this idea. If everything looks good on that try job, the patch is more or less ready for review; the only thing it's lacking that we should consider adding is clipping to the critical displayport.
The try job in comment 1 suffered from a rather unfortunate typo that caused a ton of tests to fail. Let's try that one more time.
Blocks: 1263019
OK, linux is all green. I'll push another try job once I split up the patches, to make sure the remaining platforms look good as well.
Summary: Make approximate frame visibility guarantee that a frame is marked as visible when it's within the displayport → Track whether frames are visible in the displayport as the part of the new visibility API
Summary: Track whether frames are visible in the displayport as the part of the new visibility API → Track whether frames are visible in the displayport as the part of the new frame visibility API
OK, here we go. I've split this patch into a bunch of parts to make reviewing
easier.

Part 1 (which is three patches that will need to be squashed before landing)
lays the foundation for tracking whether frames are visible in the displayport,
which we'll actually implement in part 2.

In this first patch, the visibility API on nsIFrame gets updated to support
in-displayport visibility. Essentially, the Visibility enum gets a new value,
IN_DISPLAYPORT and the existing value APPROXIMATELY_NONVISIBLE gets renamed to
NONVISIBLE since, because we'll update the in-displayport visibility information
on every paint, it's now totally accurate. Then the nsIFrame
VisibilityStateProperty gets updated so that the single 32-bit counter is split
into two 16-bit counters, one for approximate visibility and one for
in-displayport visibility. These counters are still much larger than we need in
practice; 16 bits is plenty. Finally, the nsIFrame visibility API is updated to
take the new Visibility states into account and to allow both counters to be
incremented or decremented.
Attachment #8739632 - Flags: review?(mstange)
In this second patch, a lot of renaming and tweaking happens in the
nsIPresShell/nsPresShell visibility API to prepare for displayport visibility
tracking.

Note that this patch doesn't actually implement it! It's all prep. A separate
set of frames for in-displayport visibility tracking gets added, and recording
regions for the APZ minimap visualization gets updated to support the second
frame list, but we still don't actually add anything to the in-displayport frame
set.

The exception is that we *do* start calling MarkFrameVisibleInDisplayPort() for
frames that we're *forcing* to be visible in this patch. This replaces the old
approach of forcing them to be in the approximate visibility frame set. (The
reason we do it in this patch is that there's no reason to force a frame to be
in the approximately visible set anymore, so one method just replaces another.)
The change is appropriate because when we force a frame to be visible, we want
to treat it as if it's "maximally visible". In particular, in the future we
probably want to start doing things like only animating images if they're within
the displayport. That means that when we can't accurately track visibility for a
frame (which is the reason that we force them to be visible), to be on the safe
side we should treat them as if they're in the displayport. We'll eventually
have in-viewport visibility as well and we'll probably want to promote them
further to in-viewport visibility at that point.
Attachment #8739633 - Flags: review?(mstange)
This patch just updates miscellanous frame and DOM code to use the new API and
the new visibility states. In line with the previous comment, when we force a
frame to be visible, we promote it to IN_DISPLAYPORT visibility in this patch.
Attachment #8739634 - Flags: review?(mstange)
This patch is where we actually implement in-displayport visibility tracking.
The idea is really simple: when we're constructing a displaylist for painting to
the window, if a frame ends up in the displaylist, we add it to the
in-displayport visible frame set. Then we do the usual bookkeeping for a
visibility update (clearing out the old frames from the frame set, decrementing
their visible count, doing region tracking for the APZ minimap visualization) in
nsPresShell::Paint. Since visibility tracking is done at the pres shell level,
this is the logical place to do it.

There's actually not much code here! After the preparation in part 1, the actual
implementation is pretty simple.
Attachment #8739635 - Flags: review?(mstange)
This part adds visualization of the IN_DISPLAYPORT regions to the APZ minimap
visibility debugger. This is unfortunately about as much code as part 1 and part
2 put together, but for debugging purposes it's really important to have this
available. There's nothing complicated here though, it's mostly just a bunch
of IPDL updates and plumbing.

The result of part 3 is that approximately visible regions are visualized in
red, while in-displayport visible regions are visualized in yellow.
Attachment #8739636 - Flags: review?(botond)
One more note - I think it's best to handle clipping to the critical displayport in a followup bug. It's not hard, but for efficiency it'd be good to pass the nearest enclosing scrollframe down (probably in nsDisplayListBuilder) when we're walking the frame tree to build the displaylist, and that seems like enough complexity to warrant its own bug.
Blocks: 1263349
Comment on attachment 8739636 [details] [diff] [review]
(Part 3) - Visualize Visibility::IN_DISPLAYPORT regions in the APZ minimap visibility debugger.

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

r+ with comments addressed. Thanks!

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +473,5 @@
>    }
>  }
>  
> +template <typename Func> void
> +DrawRegion(CSSIntRegion* aRegion, gfx::Color aColor, const Func& aFunc)

Please use more descriptive names for |Func| and |func|. (PaintRect?)

@@ +573,5 @@
>      MOZ_ASSERT(controller);
>  
>      ScrollableLayerGuid guid = controller->GetGuid();
>  
> +    auto regionPainter = [&](const CSSIntRect& aRect, const gfx::Color& aColor) {

This seems more like a rectPainter.

::: gfx/layers/ipc/PCompositorBridge.ipdl
@@ +192,3 @@
>  
>    // The child sends a region containing rects associated with the provided
>    // scrollable layer GUID that the child considers 'approximately visible'.

Update the 'approximately visible' part in the comment (since the function is now more general than that).

::: layout/generic/VisibilityIPC.h
@@ +19,5 @@
> +
> +namespace IPC {
> +
> +template<>
> +struct ParamTraits<mozilla::VisibilityCounter>

We have a nicer way of serializing enums for IPC [1]. I would prefer using that.

https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/ipc/glue/IPCMessageUtils.h#140
Attachment #8739636 - Flags: review?(botond) → review+
Blocks: 1259281
Comment on attachment 8739632 [details] [diff] [review]
(Part 1a) - Add a second visibility counter for frames that are within the displayport and update the API to expose it.

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

::: layout/generic/nsFrame.cpp
@@ +1624,4 @@
>  
> +  Visibility previousVisibility = VisibilityStateAsVisibility(state);
> +
> +  if (aCounter == VisibilityCounter::MAY_BECOME_VISIBLE) {

Seems like this would be simpler if VisibilityState had a CounterFor(VisibilityCounter) method that returned a reference to the appropriate member.
Attachment #8739632 - Flags: review?(mstange) → review+
Comment on attachment 8739633 [details] [diff] [review]
(Part 1b) - Rework the pres shell visibility infrastructure to prepare for displayport visibility tracking.

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

::: layout/base/nsPresShell.cpp
@@ +11115,5 @@
> +  *aPresShellSize += mInDisplayPortFrames.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +  *aPresShellSize += mVisibleRegions
> +                   ? mVisibleRegions->mApproximate.ShallowSizeOfExcludingThis(aMallocSizeOf) +
> +                     mVisibleRegions->mInDisplayPort.ShallowSizeOfExcludingThis(aMallocSizeOf)
> +                   : 0;

I admit I don't know why some of these use "excluding this" and others use "including this".
Attachment #8739633 - Flags: review?(mstange) → review+
Attachment #8739634 - Flags: review?(mstange) → review+
Attachment #8739635 - Flags: review?(mstange) → review+
Thanks for the reviews!

(In reply to Markus Stange [:mstange] from comment #14)
> Seems like this would be simpler if VisibilityState had a
> CounterFor(VisibilityCounter) method that returned a reference to the
> appropriate member.

That was the first thing I tried, but alas, C++ does not allow references to bitfields.
(In reply to Markus Stange [:mstange] from comment #15)
> I admit I don't know why some of these use "excluding this" and others use
> "including this".

When the memory reporting code wants to know how much memory an instance of class Foo takes, it can easily just call sizeof(Foo) to get the size required to hold Foo's members. However, often the members contain pointers that hold on to other regions of memory. So say you have a member of type Bar - you call a Bar::SizeOfExcludingThis() method to say "I already counted the memory Bar's members took when I called sizeof() on myself, so I just need to know how much memory is consumed by the things that Bar points to."

On the other hand, say Foo doesn't have a Bar, it has a Bar*. Then only the pointer to Bar was included in sizeof(Foo), so we need to get the amount of memory used by both Bar *and* the things that Bar points to. In that case, you'd call Bar::SizeOfIncludingThis().

Hopefully that clears things up. It comes down to this:

sizeof(Foo) + Foo::SizeOfExcludingThis() == Foo::SizeOfIncludingThis()
(In reply to Botond Ballo [:botond] from comment #13)
> We have a nicer way of serializing enums for IPC [1]. I would prefer using
> that.
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 29d5a4175c8b74f45482276a53985cf2568b4be2/ipc/glue/IPCMessageUtils.h#140

I'll use this, but I'm not loving the design. We should really use a traits class so the range of the enum can be defined right next to the enum, without compromising the compiler's ability to give exhaustiveness warnings for switch statements using the enum, as adding a NUM_FOO enum value would do.
Actually I dunno. I really don't like that design; I'd vastly prefer having exhaustiveness warnings. I think I'd prefer to just file a bug about fixing it and leave the code as is for now.
Blocks: 1266301
This is an updated version of part 3 with comments addressed, except for the
exception mentioned in comment 19.
Attachment #8739636 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/69abdc731a9962cb1220facb41f26bef7087b486
Bug 1261554 (Part 1) - Prepare for implementing in-displayport visibility tracking. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c3b4b81d82daa6cd0dcddfecdcab9e0d0d1773
Bug 1261554 (Part 2) - Mark frames which are added to the display list when painting to the window as having Visibility::IN_DISPLAYPORT. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3bf463c0ec5fae4dcb8469a1cd127d40ad8f59
Bug 1261554 (Part 3) - Visualize Visibility::IN_DISPLAYPORT regions in the APZ minimap visibility debugger. r=botond
(In reply to Seth Fowler [:seth] [:s2h] from comment #17)
> (In reply to Markus Stange [:mstange] from comment #15)
> > I admit I don't know why some of these use "excluding this" and others use
> > "including this".
> 
> [...]

But you're not adding sizeof(*mVisibleRegions), so shouldn't you be calling mVisibleRegions->mApproximate.SizeofIncludingThis()?
(In reply to Seth Fowler [:seth] [:s2h] from comment #19)
> Actually I dunno. I really don't like that design; I'd vastly prefer having
> exhaustiveness warnings. I think I'd prefer to just file a bug about fixing
> it and leave the code as is for now.

Sounds good; I'm not a fan of having to add a new enumerator, either. If you file a bug about improving the enum serializer, please cc me on it, and I'll try to fix it when I get a chance.
(In reply to Markus Stange [:mstange] from comment #22)
> But you're not adding sizeof(*mVisibleRegions), so shouldn't you be calling
> mVisibleRegions->mApproximate.SizeofIncludingThis()?

Hah, yes! Good point. The initial draft of the code hold on to the code via a pointer, and I failed to update it. I'll push a trivial followup.
(In reply to Seth Fowler [:seth] [:s2h] from comment #25)
> The initial draft of the code hold on to the code via

"didn't hold on"
https://hg.mozilla.org/integration/mozilla-inbound/rev/a909de86c18365afdf17338c850097d94b554e8b
Bug 1261554 (Followup) - Fix memory reporting for PresShell::mVisibleRegions. r=me
Blocks: 1268348
Depends on: 1284350
I backed this bug out on inbound, I expect the backout to get merged to mozilla-central. I plan to also request uplift to backout on aurora (so this bug would not be landed in any version of Firefox).

Bug 1284350 tracks the back out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.