App RenderFrameParent needs to be Opaque

RESOLVED FIXED in mozilla28

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: BenWa, Assigned: mchang)

Tracking

({perf})

Trunk
mozilla28
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [c=progress p=4 s= u=])

Attachments

(12 attachments, 20 obsolete attachments)

3.31 KB, text/plain
Details
2.74 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
BenWa
: review+
Details | Review | Splinter Review
4.46 KB, patch
Details | Diff | Splinter Review
6.02 KB, text/plain
Details
3.46 KB, patch
tnikkel
: review+
mchang
: feedback+
Details | Diff | Splinter Review
6.65 KB, text/plain
Details
4.18 KB, patch
Details | Diff | Splinter Review
6.60 KB, text/plain
Details
8.25 KB, text/plain
Details
592.92 KB, image/png
Details
1.44 KB, patch
mchang
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 806116 [details]
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.
(Reporter)

Comment 1

5 years ago
Created attachment 806118 [details] [diff] [review]
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.
(Reporter)

Comment 2

5 years ago
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

5 years ago
Is this fixed everywhere else except the homescreen?
(Reporter)

Comment 4

5 years ago
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

5 years ago
Why can we not communicate out of the app what the layer tree looks like and what the opaque region is?
(Reporter)

Comment 6

5 years ago
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

5 years ago
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?
(Reporter)

Comment 8

5 years ago
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_
Sounds like we really need this for Flatfish, and with higher res devices showing up in 1.3, nominating for 1.3.
blocking-b2g: --- → 1.3?
: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?
(Reporter)

Comment 11

5 years ago
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.
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.
(Reporter)

Comment 13

5 years ago
Created attachment 833011 [details] [diff] [review]
Fix homescreen
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #833011 - Flags: review?(21)
(Reporter)

Updated

5 years ago
Attachment #806118 - Flags: review?(matt.woodrow)

Updated

5 years ago
Attachment #833011 - Flags: review?(21) → review+
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.
(Reporter)

Comment 15

5 years ago
(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.
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.
(Reporter)

Comment 17

5 years ago
Created attachment 8334011 [details] [diff] [review]
Always make RenderFrameParent v 1.1
Attachment #806118 - Attachment is obsolete: true
Attachment #806118 - Flags: review?(matt.woodrow)
Attachment #8334011 - Flags: review?(matt.woodrow)
Attachment #8334011 - Attachment is patch: true
Attachment #8334011 - Flags: review?(matt.woodrow) → review+
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.
(Reporter)

Comment 19

5 years ago
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
(Reporter)

Comment 20

5 years ago
Created attachment 8334099 [details] [diff] [review]
Always make RenderFrameParent opaque v1.2
Attachment #8334011 - Attachment is obsolete: true
Attachment #8334099 - Flags: review+
(Reporter)

Comment 21

5 years ago
Created attachment 8334827 [details] [review]
Github pullrequest r=gal
Attachment #833011 - Attachment is obsolete: true
Attachment #8334827 - Flags: review+
(Reporter)

Comment 23

5 years ago
Checkin for https://bugzilla.mozilla.org/attachment.cgi?id=8334099 when tree re-opens.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08eaaed722ef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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?
(Reporter)

Comment 27

5 years ago
(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?
(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
Depends on: 941900
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.
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?
(Reporter)

Comment 31

5 years ago
(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).

Updated

5 years ago
Depends on: 941912
(Reporter)

Updated

5 years ago
Blocks: 942788

Updated

5 years ago
No longer blocks: 942788
Depends on: 942788
(Reporter)

Comment 32

5 years ago
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.
(Reporter)

Comment 33

5 years ago
... and waiting for inbound to open.
(Reporter)

Comment 34

5 years ago
Backed out until we support opt-in transparent iframes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33f136f3274b
Created attachment 8338224 [details] [diff] [review]
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.
Attachment #8338224 - Flags: feedback?(bgirard)
Do we need to reopen this bug now the that it's being backed out?
btw also was backedout from mozilla-central https://hg.mozilla.org/mozilla-central/rev/33f136f3274b
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8340882 [details]
2013-12-01-23-56-12.png

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

Sad fox :'(.
Created attachment 8340883 [details]
2013-12-01-23-56-23.png

The background image is centered, causing (again) the tail of the fox being cut-off.
(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!
(Reporter)

Comment 42

5 years ago
This is high on my list. I'll try my best :)
(Assignee)

Updated

5 years ago
Keywords: perf
Whiteboard: [c=progress p=4 s= u=]
(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...
(Reporter)

Comment 44

5 years ago
That's because it's not a homescreen patch. It lets us optimize remote frames.
(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.
(Assignee)

Comment 46

5 years ago
Created attachment 8344048 [details] [diff] [review]
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
Attachment #8344048 - Flags: feedback?(matt.woodrow)
Flags: needinfo?(matt.woodrow)
I think having the homescreen opaque is okay actually, but we need to make sure the keyboard is transparent.
Flags: needinfo?(matt.woodrow)
(Assignee)

Updated

5 years ago
Blocks: 948154
blocking-b2g: 1.3? → 1.3+

Updated

5 years ago
Whiteboard: [c=progress p=4 s= u=] → [c=progress p=4 s= u=1.3]
(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.

Updated

5 years ago
blocking-b2g: 1.3+ → -
Whiteboard: [c=progress p=4 s= u=1.3] → [c=progress p=4 s= u=]
(Assignee)

Comment 49

5 years ago
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.
(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.
(Reporter)

Comment 51

5 years ago
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.
(Assignee)

Comment 52

5 years ago
Created attachment 8347477 [details]
Settings Layer Tree w/o Patch
(Assignee)

Comment 53

5 years ago
Created attachment 8347478 [details]
Settings Layer Tree w/ Opacity
(Assignee)

Comment 55

5 years ago
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!
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
(Assignee)

Comment 57

5 years ago
Created attachment 8348405 [details]
Settings App Layer Tree
Attachment #8347477 - Attachment is obsolete: true
Attachment #8347478 - Attachment is obsolete: true
Attachment #8347479 - Attachment is obsolete: true
(Assignee)

Comment 58

5 years ago
Created attachment 8348411 [details] [diff] [review]
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!
Attachment #8348411 - Flags: feedback?(roc)
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.
Attachment #8348411 - Flags: feedback?(roc) → feedback-
I would like to know why the approach in comment #56 doesn't work.
(Assignee)

Comment 61

5 years ago
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!
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.
(Assignee)

Comment 63

5 years ago
Created attachment 8349796 [details]
Settings App Layer Tree w/ Black Opaque Background
(Assignee)

Comment 64

5 years ago
Created attachment 8350796 [details]
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.
(Assignee)

Comment 65

5 years ago
Created attachment 8357258 [details]
Layer Tree w/ Painting Dump
(Assignee)

Comment 66

5 years ago
Created attachment 8357260 [details]
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!
Flags: needinfo?(roc)
(Assignee)

Updated

5 years ago
Attachment #8357260 - Attachment description: log.txt → ADB Logcat w/ Painting Dump Enabled
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.
Flags: needinfo?(roc)
Created attachment 8357564 [details] [diff] [review]
force <iframe> background color into its own layer

Does this help?
Attachment #8357564 - Flags: feedback?(mchang)
(Assignee)

Comment 69

5 years ago
Created attachment 8357917 [details]
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 :)
Flags: needinfo?(roc)
(Assignee)

Updated

5 years ago
Attachment #8357564 - Flags: feedback?(mchang) → feedback+
(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.
Flags: needinfo?(roc)
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).
Attachment #8357564 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 72

5 years ago
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.
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.
(Assignee)

Comment 74

5 years ago
Created attachment 8358625 [details]
logcat w/ rendering layers
(Assignee)

Comment 75

5 years ago
Created attachment 8358628 [details] [diff] [review]
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.
Flags: needinfo?(roc)
(Assignee)

Comment 76

5 years ago
Created attachment 8358641 [details] [diff] [review]
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.
Attachment #8358628 - Attachment is obsolete: true
(Assignee)

Comment 77

5 years ago
Created attachment 8358642 [details]
logcat w/ renderLayer.patch
Attachment #8358625 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 958727
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?
Flags: needinfo?(roc)
(Assignee)

Updated

5 years ago
Assignee: bgirard → mchang
(Assignee)

Comment 81

5 years ago
Created attachment 8360146 [details] [diff] [review]
opaque.patch

Patch with instrumentation. Final patch will have the printfs deleted :)
Attachment #8360146 - Flags: review?(roc)
(Assignee)

Comment 82

5 years ago
Created attachment 8360148 [details]
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.
Please add -p to your hg diff options so that function names get included in the patch.
Also it's helpful to set the checkin message in the patch before requesting review.
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.
Attachment #8360146 - Flags: review?(roc) → review-
(Assignee)

Comment 86

5 years ago
Created attachment 8360739 [details] [diff] [review]
opaque.patch
Attachment #8360146 - Attachment is obsolete: true
Attachment #8360739 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Depends on: 960325
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
Attachment #8360739 - Flags: review?(roc) → review+
(Assignee)

Comment 88

5 years ago
Created attachment 8361228 [details] [diff] [review]
opaque.patch

Carrying r+ from roc. Waiting to land until bug 960325 gets resolved.
Attachment #8360739 - Attachment is obsolete: true
Attachment #8361228 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8344048 - Flags: feedback?(matt.woodrow)
(Assignee)

Updated

5 years ago
Depends on: 960683
(Assignee)

Comment 89

5 years ago
Created attachment 8368981 [details] [diff] [review]
opaque.patch

Carrying r+ from roc. Updated for mozilla-central tip.
Attachment #8361228 - Attachment is obsolete: true
Attachment #8368981 - Flags: review+
(Assignee)

Comment 90

5 years ago
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.
No longer depends on: 970284
(Assignee)

Comment 92

5 years ago
Created attachment 8373822 [details]
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!
Flags: needinfo?(roc)
It would be better to fix the black checkerboarding if we can do that quickly. Do you know what causes it?
Flags: needinfo?(roc)
(Assignee)

Comment 94

5 years ago
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.
(Assignee)

Comment 96

5 years ago
Created attachment 8379797 [details] [diff] [review]
opaque.patch

Made checkin-friendly.
Attachment #8338224 - Attachment is obsolete: true
Attachment #8340882 - Attachment is obsolete: true
Attachment #8340883 - Attachment is obsolete: true
Attachment #8348411 - Attachment is obsolete: true
Attachment #8349796 - Attachment is obsolete: true
Attachment #8350796 - Attachment is obsolete: true
Attachment #8357258 - Attachment is obsolete: true
Attachment #8357260 - Attachment is obsolete: true
Attachment #8368981 - Attachment is obsolete: true
Attachment #8338224 - Flags: feedback?(bgirard)
Attachment #8379797 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [c=progress p=4 s= u=][leave open] → [c=progress p=4 s= u=]
(Assignee)

Comment 100

5 years ago
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
(Assignee)

Comment 101

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.