795259 - Enable progressive tile drawing by default on Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Chris Lord [:cwiiis]]

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.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Enable progressive tile drawing by default

I don't intend to commit this until the dependent bugs are fixed, just getting early review to save time.

Comment 2

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

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

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Add pref now for testing

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Enable progressive tile drawing by default

rebased

Comment 5

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bbdc2a1b3d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5bbdc2a1b3d

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Disable screenshot layer

After some discussion on IRC, we agreed that enabling progressive tile drawing should be coupled with disabling the screenshot layer.

Comment 8

[Chris Lord [:cwiiis]]

Enabling this, there appears to be a crasher whose cause is not immediately obvious: https://tbpl.mozilla.org/?tree=Try&rev=81e9f9820e9a

Comment 9

[Chris Lord [:cwiiis]]

(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...

Comment 10

[Chris Lord [:cwiiis]]

(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...

Comment 11

[Chris Lord [:cwiiis]]

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.

Comment 12

[Chris Lord [:cwiiis]]

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.

Comment 13

[Chris Lord [:cwiiis]]

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

Comment 14

[Chris Lord [:cwiiis]]

Unfortunately not (the whole?) problem :( https://tbpl.mozilla.org/?tree=Try&rev=e070384d5ca5

Comment 15

[Chris Lord [:cwiiis]]

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...

Comment 16

[Chris Lord [:cwiiis]]

Another note, this is almost certainly happens regardless of bug 803013. Jumped to the wrong conclusion there.

Comment 17

[Chris Lord [:cwiiis]]

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.

Comment 18

[Chris Lord [:cwiiis]]

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).

Comment 19

[Chris Lord [:cwiiis]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bd9f564650
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc37887f621

Comment 20

[Geoff Brown [:gbrown]]

Should we retire talos test tcheck3 now? (tcheck3 is identical to tcheck2, except that it first disables screenshots.)

Comment 21

[Benoit Girard (:BenWa)]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6bd9f564650
https://hg.mozilla.org/mozilla-central/rev/8cc37887f621

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 24

[Phil Ringnalda (:philor)]

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.

Comment 25

[Phil Ringnalda (:philor)]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Phil Ringnalda (:philor) from comment #24)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/84cfd17d0647

https://hg.mozilla.org/mozilla-central/rev/84cfd17d0647

Comment 27

[Chris Lord [:cwiiis]]

Oranges and talos misreporting fixed in other bugs, back on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0161b9250f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7dc8db41317

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd0161b9250f
https://hg.mozilla.org/mozilla-central/rev/b7dc8db41317

Comment 29

[Aaron Train [:aaronmt]]

What improvements are we looking for here that we can verify? Also, what are some possible regression cases to watch for?

Comment 30

[Benoit Girard (:BenWa)]

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)