634397 - Setting displayport redraws entire displayport

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benjamin Stover (:stechz)]

STR:
1. Run firefox -chrome chrome://global/content/test-ipcbrowser.xul
2. Go to http://google.com/search?q=test
3. Set displayport to 0, 0, 600, 600
4. Set a breakpoint at the end of ThebesLayerBuffer::BeginPaint
5. Set displayport to 0, 20, 600, 600

For step 5, result.mRegionToDraw is entire displayport when it only needs to be the bottom strip, since most of the pixels are the same.

Comment 1

[Benjamin Stover (:stechz)]

(this is on Linux desktop)

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

google.com is actually a fairly complicated page nowadays.  If you can repro this on just a tall page of text, it would help simplify analysis.

If this is a build with --enable-mobile-optimize (default for mobile, custom mozconfig setting on desktop), then this isn't unexpected because we can't self-copy image surfaces.

If this isn't an --enable-mobile-optimize build, then the things to check are
 - retained layers aren't being tossed out.  If they are, you'll see a       NS_WARNING("Flushing retained layers!") message.
 - the "right" layers are retained.  Check the layer tree before/after the paint that makes the too-large redraw.  If the layer pointers have changed, then the layers are perhaps being recreated unexpectedly.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There are other candidates, like a too-large invalidate coming from somewhere, but the ones above are most likely and easier to diagnose.

Comment 4

[Benjamin Stover (:stechz)]

OK, using http://people.mozilla.org/~bstover/font.html now. I commented out the SupportsSelfCopy check (that would be equivalent?). mRegionToDraw is still entire displayport.

In fact, when we calculate draw bounds:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ThebesLayerBuffer.cpp#210

It is the entire displayport, so I'm assuming this is because our valid region is completely empty. This makes sense to me, since we invalidate everything every time we call displayport. Am I missing something?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

See what the layer tree says.  Try running with MOZ_DUMP_PAINT_LIST=1.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Output for dumping paint list

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Setting displayport redraws entire displayport

Comment 8

[Benjamin Stover (:stechz)]

Comment on attachment 512607 [details] [diff] [review]
Setting displayport redraws entire displayport

Something like this seems to do the trick for me. Unforuntately, some sites like news.google.com have some corruption issues (you can see this as well when you call content.scrollTo instead of setting displayport, so it doesn't have anything to do with this patch).

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 512607 [details] [diff] [review]
Setting displayport redraws entire displayport

Nice!  I /think/ this is OK, but you would need real review from a layout person (obviously ;) ).

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Oops, didn't mean to reset assignee.)

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 512607 [details] [diff] [review]
Setting displayport redraws entire displayport

>+      rootFrame->InvalidateInternal(rootFrame->GetVisualOverflowRectRelativeToSelf() + rootFrame->GetPosition(),
>+                                    0, 0, rootFrame, nsIFrame::INVALIDATE_NO_THEBES_LAYERS);

This seems ok. Use InvalidateWithFlags instead. And the damage rect is relative to the frame itself, not its parent, so get rid of rootFrame->GetPosition().

Comment 12

[Benjamin Stover (:stechz)]

Changing dependencies due to changing the meaning of bug 634759. This bug can be checked in once ThebesLayerBuffer renders accurately and we fix the assertion that causes corruption.

Comment 13

[Benjamin Stover (:stechz)]

Attachment: Setting displayport redraws entire displayport

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 514925 [details] [diff] [review]
Setting displayport redraws entire displayport

what are the dependencies that are needed for this to land. The bugs that block this one are marked as soft blockers and have been denied approval to land.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Re-nom'ing for 4.x based on bug 637852.  We could take this and bug 635035 for 4.0 if we can live with the seaming, but I don't think that's a good idea.

Comment 16

[Benjamin Stover (:stechz)]

Perhaps we could add a redraw call at the very end of a scroll to Fennec and include this patch? As I understand we really want this for 4.0.

Comment 17

[Stuart Parmenter]

We have to get this in to speed up scrolling, reduce checkerboarding, and remove text jiggling

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #16)
> Perhaps we could add a redraw call at the very end of a scroll to Fennec and
> include this patch?

Can you elaborate a bit?

Comment 19

[Benjamin Stover (:stechz)]

Sure. We don't care *that* much about how precise the pixels look while we are panning, but when the pan is finished we expect the results to be correct. Fennec could ask for a redraw when a pan is finished, clearing up any inconsistencies that have been introduced by only doing partial invalidations.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I see.  Sounds worth trying.

Comment 21

[Benjamin Stover (:stechz)]

Attachment: Mobile workaround: redraw after pan is finished

Comment 22

[Benjamin Stover (:stechz)]

Attachment: Mobile workaround: redraw after pan is finished

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Does bug 633344 block this?

Comment 24

[Benjamin Stover (:stechz)]

I don't think so.

Comment 25

[Benjamin Stover (:stechz)]

No longer depends on 635373 because appropriate patches have landed.

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 517895 [details] [diff] [review]
Mobile workaround: redraw after pan is finished

Looks good to me.

Comment 27

[Benjamin Stover (:stechz)]

Pushed
mobile: http://hg.mozilla.org/mobile-browser/rev/e3b54fa2e3c6
m-c: http://hg.mozilla.org/mozilla-central/rev/71c15b2de18e