917416 - App RenderFrameParent needs to be Opaque

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: Settings app layer tree

Currently GetOpaqueRegion is not implemented in nsDisplayRemote. Because of this fullscreen apps are not treated as opaque by the b2g process so it continues to retain and draw the background. This is bad for memory usage and overfill.

The one gotcha is the Homescreen app is actually transparent and uses the background provided by the b2g app so we can't make all RenderFrameParent opaque.

I have attached the layer tree of the Settings app. Notice that the first ThebesLayerComposite spans the entire screen (status bar + background) when it really should just be the status bar.

Fixing this drops our overfill rate by nearly 100% of the screen and will save a screen size buffer.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Always make RenderFrameParent - Breaks Homescreen

Here is the patch to make RenderFrameParent opaque but this regresses the Homescreen so we need to indicate that an app is opaque or not.

Comment 2

[Benoit Girard (:BenWa)]

Alright looks like a solution to this bug would be very useful but I'm not the right person to drive this bug. Can we find an owner for this? Fixing this properly will let us reclaim two screen size buffers (double buffers). That's a substantial memory saving on HiDPI devices.

Comment 3

[Andreas Gal :gal]

Is this fixed everywhere else except the homescreen?

Comment 4

[Benoit Girard (:BenWa)]

With this patch yes. We would need a way for apps to either opt-in or opt-out of transparency which will be the bulk of the work for this bug.

Comment 5

[Andreas Gal :gal]

Why can we not communicate out of the app what the layer tree looks like and what the opaque region is?

Comment 6

[Benoit Girard (:BenWa)]

The content thread of the system b2g process which builds the main layer tree never sees the layer tree of the content app. The b2g content thread knows it has a RenderFrameParent and has to assume that it's transparent and thus must allocate a background for RenderFrameParent. Since from the perspective of the b2g content thread the content app can post update directly to the compositor it can't be notified that the layer tree is opaque because that could easily change independently.

We can cull in the compositor which I will land soon, but this bug is about finding a way to avoid having the background in our layer tree at all and saving the memory/painting.

Comment 7

[Andreas Gal :gal]

Yes, thats actually what I meant. We can track and cull the visible region in the compositor and update that back to the content process, right?

Comment 8

[Benoit Girard (:BenWa)]

We can't because layer trees are updated separately. So if the content app update its tree to be transparent suddenly then the compositor would be left without the b2g background until the b2g main thread is notified and sends an update for that content.

I think the best thing to do here is to have apps either opt in or out of transparency. This is similar to how flash handles wmode opaque/transparent:
http://helpx.adobe.com/flash/kb/flash-object-embed-tag-attributes.html#main_Using_Window_Mode__wmode__values_

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Sounds like we really need this for Flatfish, and with higher res devices showing up in 1.3, nominating for 1.3.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

:BenWa just showed me how, in e.g. the Settings app, we constantly overpaint at >= 200% because we paint the wallpaper even though the app hides it completely.

Since it seems that the only app that wouldn't hide the wallpaper completely is the Homescreen app, could we move the wallpaper to be painted by the Homescreen app, so that the wallpaper is only ever painted when the Homescreen app is foreground?

Comment 11

[Benoit Girard (:BenWa)]

I've been thinking about bjacob's suggestion and I think it's a great idea. We don't have a great use case for transparent apps so I think for now we should just disallow it.

The changes required to fix this + pull the wall paper into the homescreen app are trivial. I'm going to bring this up tomorrow in the gfx meeting.

Comment 12

[[:fabrice] Fabrice Desré]

Moving the wallpaper to the homescreen app will work for now, but will cause pain down the road when we enable 3rd party homescreens (that won't happen for 1.3 unless some miracle happens, but will be in 1.4).
This is because the wallpaper is stored in settings and only certified apps can access settings. For that to work with 3rd party homescreen we would need to let privileged apps access settings with a better granularity level than the all on/all off we have now, and I'm not sure when this will be possible.

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Fix homescreen

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 806118 [details] [diff] [review]
Always make RenderFrameParent - Breaks Homescreen

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

Why can't we just cull the extra drawing inside the compositor?

At the very least we'd also need to check this code to return eTransparencyOpaque - http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/PuppetWidget.h#142

This will ensure that sub-process layer trees will always actually be opaque, see http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5038

I'm also not actually sure if this will always be true. I *think* it's ok for the current use-cases (b2g apps and e10s tabs), but if we ever try to do <iframe> in a child process, then it won't work.

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> Comment on attachment 806118 [details] [diff] [review]
> Always make RenderFrameParent - Breaks Homescreen
> 
> Review of attachment 806118 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why can't we just cull the extra drawing inside the compositor?

I'm more worried about having an extra active layer all the time that's entirely useless. Mostly worried about the memory but also the overdraw.

Comment 16

[Matt Woodrow (:mattwoodrow)]

I guess that's ok.

If we ever decide we want to support transparent sub-processes, then we'll need to add a way for them to opt into that.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: Always make RenderFrameParent v 1.1

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8334011 [details] [diff] [review]
Always make RenderFrameParent v 1.1

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

Actually, can you please update the comment in PuppetWidget too. Explain that marking them as opaque is a performance optimization, and that we'll need a way for them to opt in to transparency if they ever need it.

Comment 19

[Benoit Girard (:BenWa)]

gaia changes should land first:
http://git.io/2iZN7Q

Waiting on the merge then I can land the c++ changes.

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
Sure thing

Comment 20

[Benoit Girard (:BenWa)]

Attachment: Always make RenderFrameParent opaque v1.2

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Github pullrequest r=gal

Comment 22

[Kevin Grandon :kgrandon]

Gaia patch landed in master: https://github.com/mozilla-b2g/gaia/commit/d14af806d87a2ca6098ca0a011256cb3b1d4cf9a

Comment 23

[Benoit Girard (:BenWa)]

Checkin for https://bugzilla.mozilla.org/attachment.cgi?id=8334099 when tree re-opens.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/08eaaed722ef

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08eaaed722ef

Comment 26

[[:fabrice] Fabrice Desré]

That regressed the app launch animation in gaia. Instead of a black background we now have the wallpaper + icons zooming in. Should we back out?

Comment 27

[Benoit Girard (:BenWa)]

(In reply to Fabrice Desré [:fabrice] from comment #26)
> That regressed the app launch animation in gaia. Instead of a black
> background we now have the wallpaper + icons zooming in. Should we back out?

Can we instead file a regression bug and modify the animation to properly provide a background like it expects?

Comment 28

[[:fabrice] Fabrice Desré]

(In reply to Benoit Girard (:BenWa) from comment #27)
> (In reply to Fabrice Desré [:fabrice] from comment #26)
> > That regressed the app launch animation in gaia. Instead of a black
> > background we now have the wallpaper + icons zooming in. Should we back out?
> 
> Can we instead file a regression bug and modify the animation to properly
> provide a background like it expects?

If you prefer, but that needs to be fixed for 1.3

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Keyboard apps also need to support transparency. I think the fix here was a bit of a hack.

We tried to fix overcompositing in bug 907048. I guess that didn't work (enough)?

We should be able to avoid the big layer upload just by ensuring the homescreen app puts a solid color generating a ColorLayer behind every app that it expects to be opaque.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Ah, damn :( Didn't think of the keyboard.

It probably is a bit of a hack, but I think it's the right direction to be taking.

Not being able to reason about visibility for a node in the layer tree is a fairly big limitation, and it's apparently costing us here.

Forcing a ColorLayer behind it would work (and maybe is the right idea for now), but it feels like we're just avoiding the issue.

Can't we have an attribute on the subprocess frame that opts in/out of transparency?

Comment 31

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> We tried to fix overcompositing in bug 907048. I guess that didn't work
> (enough)?

This also reduces memory usage. It prevents us from retaining a full screen layer (+ the back buffer).

Comment 32

[Benoit Girard (:BenWa)]

I'm going to backout the gecko change. The gaia change can stay which means that for now the Homescreen app will remain opaque even if the system process can't infer it.

Comment 33

[Benoit Girard (:BenWa)]

... and waiting for inbound to open.

Comment 34

[Benoit Girard (:BenWa)]

Backed out until we support opt-in transparent iframes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33f136f3274b

Comment 35

[Matt Woodrow (:mattwoodrow)]

Attachment: Make remote frames opaque if moz-opaque is set

Does this work?

You need to set the 'moz-opaque' attribute on the remote frames you want to be opaque, probably something similar to what was done for bug 815886.

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Do we need to reopen this bug now the that it's being backed out?

Comment 37

[Carsten Book [:Tomcat]]

btw also was backedout from mozilla-central https://hg.mozilla.org/mozilla-central/rev/33f136f3274b

Comment 38

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kevin Grandon :kgrandon from comment #22)
> Gaia patch landed in master:
> https://github.com/mozilla-b2g/gaia/commit/
> d14af806d87a2ca6098ca0a011256cb3b1d4cf9a

Sorry guys, I have to backout this commit too. 

master revert: bae6b97c8f5d0ebb443b6da989bb8ae3dc9cc647

This commit have caused background of home screen app mis-positioned in some situations. I have screenshot for that.

REOPEN because all the patches of this bug is backed out everywhere.

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: 2013-12-01-23-56-12.png

Noted the upper-left, the tail of the Fox is cut-off a bit.

Sad fox :'(.

Comment 40

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: 2013-12-01-23-56-23.png

The background image is centered, causing (again) the tail of the fox being cut-off.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #38)
> (In reply to Kevin Grandon :kgrandon from comment #22)
> > Gaia patch landed in master:
> > https://github.com/mozilla-b2g/gaia/commit/
> > d14af806d87a2ca6098ca0a011256cb3b1d4cf9a
> 
> Sorry guys, I have to backout this commit too. 
> 
> master revert: bae6b97c8f5d0ebb443b6da989bb8ae3dc9cc647
> 
> This commit have caused background of home screen app mis-positioned in some
> situations. I have screenshot for that.
> 
> REOPEN because all the patches of this bug is backed out everywhere.

Just FYI, this backout regressed the datazilla cold_load_time for many apps.  Hopefully we can fix the problems and reland at some point.  Thanks!

Comment 42

[Benoit Girard (:BenWa)]

This is high on my list. I'll try my best :)

Comment 43

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ben Kelly [:bkelly] from comment #41)
> Just FYI, this backout regressed the datazilla cold_load_time for many apps.
> Hopefully we can fix the problems and reland at some point.  Thanks!

I don't understand how a homescreen patch could affect cold launch time of other apps, but data speaks, so...

Comment 44

[Benoit Girard (:BenWa)]

That's because it's not a homescreen patch. It lets us optimize remote frames.

Comment 45

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Benoit Girard (:BenWa) from comment #44)
> That's because it's not a homescreen patch. It lets us optimize remote
> frames.

I thought comment 41 was specifically talking about the Gaia patch.... never mind.

Comment 46

[Mason Chang [Inactive] [:mchang]]

Attachment: moz-opaque.patch

Merges in the home-screen fixes and sets moz-opaque to true on all newly created browser frames. Seems to work, on the settings app, during scroll, we overlay at around ~180 instead of ~280. Same with the Messages app. For now, the strategy seems to be make all apps opaque and fix the homescreen to be unique. Is this right?

In regards to cold launch times, I could not reproduce the regressions on an otoro device for the Settings app.

Raw data of cold launch times using b2gperf:
With patch:

2013-12-06 14:42:52 | B2GPerfRunner | INFO | Results for settings, cold_load_time: median:759, mean:766, std: 28, max:897, min:736, all:793,760,771,785,760,748,783,897,736,772,748,771,750,759,757,744,758,802,749,751,754,741,774,753,752,759,770,760,764,766

Without patch:
2013-12-06 14:55:02 | B2GPerfRunner | INFO | Results for settings, cold_load_time: median:758, mean:772, std: 52, max:976, min:741, all:789,744,757,767,786,743,751,760,747,759,748,756,976,746,948,786,748,747,756,771,750,762,761,741,767,798,746,767,761,744

Comment 47

[Matt Woodrow (:mattwoodrow)]

I think having the homescreen opaque is okay actually, but we need to make sure the keyboard is transparent.

Comment 48

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #47)
> I think having the homescreen opaque is okay actually, but we need to make
> sure the keyboard is transparent.

I disagree. It would be hard to fix attachment 8340882 [details] and attachment 8340883 [details] if home screen is made opaque, unless we could convince UX that we could

1) make status bar stay opaque forever (style is currently being updated at bug 936201)
2) stop 3rd-party home screens from using the phone-wide background settings

*then* we could remove background from system app and move it to home screen app.

Comment 49

[Mason Chang [Inactive] [:mchang]]

Removed 1.3 blocking status for the following reasons:

1) This shouldn't be a regression since we're been painting the parent on 1.2.
2) Unsure if solution to make home screen is opaque while UX reviews occur per comment 48.

Please renom if this is invalid.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Girard (:BenWa) from comment #31)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > We tried to fix overcompositing in bug 907048. I guess that didn't work
> > (enough)?
> 
> This also reduces memory usage. It prevents us from retaining a full screen
> layer (+ the back buffer).

If you put a ColorLayer behind the <browser>, that gives you the same memory usage reduction since we don't have to store anything for the ColorLayer and we won't create any other ThebesLayers in that process.

Comment 51

[Benoit Girard (:BenWa)]

That would work just fine. Maybe a bit more error/regression prone. Ideally we'd be smart enough to avoid compositing this layer for both OGL and HWC. We'd still need to know when to provide that layer behind the app i.e. knowing when the app is transparent.

Comment 52

[Mason Chang [Inactive] [:mchang]]

Attachment: Settings Layer Tree w/o Patch

Comment 53

[Mason Chang [Inactive] [:mchang]]

Attachment: Settings Layer Tree w/ Opacity

Comment 54

[Mason Chang [Inactive] [:mchang]]

Attachment: https://www.dropbox.com/s/ewfllagr12zfvp8/settings_with_opacity.mov

Video showing keyboard transparency

Comment 55

[Mason Chang [Inactive] [:mchang]]

I did some analysis on the settings layer tree with Benwa and with the opacity patch. A few things stand out:

1) The thebes layer for the status bar is now correctly only 20px instead of 480px, which was the original intent. 
2) Due to comment 29 and comment 47, I double checked the keyboard. Since the keyboard isn't a browser app (I think?), it's not being labeled as opaque and it works as intended. Please see the video in Comment 54.
3) By marking the browser frame as moz-opaque, we now have a black ColorLayer over a Thebes Layer, was this what you meant ROC in comment 50? Otherwise, can you please explain a little more by what you meant?

Thanks!

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we need a moz-opaque attribute. I think if you have a full-screen app with a solid color background on the <iframe> element containing the app, we should optimize that to a ColorLayer in the master process and we should get the performance benefits you're looking for.

Comment 57

[Mason Chang [Inactive] [:mchang]]

Attachment: Settings App Layer Tree

Comment 58

[Mason Chang [Inactive] [:mchang]]

Attachment: opaque-parent.patch

I went hunting through the layout code and wasn't sure if this was way off or spot on. It works and keeps the homescreen app transparent while at the same time, most Gaia apps are detected as not transparent. The browser app isn't detected correctly at this time and still overpaints.

Another option I was exploring, but couldn't quite find yet, was to read the 'background-color' CSS property for the frame and parse it for a solid RGB color rather than an image. Is this a better route than this current patch? 

Sorry if I'm way off, this is my first patch to Gecko's layout code. If you have a few pointers on what I should be changing, I'd appreciate it.

Thanks!

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8348411 [details] [diff] [review]
opaque-parent.patch

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

::: layout/ipc/RenderFrameParent.cpp
@@ +1154,5 @@
> +{
> +  const nsStyleBackground* background = mFrame->StyleBackground();
> +  bool isTransparent = background->IsTransparent();
> +  if (!isTransparent) {
> +    return GetBounds(aBuilder, aSnap);

I don't understand how this helps. If the element has a solid background then there should be an nsDisplayBackgroundColor which is opaque and has the same effect as this code.

::: widget/xpwidgets/PuppetWidget.h
@@ +140,2 @@
>    virtual nsTransparencyMode GetTransparencyMode() MOZ_OVERRIDE
> +  { return eTransparencyOpaque; }

Does this have any effect? I don't see how.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would like to know why the approach in comment #56 doesn't work.

Comment 61

[Mason Chang [Inactive] [:mchang]]

I don't know if the approach in Comment #56 won't work, I was just taking a stab at it based off the previous work. With the patch, I was able to make the AppRenderFrameParent opaque without a moz-opaque attribute or changing the home screen. 

I just have to dig into the layer code and see if I can find out where to stick in a Color Layer. If you have any pointers on where to start, I'd appreciate it. Thanks!

Comment 62

[Matt Woodrow (:mattwoodrow)]

Roc's suggestion is to make no c++ changes at all. Just add a css background style that is a solid color to the remote frame element witihn gaia.

That should result in an nsDisplayBackgroundColor display item being created, which turns into an opaque ColorLayer and should hopefully trigger the optimizations we want here.

Comment 63

[Mason Chang [Inactive] [:mchang]]

Attachment: Settings App Layer Tree w/ Black Opaque Background

Comment 64

[Mason Chang [Inactive] [:mchang]]

Attachment: example.zip

Just a brain dump before the holidays. An example showing the layers that occur due to the html on the parent process and child process. With this example, on desktop, we create two layers. We could put an opaque black div at the top of the divs in Gaia, but that would involve a large rewrite of Gaia CSS, which assumes that the <div id=appframe> is always a child of <div id=windows>. Inserting a div between the <appframe> and <iframe> does not get the optimization we want.

Comment 65

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ Painting Dump

Comment 66

[Mason Chang [Inactive] [:mchang]]

Attachment: ADB Logcat w/ Painting Dump Enabled

Attached are the MOZ_DUMP_PAINT_LIST=1 and layer tree. I've tried multiple things to get the ColorLayer to be inserted, but it's never quite worked.

1) The iFrame has a solid color background already.
2) The App's body has a solid color background.
3) I've tried putting a div as a parent of the iframe that is a solid color background. This didn't matter.
4) The iframe's parent already had a solid background color as well. 
5) I've been able to get the correct color layer if i remove the top padding, but that shifts the app up and the status bar covers the top of the app. 

Any ideas why the color layer isn't being inserted? From my naive stepping through the Layout code, it's because the layer isn't being treated as opaque because there is no GetOpaqueRegion implemented. Thanks!

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks for that!

The reason the BackgroundColor of the <iframe> has not been turned into a ColorLayer is that it's been combined into a ThebesLayer with the content of the 20px-high status bar.

There are various ways to fix that but I'm not sure which one is best:
#1a Force creation of a ContainerLayer around the <iframe> (with a remote subdocument) so any background can't be combined into a layer with any other content from outside the <iframe>.
#1b If an <iframe> with a remote subdocument has a background color, force that background color into its own layer.
#2 Force creation of a ContainerLayer around the status bar so its content can't combine with any other content.
#3 Teach FrameLayerBuilder to not combine a solid-color region with disjoint non-solid-color content.

#1a and #1b are less fragile than #2 or #3. #3 may be worth doing on its own. #1a is easier to implement than #1b. #1a has the problem that if you add an outline or border or box-shadow to the <iframe> the optimization will stop working.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: force <iframe> background color into its own layer

Does this help?

Comment 69

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ iframe Patch

We get the Color Layer we want here, but the problem is that the status bar Thebes Layer still has a height=480 texture if I'm reading the layer tree correctly. I think however, this may be related to bug 952590. I'll have to verify that. Otherwise, is that what you expected?

Also, just wondering what were you looking for in the paint dump? I couldn't find anything in the paint dump showing us painting the status bar thebes layer (0x48af5c00). Maybe a better question is, what do you generally look for with MOZ_DUMP_PAINT_LIST=1? Thanks! I ask because I'd like to learn to diagnose and fix these myself :)

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mason Chang [:mchang] from comment #69)
> We get the Color Layer we want here, but the problem is that the status bar
> Thebes Layer still has a height=480 texture if I'm reading the layer tree
> correctly. I think however, this may be related to bug 952590. I'll have to
> verify that. Otherwise, is that what you expected?

The important thing is that the visible region of the ThebesLayer is only 20 high. That should let us do the compositing optimization we need to do.

The texture may be bigger because this ThebesLayer was bigger in the past.

> Also, just wondering what were you looking for in the paint dump? I couldn't
> find anything in the paint dump showing us painting the status bar thebes
> layer (0x48af5c00). Maybe a better question is, what do you generally look
> for with MOZ_DUMP_PAINT_LIST=1? Thanks! I ask because I'd like to learn to
> diagnose and fix these myself :)

Good question. I actually wrote down my analysis and then lost it due to some user error :-).

First I looked for the Remote display item and the RefLayer associated with it. That's the subprocess app. The BackgroundColor display item just before it must be the background color of the <iframe> (and its color and size confirm that). Then the display list dump tells us which layer that item ended up in, "layer=0x471e6860" in your case. Then we look at the other items that ended up in that layer that have non-empty visible rect. They are:
BackgroundColor 0x43c27010() bounds(0,0,19200,1200) visible(0,0,19200,1200) componentAlpha(0,0,0,0) clip(0,0,19200,28800)  uniform (rgba 0,0,0,255) layer=0x471e6860
Background 0x43c2e678() bounds(14520,120,1500,960) visible(14520,120,1500,960) componentAlpha(0,0,0,0) clip(0,0,19200,1200) layer=0x471e6860
Background 0x43e80708() bounds(13320,120,960,960) visible(13320,120,960,960) componentAlpha(0,0,0,0) clip(0,0,19200,1200)  layer=0x471e6860
Text 0x47deca48() bounds(16200,0,1740,1200) visible(16200,0,1740,1200) componentAlpha(16200,0,1740,1200) clip(0,0,19200,1200)  layer=0x471e6860
Text 0x4637a3a0() bounds(18000,240,960,900) visible(18000,240,960,900) componentAlpha(18000,240,960,900) clip(0,0,19200,1200)  layer=0x471e6860
These are all in the top 20 pixels of the screen so clearly they're the status bar.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With this patch, bug 907048's optimization should be working so the ColorLayer we just generated is not actually composited. Can you verify that? If so, we're done here (with bug 952590 fixed).

Comment 72

[Mason Chang [Inactive] [:mchang]]

Just tried with bug 952590, that only cleans up the thebes layer if the entire region was not-visible. Looks like something in thebes doesn't clean up the texture if the visible area is resized to something smaller but greater than 0.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK. So the patches here should fix up the overdraw but not the ThebesLayer memory usage. Let's split that issue off into a separate bug since it has nothing to do with apps being opaque.

Comment 74

[Mason Chang [Inactive] [:mchang]]

Attachment: logcat w/ rendering layers

Comment 75

[Mason Chang [Inactive] [:mchang]]

Attachment: renderLayer.patch

Patch to test if a Color Layer is being composited. Looks like we're still compositing the new Color Layer. Am I reading this right? Digging some more.

Comment 76

[Mason Chang [Inactive] [:mchang]]

Attachment: renderLayer.patch

Added some more instrumentation. Looks like the ColorLayer optimization in bug 907048 only checks the children in the same container layer. The Color Layer we've inserted is in a different ContainerLayer (0x45773800) than the RefLayer's ContainerLayer Parent(0x4665c800). Since the RefLayer isn't labeled as Opaque, we bail out and don't optimize the ColorLayer we want here. At least that's how I'm reading it. Log attached.

Comment 77

[Mason Chang [Inactive] [:mchang]]

Attachment: logcat w/ renderLayer.patch

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch in https://bug907048.bugzilla.mozilla.org/attachment.cgi?id=805124 skips any layer that is covered by the opaque rect, so it should be able to skip layer 0x45773800.

I think the problem is that GetOpaqueRect isn't drilling down through the RefLayer because the RefLayer has a non-identity transform (translation down by 20px). It's not hard to improve the optimization to handle an integer translation such as we have here. If there's an integer translation, we just need to translate the opaque rect by that amount. Can you do that?

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3410ff3c525f

Comment 80

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3410ff3c525f

Comment 81

[Mason Chang [Inactive] [:mchang]]

Attachment: opaque.patch

Patch with instrumentation. Final patch will have the printfs deleted :)

Comment 82

[Mason Chang [Inactive] [:mchang]]

Attachment: Render Tree w/ opaque.patch

Attached is the output from the opaque.patch. We see that we skip rendering the ContainerLayer with the ColorLayer(0x476f5c00) and ColorLayer::RenderLayer never gets called. Woot! We also underpaint a bit with FPS counters. We still have to move the homescreen background to the homescreen app. Before we land these, I want to test for:

1) Startup time regressions
2) Fabrice's App Loading Animation regression in comment 26, although I can't seem to reproduce this on master.

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please add -p to your hg diff options so that function names get included in the patch.

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also it's helpful to set the checkin message in the patch before requesting review.

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8360146 [details] [diff] [review]
opaque.patch

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

And please take out the printfs :-)

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +48,4 @@
>    return false;
>  }
>  
> +static bool shouldSkipLayer(Layer* aLayer, gfxMatrix& matrix, bool is2d)

capital S. aMatrix, aIs2D.

Don't call this shouldSkipLayer, since that suggests we might skip rendering this layer if we return true.

In fact I think it's confusing to create an out of line function here at all, since it's not clear how to divide functionality between this function and GetOpaqueRect. Let's just do this all in GetOpaqueRect.

@@ +55,5 @@
> +  }
> +
> +  // Allow only simple integer translations
> +  if (!aLayer->GetEffectiveTransform().IsIdentity()) {
> +    return matrix.HasNonIntegerTranslation();

The logic here is incorrect. You still need to check GetEffectiveOpacity and GetMaskLayer.

Also, there's no need for the IsIdentity check here. An identity matrix is an integer translation.

@@ +81,2 @@
>    // Just bail if there's anything difficult to handle.
> +  if (shouldSkipLayer(aLayer, matrix, is2d)) {

Check is2d, GetMaskLayer, GetEffectiveOpacity and matrix.HasNonIntegerTranslation right here.

@@ +96,5 @@
> +
> +    // Translate our opaque region to cover the child
> +    if (is2d) {
> +      gfxPoint point = matrix.GetTranslation();
> +      result.MoveBy(point.x, point.y);

is2d must be true or we wouldn't reach here. You can unconditionally do result.MoveBy(int(point.x), int(point.y)).

However, you need to do this for the aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE case too.

Comment 86

[Mason Chang [Inactive] [:mchang]]

Attachment: opaque.patch

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8360739 [details] [diff] [review]
opaque.patch

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +56,5 @@
>  GetOpaqueRect(Layer* aLayer)
>  {
>    nsIntRect result;
> +  gfxMatrix aMatrix;
> +  bool aIs2D = aLayer->GetBaseTransform().Is2D(&aMatrix);

No 'a' for is2D and matrix since these are just local variables now

@@ +79,5 @@
>    }
> +
> +  // Translate our opaque region to cover the child
> +  gfxPoint point = aMatrix.GetTranslation();
> +  result.MoveBy(point.x, point.y);

Use an explicit cast to int for point.x/point.y

Comment 88

[Mason Chang [Inactive] [:mchang]]

Attachment: opaque.patch

Carrying r+ from roc. Waiting to land until bug 960325 gets resolved.

Comment 89

[Mason Chang [Inactive] [:mchang]]

Attachment: opaque.patch

Carrying r+ from roc. Updated for mozilla-central tip.

Comment 90

[Mason Chang [Inactive] [:mchang]]

With homescreen patch from bug 960325, here are the startup cold launch times:

w/ all patches
2014-02-03 10:17:53,650 B2GPerfRunner INFO    | Results for settings, cold_load_time: median:2563, mean:2602, std: 272, max:3276, min:1498, all:1498,2425,2920,2939,2554,3276,2704,2556,2502,2496,2737,2504,2532,2463,2713,2705,2563,2690,2563,2726,2738,2519,2704,2472,2667,2672,2357,2805,2550,2512

w/o Gecko Patch, w/ Gaia Patch
2014-02-03 10:24:40,088 B2GPerfRunner INFO    | Results for settings, cold_load_time: median:2670, mean:2640, std: 268, max:3020, min:1410, all:1410,2582,2499,2613,2760,2606,2619,2825,2640,2691,2311,2560,3020,2508,2721,2754,2602,2759,2846,2843,2542,2527,2753,2927,2793,2650,2635,2696,2812,2716

w/o Gecko Patch, w/o Gaia Patch
2014-02-03 10:31:27,599 B2GPerfRunner INFO    | Results for settings, cold_load_time: median:2753, mean:2724, std: 308, max:3123, min:1434, all:1434,2608,3123,2964,2656,2761,2944,2768,3005,2745,2825,2402,2689,3008,2568,2553,2714,2471,3104,2419,2634,3001,2741,2860,2550,2686,2765,2874,2804,3049

Gecko:
166610:067123df4f77

Gaia:
899afd58f3c6d93dbc36ca2745068a8fc1cc2829

Looks like we actually reduce start up times with these patches.

Comment 91

[Mason Chang [Inactive] [:mchang]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=460db19b49d0

Comment 92

[Mason Chang [Inactive] [:mchang]]

Attachment: black_flashing.png

I finally finished all the dependent bugs, but don't want to land moving the homescreen into the homescreen app since we add an extra layer by moving the background there. This bug puts performance back in line for the homescreen app. However, with APZ enabled, when we checkerboard, we flash black as shown in the image instead of flashing white, which is the normal checkerboarding pattern. If we disable checkerboarding or APZ, we no longer have the issue. Is it ok to land this and file a follow up bug to fix the black flashing, otherwise all the patches will probably go stale while trying to waiting to fix checkerboarding. Thanks!

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It would be better to fix the black checkerboarding if we can do that quickly. Do you know what causes it?

Comment 94

[Mason Chang [Inactive] [:mchang]]

I don't know what causes it exactly, I think it's just general checkerboarding nor am I confident that I can fix it quickly. I noticed black flashing on a Tarako device without any modification, so I think it's actually already happening on some devices when checkerboarding occurs.

Comment 95

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess you should land then.

Comment 96

[Mason Chang [Inactive] [:mchang]]

Attachment: opaque.patch

Made checkin-friendly.

Comment 97

[Mason Chang [Inactive] [:mchang]]

Successful try - https://tbpl.mozilla.org/?tree=Try&rev=4d2e2c0a7dad

Comment 98

[Ryan VanderMeulen [:RyanVM]]

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

Comment 99

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 100

[Mason Chang [Inactive] [:mchang]]

Just in case we get a startup regression since that happened earlier. Tested on a hamachi

w/o patch
Results for Settings, cold_load_time: median:3621, mean:3519, std: 413, max:3723, min:1358, all:1358,3622,3652,3625,3723,3576,3482,3635,3608,3624,3645,3644,3682,3397,3647,3575,3540,3577,3590,3632,3639,3573,3654,3704,3233,3599,3620,3608,3701,3422
Results for Settings, cold_load_time: median:3634, mean:3526, std: 464, max:3941, min:1110, all:1110,3941,3672,3609,3663,3525,3678,3663,3416,3646,3654,3580,3683,3614,3690,3513,3665,3574,3638,3465,3706,3613,3624,3693,3564,3642,3645,3173,3630,3516

w/ patch
Results for Settings, cold_load_time: median:3645, mean:3556, std: 458, max:3937, min:1150, all:1150,3937,3667,3623,3734,3587,3553,3643,3682,3685,3693,3625,3725,3604,3675,3610,3576,3640,3685,3666,3637,3630,3723,3621,3267,3426,3673,3650,3667,3647
Results for Settings, cold_load_time: median:3624, mean:3525, std: 460, max:3926, min:1116, all:1116,3926,3651,3619,3655,3496,3601,3595,3617,3653,3342,3638,3646,3632,3656,3630,3681,3526,3725,3594,3441,3656,3585,3656,3589,3614,3666,3580,3315,3675

Comment 101

[Mason Chang [Inactive] [:mchang]]

Very weird situation. Initially, when we developed this patch, we had to move the background image into the homescreen app (bug 960325). However, this caused lots of visual regressions such as flashing black when the homescreen app was OOM killed. We tried to have the background in both the system app + homescreen app, but that caused visual regressions if the background was a unique picture since we'd have duplicates. I backed out the gaia homescreen patches and left this 917416 in gecko.... and magically we don't have a black background in the homescreen app (or I can't reproduce). I asked around and we're not quite sure why.. but it seems like an accident.

I have backed out bug 970284, bug 960325, and bug 975811, but kept this in. We still get the performance increases we expect, but my instinct says something is fishy and we're not sure why it's working. Leave bug 917416 in for now, but if we see black backgrounds instead of a homescreen wallpaper, back this out too.