1159997 - Use a tight dirty region/rect for PaintFrame

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Chiajung Hung [:chiajung]]

While PaintFrame, we always use full viewport/displayport as dirty region and build displayItem for all elements even if they are not dirty. This contributes a huge BuildDisplayList/ProcessDisplayItems chunk on GeckoProfiler.

We should make PaintFrame use a tight dirtyRegion to process displayList first, then try to make BuildDisplayList use nsRegion rather than nsRect for a tighter DisplayList.

Comment 1

[Markus Stange [:mstange]]

(In reply to Chiajung Hung [:chiajung] from comment #0)
> and build displayItem for all elements even if they are not dirty.

How would you figure out whether a displayItem needs to be repainted if you don't build the complete display list + run layer building?

Comment 2

[Chiajung Hung [:chiajung]]

The idea is to collect dirty region by collect changed nsFrame(reflowed or need repaint).

For animation case, we will keep 2 dirty rect for each frame. 1 for last paint, 1 for this paint. And I think occulusion is not a problem because nsFrame::BuildDisplayListForChild intersect the dirtyRect with the nsFrame's child. Even if a fully covered nsFrame builds its displayItem.

(In reply to Markus Stange [:mstange] from comment #1)
> (In reply to Chiajung Hung [:chiajung] from comment #0)
> > and build displayItem for all elements even if they are not dirty.
> 
> How would you figure out whether a displayItem needs to be repainted if you
> don't build the complete display list + run layer building?

Would any displayItem needs paint but its nsFrame have no NS_FRAME_NEED_PAINT set or not reflowed? (Except nsFrame with nsView, I found such frame are not invalidated but need paint anyway)
Do we need build layer for those unchanged frame/display item?

In fact, I am still trying to have a prove of concept patch first.
Most of my question maybe obvious once I get there. :)

Comment 3

[Matt Woodrow (:mattwoodrow)]

The big problem is that building a partial display list can change layerization.

Imagine something like this:

Background 0,0,1000,1000
Video(ImageLayer) 0,800, 100, 100
Background 0,0, 200, 900

If we build a display list for the entire 1000x1000, then we end up needing 3 layers, because the layer for the video is in between the other two items.

If we build a display list for a dirty rect (say 0,0,500,500), then we don't build the video display item, and we think we can put both Background items in the same layer.

Roc's proposal [1] to split things into tiles solves this, but it's fairly complicated.

[1] https://wiki.mozilla.org/Gecko:PortlandRendering

Comment 4

[Chiajung Hung [:chiajung]]

Attachment: WIP

This is a simple prototype which cause various rendering error for now:
(1) Layer management is currently based on DisplayList, a compact display list make it remove all layer except dirty one.
(2) If we force to not remove layer based on DisplayList, some layer may recycled but nothing to draw and hit an assertion in [1]

A proper fix for (1) will involving a new Layer removal mechanism which I am currently working on, and (2) is found after my rebase so I am still study for the solution.

Here is some number to show how much performance this may provide:
Base version:
changeset:   243712:1fab94ad196c
tag:         qparent
user:        Wes Kocher <[e-mail]>
date:        Wed May 13 14:17:19 2015 -0700
summary:     Backed out changeset bff13db4c158 (bug 1137593) for being the best guess of what's breaking Gij(20) a=backout CLOSED TREE

30x30 maze solver runs about 20~25 sec to complete. With this patch it cost only 5~8 sec to complete.

However, there still many things to be fixed, this number may not reflect the final result.

[1]https://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#5248

Comment 5

[Markus Stange [:mstange]]

Can you please summarize your planned solution for (1)? It wouldn't be wise to spend your time going down the wrong path.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Getting this to work correctly is a pretty hard problem. I suggest you come up with a detailed design that preserves correctness before spending time implementing anything.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

As Matt pointed out, https://wiki.mozilla.org/Gecko:PortlandRendering is a sketch of such a design. It would be great to come up with something simpler though.

Comment 8

[Chiajung Hung [:chiajung]]

(In reply to Markus Stange [:mstange] from comment #5)
> Can you please summarize your planned solution for (1)? It wouldn't be wise
> to spend your time going down the wrong path.

There are 2 path for this now:
(1) While ~nsFrame, we will clean up all DisplayItemData in Properties, and we can mark associated layers to be remove from its parent at the same time. 
(2) For those nsFrame(s) that are not destructed, but do not recycle the old layer while dirty. We may add some flag into Frame while they build a new layer and while buildDisplayList for the Frame the next time, we can add a new DisplayItem into its list. Then we can check whether it reuse old layer while processDisplayItems. Or we simply hack GetOldLayerFor and mark all layers for the frame that are not recycled by any items to be removed.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Getting this to work correctly is a pretty hard problem. I suggest you come
> up with a detailed design that preserves correctness before spending time
> implementing anything.
In fact, this is my first time to hack this part, and :thinker is my mentor for this. As we are not super familiar with these pieces we are studying it from prototyping for now.

The patch there is a intermediate output from our study until now. Maybe we will come out something similar to your proposal in the end. :)

Any suggestion from all you guys is super appreciated. Please keep tracking of this bug and tell us what is wrong.

Thanks!

Comment 9

[Thinker Li [:sinker]]

I have a lot of discussions with Chiajung.  We start to look on this problem for the fact that gecko spending most of time on building and processing display items while running mazesolver.  The basic idea is
 1. the dirty rect passing to the root frame is an union of all dirty/changed frames.
 2. layers built by items of frames in the dirty rect would be removed if it is not reused.
 3. layers built by items of frames out of the dirty rect would be kept as it was.
 4. layers build by items of removed frames are removed.

For now, we depend on information on DisplayItemData to do step 2, 3 & 4.

One of known issues is if DisplayItemData should be updated, and how!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does your plan handle Matt's example in comment #3? If so, how?

Comment 11

[Chiajung Hung [:chiajung]]

Thinker and I discussed that problem, and come out a simple solution:

The basic idea is:
(1) If a DisplayListItem have DisplayItemData in nsFrame, and DisplayItemData::mLayer has intersections with other layers in the layer tree snapshot in DisplayItemData, then we forbid it keep same tree structure. (Forbid layer changing, keep same layer tree structure)

(2) Else if Layer re-assign happens, we copy/draw old data from old layer onto new layer.

For case (1), we can simply hack FindPaintedLayerDataFor and check layer tree, then returns new PaintedLayerData if case (1) Layer intersection occurs.

For case (2), we may hack only ComputeGeometryChangeFor.

Comment 12

[Bobby Chien]

[Tracking Requested - why for this release]:

Comment 13

[Matt Woodrow (:mattwoodrow)]

RDL effectively does this, and is landed now.