Closed Bug 634397 Opened 9 years ago Closed 9 years ago

Setting displayport redraws entire displayport

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: stechz, Assigned: stechz)

References

Details

(Keywords: mobile, perf, Whiteboard: [has patch][waiting on dependencies])

Attachments

(3 files, 2 obsolete files)

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.
(this is on Linux desktop)
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.
There are other candidates, like a too-large invalidate coming from somewhere, but the ones above are most likely and easier to diagnose.
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?
See what the layer tree says.  Try running with MOZ_DUMP_PAINT_LIST=1.
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).
Attachment #512607 - Flags: feedback?(jones.chris.g)
Blocks: 624444
tracking-fennec: --- → ?
Keywords: mobile, perf
tracking-fennec: ? → 2.0+
Assignee: nobody → ben
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 ;) ).
Attachment #512607 - Flags: feedback?(jones.chris.g) → feedback+
Assignee: ben → nobody
Component: Graphics → Layout
QA Contact: thebes → layout
(Oops, didn't mean to reset assignee.)
Assignee: nobody → ben
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().
Attachment #512607 - Flags: review+
Depends on: 634759
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.
Depends on: 635373, 633344
No longer depends on: 634759
Attachment #512607 - Attachment is obsolete: true
Attachment #514925 - Flags: review?(tnikkel)
Attachment #514925 - Flags: review?(tnikkel) → review+
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.
Blocks: 624454
Whiteboard: [needs-dependencies]
Depends on: 637852
Whiteboard: [needs-dependencies]
Whiteboard: [has patch][waiting on dependencies]
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.
tracking-fennec: 2.0+ → ?
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.
We have to get this in to speed up scrolling, reduce checkerboarding, and remove text jiggling
tracking-fennec: ? → 2.0+
(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?
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.
I see.  Sounds worth trying.
Attachment #517893 - Attachment is obsolete: true
Attachment #517895 - Flags: review?(wjohnston)
Does bug 633344 block this?
No longer depends on: 637852
I don't think so.
No longer depends on: 633344
No longer depends on 635373 because appropriate patches have landed.
No longer depends on: 635373
Comment on attachment 517895 [details] [diff] [review]
Mobile workaround: redraw after pan is finished

Looks good to me.
Attachment #517895 - Flags: review?(wjohnston) → review+
Pushed
mobile: http://hg.mozilla.org/mobile-browser/rev/e3b54fa2e3c6
m-c: http://hg.mozilla.org/mozilla-central/rev/71c15b2de18e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 641506
Depends on: 642282
You need to log in before you can comment on or make changes to this bug.