Closed Bug 887819 Opened 6 years ago Closed 6 years ago

Investigate using the tiled layers backend

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cwiiis, Assigned: blassey)

References

Details

(Whiteboard: [UCID: Browser1, FT:Browser, KOI:P1])

Attachments

(2 files, 2 obsolete files)

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.
(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.
(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?
(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.
Depends on: 747811
I think bug 859929 might also come into play here if we turn this on. Linking it here just in case.
Depends on: 859929
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).
Blocks: 894333
Attached patch Enable tiles on b2g (obsolete) — Splinter Review
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.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Attachment #792112 - Flags: review?(bgirard)
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 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?
(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.
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 on attachment 792112 [details] [diff] [review]
Enable tiles on b2g

Postponing review until we have an argument to support switching this on.
Attachment #792112 - Flags: review?(bgirard)
Assignee: chrislord.net → blassey.bugs
Attachment #792112 - Attachment is obsolete: true
Attachment #802493 - Flags: review?(bgirard)
Attachment #802493 - Flags: review?(bgirard) → review+
Attached patch enable tiles pref for b2g (obsolete) — Splinter Review
Attachment #802496 - Flags: review?(gal)
Attachment #802496 - Flags: review?(bgirard)
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.
We would need to restore gralloc for this to make 1.2. That seems doable though. BenWa, can you comment?
(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.
sure thing, but try is closed right now
(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?
Hi Jason I can't run gaia-ui on this because the try build is not engineering/marionette enabled.
(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
(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/blassey@mozilla.com-1e0c76686a0a/try-unagi/.
Keywords: qawanted
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
Attachment #802496 - Flags: review?(gal)
Attachment #802496 - Flags: review?(bgirard)
Attachment #802496 - Flags: review+
R6 had two tests that unexpectedly started passing, R9 has gradients with off-by-one color values.
Attachment #802496 - Attachment is obsolete: true
Attachment #803339 - Flags: review?(bgirard)
Attachment #803339 - Flags: review?(bgirard) → review+
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.
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?
https://hg.mozilla.org/mozilla-central/rev/57a03f46e0b2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Keywords: qawanted
(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
(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.
Whiteboard: [UCID: Browser1, FT:Browser, KOI:P1]
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
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
Depends on: 915673
(In reply to Sotaro Ikeda [:sotaro] from comment #34)

That's expected. See comment 14, 2nd bullet.
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
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 911391
(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.
(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.
(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
This is still in the tree, right? Please only reopen this bug if you actually back it out.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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.
(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.
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?
(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.
Tiling would not be used for video or camera so those would not be impacted by using tiling.
(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.
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.
Is it ready to be turned on for 1.3 then? Has  tiled rendering been profiled (v/s current implementation on 1.3)?
In case it isn't already assumed, turning tiles on really requires turning APZC on too (bug 909877).
You need to log in before you can comment on or make changes to this bug.