Open
Bug 1261554
Opened 8 years ago
Updated 1 year ago
Track whether frames are visible in the displayport as the part of the new frame visibility API
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
REOPENED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: seth, Unassigned)
References
(Blocks 4 open bugs)
Details
Attachments
(6 files, 1 obsolete file)
14.74 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
32.19 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
13.20 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
3.95 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
27.23 KB,
patch
|
Details | Diff | Splinter Review | |
1.65 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cec869e80bb7
Reporter | ||
Comment 2•8 years ago
|
||
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.
Reporter | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfad57e4ac90
Reporter | ||
Comment 4•8 years ago
|
||
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.
Reporter | ||
Comment 5•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
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
Reporter | ||
Updated•8 years ago
|
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
Reporter | ||
Comment 6•8 years ago
|
||
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)
Reporter | ||
Comment 7•8 years ago
|
||
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)
Reporter | ||
Comment 8•8 years ago
|
||
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)
Reporter | ||
Comment 9•8 years ago
|
||
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)
Reporter | ||
Comment 10•8 years ago
|
||
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)
Reporter | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=306f05926034
Reporter | ||
Comment 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
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+
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8739634 -
Flags: review?(mstange) → review+
Updated•8 years ago
|
Attachment #8739635 -
Flags: review?(mstange) → review+
Reporter | ||
Comment 16•8 years ago
|
||
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.
Reporter | ||
Comment 17•8 years ago
|
||
(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()
Reporter | ||
Comment 18•8 years ago
|
||
(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.
Reporter | ||
Comment 19•8 years ago
|
||
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.
Reporter | ||
Comment 20•8 years ago
|
||
This is an updated version of part 3 with comments addressed, except for the exception mentioned in comment 19.
Reporter | ||
Updated•8 years ago
|
Attachment #8739636 -
Attachment is obsolete: true
Reporter | ||
Comment 21•8 years ago
|
||
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
Comment 22•8 years ago
|
||
(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()?
Comment 23•8 years ago
|
||
(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.
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/69abdc731a99 https://hg.mozilla.org/mozilla-central/rev/82c3b4b81d82 https://hg.mozilla.org/mozilla-central/rev/bb3bf463c0ec
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter | ||
Comment 25•8 years ago
|
||
(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.
Reporter | ||
Comment 26•8 years ago
|
||
(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"
Reporter | ||
Comment 27•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a909de86c18365afdf17338c850097d94b554e8b Bug 1261554 (Followup) - Fix memory reporting for PresShell::mVisibleRegions. r=me
Reporter | ||
Comment 28•8 years ago
|
||
Here's the followup.
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a909de86c183
Comment 30•8 years ago
|
||
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 → ---
Comment 31•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: seth.bugzilla → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•