Closed Bug 795259 Opened 7 years ago Closed 7 years ago

Enable progressive tile drawing by default on Android

Categories

(Firefox for Android :: Toolbar, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 19

People

(Reporter: cwiiis, Assigned: cwiiis)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Progressive tile painting has been on m-c for a while and some recent bugs make it a pretty good replacement for the current drawing method on low-end devices (trading interactive performance for slightly increased checkerboarding).

Once we have interruptable drawing, we should enable it by default. Perhaps even before then (the jank improvement is very noticeable). I don't think this should wait for low-res tiles.
I don't intend to commit this until the dependent bugs are fixed, just getting early review to save time.
Attachment #665846 - Flags: review?(blassey.bugs)
Comment on attachment 665846 [details] [diff] [review]
Enable progressive tile drawing by default

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

r=blassey with dependent bugs fixed
Attachment #665846 - Flags: review?(blassey.bugs) → review+
Depends on: 797393
Depends on: 796117, 749063
Depends on: 800944, 796107
Attachment #671974 - Flags: review?(blassey.bugs)
rebased
Attachment #665846 - Attachment is obsolete: true
Attachment #671975 - Flags: review+
Attachment #671974 - Flags: review?(blassey.bugs) → review+
Depends on: 803013
Attachment #671974 - Flags: checkin+
After some discussion on IRC, we agreed that enabling progressive tile drawing should be coupled with disabling the screenshot layer.
Attachment #673284 - Flags: review?(blassey.bugs)
Attachment #673284 - Flags: review?(blassey.bugs) → review+
Enabling this, there appears to be a crasher whose cause is not immediately obvious: https://tbpl.mozilla.org/?tree=Try&rev=81e9f9820e9a
(In reply to Chris Lord [:cwiiis] from comment #8)
> Enabling this, there appears to be a crasher whose cause is not immediately
> obvious: https://tbpl.mozilla.org/?tree=Try&rev=81e9f9820e9a

Actually, looking again, the crash is in BasicThebesLayer and not BasicTiledThebesLayer... So perhaps this is pre-existing? Lots of red on talos though...
(In reply to Chris Lord [:cwiiis] from comment #9)
> (In reply to Chris Lord [:cwiiis] from comment #8)
> > Enabling this, there appears to be a crasher whose cause is not immediately
> > obvious: https://tbpl.mozilla.org/?tree=Try&rev=81e9f9820e9a
> 
> Actually, looking again, the crash is in BasicThebesLayer and not
> BasicTiledThebesLayer... So perhaps this is pre-existing? Lots of red on
> talos though...

My bad, just didn't look far enough up the stack. Sorry for the noise, will stop using bz as a sounding board now...
Looked a little into this... Going on the stack, the problem is that the layer builder is NULL - this shouldn't happen outside of a transaction and painting shouldn't happen outside of a transaction, so I don't think this ought to happen. Going on the stack, this isn't happen as the drawing is happening during EndTransaction (as expected).

My only guess here is that either the layer builder isn't set where it should be (unlikely, it gets set when the FrameLayerBuilder is initialised), or that it's being unset where it shouldn't. Perhaps this is exposed by the patch in bug 803013, as paint can be called multiple times for the same layer where previously it would only have been called once.

The only place I see it getting unset where perhaps it shouldn't is in nsDisplayList::PaintForFrame, where it unsets it even if it didn't start the transaction - it can also return without ending the transaction, where it also unsets it.

Cc'ing Matt for comment, as he's more familiar with this code.
Think the problem is indeed in nsDisplayList - if PaintForFrame is called with an existing layer manager, it will NULL out the existing layer builder on return. Bug + patch incoming, pending on try to confirm.
Simply checking the return value and returning also fixes the problem, but I'd have thought this would manifest in visual errors: https://tbpl.mozilla.org/?tree=Try&rev=613d5c384bde
Unfortunately not (the whole?) problem :( https://tbpl.mozilla.org/?tree=Try&rev=e070384d5ca5
Ok, noticed another problem - PaintInactiveLayer is used by ClippedDisplayItem, and clears the layer builder pointer - so if this was called twice, it'd likely lead to this crash. There's no need (I think) for it to clear it, as it gets cleared again when ClippedDisplayItem is destroyed.

Testing...
Another note, this is almost certainly happens regardless of bug 803013. Jumped to the wrong conclusion there.
Depends on: 803826
Filed blocking bug 803826 - try run with patches from that bug: https://tbpl.mozilla.org/?tree=Try&rev=908afe795512

Not too happy about the amount of red in talos, but possibly a coincidence - C3 is fixed at least.
Removing two blocking bugs - we've decided to disable the screenshot layer, so no need to draw it underneath progressive drawing, and we've decided we can take this without improving the reusable tile store (though this will provide a nice improvement later).
No longer depends on: 796107, 796117
Should we retire talos test tcheck3 now? (tcheck3 is identical to tcheck2, except that it first disables screenshots.)
Thanks for pointing that out. I'd like to wait perhaps 2 nightlies to make sure this feature doesn't bounce then let's do it.
https://hg.mozilla.org/mozilla-central/rev/b6bd9f564650
https://hg.mozilla.org/mozilla-central/rev/8cc37887f621
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
(In reply to Geoff Brown [:gbrown] from comment #20)
> Should we retire talos test tcheck3 now? (tcheck3 is identical to tcheck2,
> except that it first disables screenshots.)

Sounds like you could file a bug for it, so we don't forget.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/84cfd17d0647, though we'll see whether or not it stays out - this landed while reftests were very nearly completely broken, and when they got unbroken, reftest-1 and reftest-4 came out permaorange. The push before this had a wee bit of green, but even with massive retriggers all I could get on this push was the all-white useless results. If we wind up still permaorange without this, it can always just reland again.
Dunno which of this or bug 803013 (which I also backed out in the same cset) it was, but with them backed out, reftests are back to as normal and as green as they get.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 805014
Depends on: 805028
https://hg.mozilla.org/mozilla-central/rev/bd0161b9250f
https://hg.mozilla.org/mozilla-central/rev/b7dc8db41317
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
What improvements are we looking for here that we can verify? Also, what are some possible regression cases to watch for?
The latest video on eideticker should give a good idea of the changes that have landed:
http://eideticker.wrla.ch/#/samsung-gn/taskjs-scrolling/checkerboard

Improvements
- Progressive tile by tile drawing while panning
- Drawing abort - While changing pan direct and zoom level quickly we cancel the paint. Previous we would have to wait for old and useless paints to complete.
- The screen should fill in faster in any situation and then draw the edge of the screen.
- No more screenshot jank sporadically slowing down the drawing and causing the browser to be unresponsive

Regression
- Lack of screenshot when panning quickly (bug 796117 will improve this)
Blocks: 806984
Depends on: 807299
Depends on: 808562
Blocks: 809185
Depends on: 795438
You need to log in before you can comment on or make changes to this bug.