887819 - Investigate using the tiled layers backend

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

We get quite a performance boost using the tiled layers backend in fennec, as it:

- Allows us to avoid NPOT textures
- Allows us to avoid subimage upload
- Enables progressive updating
- Enables update cancelling

It makes sense to investigate using the same backend on b2g. Note that unfortunately, it's not as simple as just flipping a switch, this likely requires some supporting code that is currently implemented in Java in fennec.

Comment 1

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #0)
> - Allows us to avoid NPOT textures
The tile backend actually use 256^2 tiles
> - Allows us to avoid subimage upload
> - Enables progressive updating
> - Enables update cancelling

Other benefit are handling resizing the buffer more efficiently. Running with layer border you can notice that we resize our layers quite often.

> 
> It makes sense to investigate using the same backend on b2g. Note that
> unfortunately, it's not as simple as just flipping a switch, this likely
> requires some supporting code that is currently implemented in Java in
> fennec.

We could just disable those 2 features for now and still investigate the tile layer backend.

The real problem is making tile sharing cross-process safe. I had some patch that were nearly there but they will be bit-rotted so will need some tedious manual rebasing. This is the bulk of the work.

Comment 2

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #1)
> (In reply to Chris Lord [:cwiiis] from comment #0)
> > - Allows us to avoid NPOT textures
> The tile backend actually use 256^2 tiles
> > - Allows us to avoid subimage upload
> > - Enables progressive updating
> > - Enables update cancelling
> 
> Other benefit are handling resizing the buffer more efficiently. Running
> with layer border you can notice that we resize our layers quite often.

Missed this one :)

> > 
> > It makes sense to investigate using the same backend on b2g. Note that
> > unfortunately, it's not as simple as just flipping a switch, this likely
> > requires some supporting code that is currently implemented in Java in
> > fennec.
> 
> We could just disable those 2 features for now and still investigate the
> tile layer backend.

Right, we should definitely do this in stages. I would imagine that even without these features, we may see a performance boost, especially on devices with larger screens.

> The real problem is making tile sharing cross-process safe. I had some patch
> that were nearly there but they will be bit-rotted so will need some tedious
> manual rebasing. This is the bulk of the work.

Care to attach those to this bug? Maybe someone will have the time to do so before you do, unless you think it's not quite at that stage yet?

Comment 3

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #2)
> > The real problem is making tile sharing cross-process safe. I had some patch
> > that were nearly there but they will be bit-rotted so will need some tedious
> > manual rebasing. This is the bulk of the work.
> 
> Care to attach those to this bug? Maybe someone will have the time to do so
> before you do, unless you think it's not quite at that stage yet?

Making this depend on bug 747811 since it still accurately describes the pre-req.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I think bug 859929 might also come into play here if we turn this on. Linking it here just in case.

Comment 5

[Benoit Girard (:BenWa)]

You're right. I suggest we use this bug for enabling tiling without progressive drawing and without draw aborting. Those features have their own set of problems but they just happen to depend on tiling. Tiling should remain independent of features that use them (progressive, drawing aborting).

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Enable tiles on b2g

So I'm going to be pushing the patches on bug 747811 today, and with those, the tiles backend now works on b2g.

Discussing this with blassey, we came to the conclusion that even without better display-port sizing and the other features listed in comment #0 and comment #1, we should enable this to see how it does/get further testing.

This patch enables tiles on b2g.

Comment 7

[Chris Lord [:cwiiis]]

I'm not really sure whether the patch I've attached should be here or on bug 894333 (and if it should be there, I don't really know what this bug is for).

I'm leaving it here for now, though I would say that we've done the investigation now and initial results look pretty promising.

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 792112 [details] [diff] [review]
Enable tiles on b2g

Landing this would disable gralloc for content. Do you have data to suggest that we're ready doing this without a Gralloc implementation of tiles?

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #8)
> Comment on attachment 792112 [details] [diff] [review]
> Enable tiles on b2g
> 
> Landing this would disable gralloc for content. Do you have data to suggest
> that we're ready doing this without a Gralloc implementation of tiles?

I don't yet, but hopefully I'll have eideticker results soon.

Personally, I think if the drop just switching it on with no further enhancements isn't too large (who knows, it may not even be a drop), we should take it and not use gralloc anyway - I see obvious visible glitches using gralloc on both the Keon and Peak (though I pushed a patch to disable gralloc on the Peak because it got hopelessly broken at some point).

I'd be very surprised if after enabling progressive rendering and cancelling, if using tiles without gralloc was outperformed by not using it, with.

Comment 10

[Benoit Girard (:BenWa)]

I agree with what you said. I'd just like to have some analysis to back up our decision in this bug. Lets wait for the eideticker results.

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 792112 [details] [diff] [review]
Enable tiles on b2g

Postponing review until we have an argument to support switching this on.

Comment 12

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

Attachment: make tile-enabling pref

Comment 13

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

Attachment: enable tiles pref for b2g

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 802496 [details] [diff] [review]
enable tiles pref for b2g

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

Flipping this preference will have the following results:
- We wont use buffer rotation for thebes content, we will use tiling which will support changing dimensions better.
- Thebes layer will no longer use gralloc but instead use regular OGL textures. Some things will get better but some things may get worse. This means disabling HWC since we always have a non gralloc layer.

I think at this point we should get wider testing with the preference flipp before officially flipping it by default during feature freeze.

Comment 15

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

pushed the pref patch https://hg.mozilla.org/mozilla-central/rev/efc53394043e

Comment 16

[Andreas Gal :gal]

We would need to restore gralloc for this to make 1.2. That seems doable though. BenWa, can you comment?

Comment 17

[Jason Smith [:jsmith]]

(In reply to Benoit Girard (:BenWa) from comment #14)
> Comment on attachment 802496 [details] [diff] [review]
> enable tiles pref for b2g
> 
> Review of attachment 802496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Flipping this preference will have the following results:
> - We wont use buffer rotation for thebes content, we will use tiling which
> will support changing dimensions better.
> - Thebes layer will no longer use gralloc but instead use regular OGL
> textures. Some things will get better but some things may get worse. This
> means disabling HWC since we always have a non gralloc layer.
> 
> I think at this point we should get wider testing with the preference flipp
> before officially flipping it by default during feature freeze.

FWIW - This sounds like a good case to spin a try build and see if we can run a specialized Gaia UI Test run against this build to see if there's critical gfx regressions that result from this patch.

Comment 18

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

sure thing, but try is closed right now

Comment 19

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

https://tbpl.mozilla.org/?tree=Try&rev=1e0c76686a0a

Comment 20

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

(In reply to Andreas Gal :gal from comment #16)
> We would need to restore gralloc for this to make 1.2. That seems doable
> though. BenWa, can you comment?

why?

Comment 21

[Zac C (:zac)]

Hi Jason I can't run gaia-ui on this because the try build is not engineering/marionette enabled.

Comment 22

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #16)
> We would need to restore gralloc for this to make 1.2. That seems doable
> though. BenWa, can you comment?

If we try to do gralloc tiles for 1.2 were going to have to fix the gralloc lock regression. Turning on tiling over a regression will give us a win so that's not useful data. I think we need to fix the regressions for 1.2 and take our time to carefully tweak tiling. Jeff has some idea that Gralloc should be a mix of gralloc+textures which would match the android browser and perform better. If we rush tiling were likely to end in a state where tiling isn't polished enough to be better then 1.1 and now we're stuck with buffer rotation and tiling that are both worse then 1.1. IMO tiles need to wait for 1.3

Comment 23

[Jason Smith [:jsmith]]

(In reply to Zac C (:zac) from comment #21)
> Hi Jason I can't run gaia-ui on this because the try build is not
> engineering/marionette enabled.

Ack okay - didn't realize that. We should get a bug file to generating try builds with marionette enabled though. i'll do that.

QA Wanted - Do some exploratory testing around the daily smoketests to see if there's any critical issues that result from this patch.

Build for testing is here - https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-1e0c76686a0a/try-unagi/.

Comment 24

[Andreas Gal :gal]

Comment on attachment 802496 [details] [diff] [review]
enable tiles pref for b2g

a=me, assuming this had sufficient basic testing that this won't burn our tree and make trunk unusable

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest failures.
https://hg.mozilla.org/mozilla-central/rev/5b9ad615d2e1

https://tbpl.mozilla.org/php/getParsedLog.php?id=27720824&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=27720616&tree=Mozilla-Central

Comment 26

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

Attachment: flip_tiles_pref.patch

R6 had two tests that unexpectedly started passing, R9 has gradients with off-by-one color values.

Comment 27

[Andreas Gal :gal]

Brad, if I understand the dependencies here correctly then bug 909887 will break the FFOS homescreen. In other words we can't flip the pref until that and any other major regression is addressed.

Comment 28

[Mandyam Vikram]

Looks like the tiled layers pref is turned on Moz-central. I'm seeing it in the build I synced to last night. HWC is disabled with this.
Also, does this apply to other use cases such as video, camera playback?

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/57a03f46e0b2

Comment 30

[Jason Smith [:jsmith]]

Looks like given that this landed, we no longer need to do testing pre-landing. If regressions come up, we'll see it tomorrow's smoketest run.

Comment 31

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

(In reply to Andreas Gal :gal from comment #27)
> Brad, if I understand the dependencies here correctly then bug 909887 will
> break the FFOS homescreen. In other words we can't flip the pref until that
> and any other major regression is addressed.

no bug 909887 is a regression from turning APZC on

Comment 32

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

(In reply to Mandyam Vikram from comment #28)
> Looks like the tiled layers pref is turned on Moz-central. I'm seeing it in
> the build I synced to last night. HWC is disabled with this.
> Also, does this apply to other use cases such as video, camera playback?

Yes, this change applies to all apps.

(In reply to Jason Smith [:jsmith] from comment #30)
> Looks like given that this landed, we no longer need to do testing
> pre-landing. If regressions come up, we'll see it tomorrow's smoketest run.


Thanks, please ping me if there are any regressions.

Comment 33

[Mandyam Vikram]

Some preliminary observations (eyeballing on a 7x27 QRD device without any detail testing):
- Homescreen looks ok 
- Without detailed profiling, there is improvement in scrolling in Contacts (maybe not 60 FPS yet)over the earlier build at the beginning of the week
- Ditto on Gallery app scroll

Comment 34

[Sotaro Ikeda [:sotaro]]

When tiling is enabled on b2g device, it always renders by using OpenGL, HwComposer rendeirng is aborted with the following log.
-----------
D/HWComposer(  143): ThebesLayerComposite Layer doesn't have a gralloc buffer
D/HWComposer(  143): Render aborted. Nothing was drawn to the screen
D/HWComposer(  143): ThebesLayerComposite Layer doesn't have a gralloc buffer
D/HWComposer(  143): Render aborted. Nothing was drawn to the screen

Comment 35

[Benoit Girard (:BenWa)]

(In reply to Sotaro Ikeda [:sotaro] from comment #34)

That's expected. See comment 14, 2nd bullet.

Comment 36

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

Does this patch affect Fennec? I see the patch in regression ranges for several branches as a possible cause for a checkerboarding regression. For example:

Regression: Mozilla-Inbound - Robocop Checkerboarding Real User Benchmark - Android 4.0.4 - 249% increase
---------------------------------------------------------------------------------------------------------
    Previous: avg 8.829 stddev 1.380 of 12 runs up to revision feea0997db36
    New     : avg 30.777 stddev 1.775 of 12 runs since revision 9fd3e98cf2f8
    Change  : +21.948 (249% / z=15.909)
    Graph   : http://mzl.la/18Syabr

Regression: Fx-Team - Robocop Checkerboarding Real User Benchmark - Android 4.0.4 - 234% increase
-------------------------------------------------------------------------------------------------
    Previous: avg 9.226 stddev 1.161 of 12 runs up to revision bdf495121827
    New     : avg 30.791 stddev 1.313 of 12 runs since revision a98569f21abe
    Change  : +21.565 (234% / z=18.573)
    Graph   : http://mzl.la/1eJxudg

Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=bdf495121827&tochange=a98569f21abe

Comment 37

[Diego Wilson [:diego]]

Can we revert this change while bug 915673 is investigated? It's affecting bring up of Hwc on JB (bug 911391) and will affect power measurements in v1.2 in general.

Comment 38

[Benoit Girard (:BenWa)]

(In reply to Mark Finkle (:mfinkle) from comment #36)
> Does this patch affect Fennec?

It really shouldn't. I doubled check. I could be wrong so I recommend having someone break in TiledLayerBuffer.h.

(In reply to Diego Wilson [:diego] from comment #37)
> and will affect power measurements in
> v1.2 in general.

Can you quantify or share a way to measure power? The GFX team is hoping to switch to tiling everywhere (and ideally not just for scrolling) including b2g in some future release. Understanding the power impact of tiles is key.

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to Benoit Girard (:BenWa) from comment #38)
> 
> Can you quantify or share a way to measure power? The GFX team is hoping to
> switch to tiling everywhere (and ideally not just for scrolling) including
> b2g in some future release. Understanding the power impact of tiles is key.

b2g preformance team made a small hw and tool to measure power consumption in realtime from actual b2g phone. Jon Hylands showed a demo in b2g work week. I think that video playback(youtube) in web browser is the important use case to measure power. Diego know better about it.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Finkle (:mfinkle) from comment #36)
> Does this patch affect Fennec? I see the patch in regression ranges for
> several branches as a possible cause for a checkerboarding regression. For
> example:
> 
> Regression: Mozilla-Inbound - Robocop Checkerboarding Real User Benchmark -
> Android 4.0.4 - 249% increase
> -----------------------------------------------------------------------------
> ----------------------------
>     Previous: avg 8.829 stddev 1.380 of 12 runs up to revision feea0997db36
>     New     : avg 30.777 stddev 1.775 of 12 runs since revision 9fd3e98cf2f8
>     Change  : +21.948 (249% / z=15.909)
>     Graph   : http://mzl.la/18Syabr
> 
> Regression: Fx-Team - Robocop Checkerboarding Real User Benchmark - Android
> 4.0.4 - 234% increase
> -----------------------------------------------------------------------------
> --------------------
>     Previous: avg 9.226 stddev 1.161 of 12 runs up to revision bdf495121827
>     New     : avg 30.791 stddev 1.313 of 12 runs since revision a98569f21abe
>     Change  : +21.565 (234% / z=18.573)
>     Graph   : http://mzl.la/1eJxudg
> 
> Changeset range:
> http://hg.mozilla.org/integration/fx-team/
> pushloghtml?fromchange=bdf495121827&tochange=a98569f21abe

I believe bug 912806 is the guilty one here. See https://bugzilla.mozilla.org/show_bug.cgi?id=912806#c27

Comment 41

[Kartikaya Gupta (email:[e-mail])]

This is still in the tree, right? Please only reopen this bug if you actually back it out.

Comment 42

[Mandyam Vikram]

Can someone please clarify the status of tiled rendering for 1.3? As mentioned previously, if we don't use HWC (due to non usage of gralloc bufers), this will negatively impact power consumption.

Comment 43

[Chris Lord [:cwiiis]]

(In reply to Mandyam Vikram from comment #42)
> Can someone please clarify the status of tiled rendering for 1.3? As
> mentioned previously, if we don't use HWC (due to non usage of gralloc
> bufers), this will negatively impact power consumption.

So a patch has gone in that limits tiled layer usage to scrollable layers (when tiles are enabled at all, that is) - So if we enable it now, it will only have an impact while a scrollable layer is visible on the screen (this is not the case when just the home-screen is visible, for example).

A lot of apps will always have tiled layers visible though, but would this affect power usage unless we're recompositing? And if we're recompositing, don't we just want the best performance?

I don't know what our decision is on tiles in 1.3, but just adding this information here.

Comment 44

[Andreas Gal :gal]

APZC without tiles is not feasible since we lose buffer rotation and repainting is too slow.

Mandyam, my understanding from previous conversations is that power only matters for video playback and camera. Both don't scroll, so this bug won't affect them in any way. Is that correct?

Comment 45

[Mandyam Vikram]

(In reply to Andreas Gal from https://bugzilla.mozilla.org/show_bug.cgi?id=887819#c44)

> Mandyam, my understanding from previous conversations is that power only matters for video playback and
> camera. Both don't scroll, so this bug won't affect them in any way. Is that correct?

Will tiled rendering be effective for AZPC use cases only? Can you please describe which use cases it would affect? Is tiled rendering then planning to be turned on for 1.3? 

We do need to use HWC for camera and video playback. We haven't gotten good power numbers for the other use cases for 1.3 yet.

Comment 46

[Benoit Girard (:BenWa)]

Tiling would not be used for video or camera so those would not be impacted by using tiling.

Comment 47

[Diego Wilson [:diego]]

(In reply to Benoit Girard (:BenWa) from comment #46)
> Tiling would not be used for video or camera so those would not be impacted
> by using tiling.

What about WebGL? IIRC the performance of WegGL was much better with HWC.

Comment 48

[Andreas Gal :gal]

As BenWa said, tiling would be used only scrolling layers, so no WebGL, no Camera, no Video playback. Mostly app content (scrolling contacts) and browser etc.

Comment 49

[Mandyam Vikram]

Is it ready to be turned on for 1.3 then? Has  tiled rendering been profiled (v/s current implementation on 1.3)?

Comment 50

[Chris Lord [:cwiiis]]

In case it isn't already assumed, turning tiles on really requires turning APZC on too (bug 909877).