Sushil Reporter
	Description • 10 years ago

Thebes layer is being used for Black background in following 5 critical use cases. A Color layer can be used instead. Using a Thebes layer is very expensive for graphics/display pipeline as compared to a Color layer due to following reasons:
1. RGBA Thebes layer needs Gralloc buffers and it is full screen in this case. So it leads to unnecessary use of memory (think about 256 MB devices).
2. Full screen RGBA data (4 bytes per pixel) needs to be fetched by display driver for a full screen Thebes layer. Where as Color layer can be displayed by simple SOLID_FILL operation in driver which is not expensive.
3. Data fetching and extra processing for a Thebes layer will impact power numbers in these critical use cases.

Case 1: Portrait Video playback (video controls are not visible)
E/HWComposer(  210): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Tr=0 Blend=0x100 Flags=0x40
E/HWComposer(  210): HWC layer[1]::ImageLayerComposite: dst=[0 265 480 535] src=[0.000000 0.000000 320.000000 180.000000] Tr=0 Blend=0x100 Flags=0x0
E/HWComposer(  210): Frame rendered

Case 2: Landscape Video playback (video controls are not visible)
E/HWComposer(  210): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 800.000000 480.000000] Tr=4 Blend=0x100 Flags=0x40
E/HWComposer(  210): HWC layer[1]::ImageLayerComposite: dst=[15 0 465 799] src=[0.000000 0.000000 1280.000000 720.000000] Tr=4 Blend=0x100 Flags=0x0
E/HWComposer(  210): Frame rendered

Case 3: Camcorder recording
E/HWComposer(  210): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Tr=0 Blend=0x100 Flags=0x40
E/HWComposer(  210): HWC layer[1]::ImageLayerComposite: dst=[0 1 480 800] src=[0.000000 24.000000 719.000000 456.000000] Tr=4 Blend=0x100 Flags=0x0
E/HWComposer(  210): HWC layer[2]::ThebesLayerComposite: dst=[21 26 317 785] src=[0.000000 0.000000 296.000000 759.000000] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  210): Frame rendered

Case 4: Camcorder preview
E/HWComposer(  210): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Tr=0 Blend=0x100 Flags=0x40
E/HWComposer(  210): HWC layer[1]::ImageLayerComposite: dst=[0 1 480 800] src=[0.000000 24.000000 719.000000 456.000000] Tr=4 Blend=0x100 Flags=0x0
E/HWComposer(  210): HWC layer[2]::ThebesLayerComposite: dst=[0 0 480 141] src=[0.000000 0.000000 480.000000 141.000000] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  210): HWC layer[3]::ThebesLayerComposite: dst=[164 632 464 785] src=[0.000000 0.000000 300.000000 153.000000] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  210): Frame rendered

Case 5: Camera preview
E/HWComposer(  210): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Tr=0 Blend=0x100 Flags=0x40
E/HWComposer(  210): HWC layer[0]::ImageLayerComposite: dst=[0 0 480 800] src=[0.000000 48.000000 640.000000 432.000000] Tr=4 Blend=0x100 Flags=0x0
E/HWComposer(  210): HWC layer[1]::ThebesLayerComposite: dst=[0 0 480 141] src=[0.000000 0.000000 480.000000 141.000000] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  210): HWC layer[2]::ThebesLayerComposite: dst=[29 641 164 776] src=[0.000000 0.000000 135.000000 135.000000] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  210): HWC layer[3]::ThebesLayerComposite: dst=[56 668 137 749] src=[0.000000 0.000000 81.000000 81.000000] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  210): HWC layer[4]::ThebesLayerComposite: dst=[164 632 464 785] src=[0.000000 0.000000 300.000000 153.000000] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  210): Frame rendered

	Sushil Reporter
	Comment 1 • 10 years ago

Benwa, who should be right poc for this bug?

	Benoit Girard (:BenWa)
	Comment 2 • 10 years ago

I can help. Lets start by getting a display list for the app providing the layer.

Build with either debug or 'export B2G_DUMP_PAINTING=1', then run with user_pref("layout.display-list.dump", true);. Each process will dump a display list when it paints. It will explain what we're trying to break and how it gets assign to a layer.

Typically when we get a thebes layer it's because we have something on the layer other then a trivial solid color covering up everything.

Lets get the dumps collected to understand whats causing the layer.

	Sushil Reporter
	Comment 3 • 10 years ago

Benwa, is it MOZ_DUMP_PAINTING=1 ? Because I do not see "B2G_DUMP_PAINTING" anywhere in gecko code. So layer tree is being dumped only in b2g process. Is it sufficient ?

	Sushil Reporter
	Comment 4 • 10 years ago

I have attached layer tree dumped in b2g process for Case 1 of Comment 0.

	Benoit Girard (:BenWa)
	Comment 5 • 10 years ago

It's a build option that needs to be in the .userconfig which will be detected by default-gecko-config. You will need to do a full rebuild.

What you attached is a layers dump, not a display list.

	Sushil Reporter
	Comment 6 • 10 years ago

For Case 1, I have attached the ADB logs, which has display list dump from b2g process (pid: 212) and content process (pid: 1075). Logs begin when the Video is played from the Video app list of videos and then few seconds of playback in Portrait mode.

	Sushil Reporter
	Comment 7 • 10 years ago

Updated the ADB logs file having display list dump.

	Benoit Girard (:BenWa)
	Comment 8 • 10 years ago

This log intermixes output from different processes, has many layers dump and display list of vastly different things. Can we get a log of just the display list for a case where we want to be getting a color layer and we're not. From it we just have to look up which Display Item are going into the thebes layer and we have to remove them from the app.

	Sushil Reporter
	Comment 9 • 10 years ago

Benwa, I have attached the display list and layer tree dump of the frame in Case 1.

	Benoit Girard (:BenWa)
	Comment 10 • 10 years ago

Comment on attachment 8461024 [details]
Case1 frame display list and layer tree.

Display list looks fine. I'd imagine we should be building a color layer for b3f51850 but we're not. Any ideas tn?

	Timothy Nikkel (:tnikkel)
	Comment 11 • 10 years ago

Comment on attachment 8461024 [details]
Case1 frame display list and layer tree.

Looking at the code I would expect this to get a color layer.

We should hit this case for the topmost of the three background color items http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?force=1#2330 because the first two are not visible. And that should mean that we hit this if http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?force=1#2048

	Benoit Girard (:BenWa)
	Comment 12 • 10 years ago

Also the display items should be a whole number of device pixel which is what we ran into the past.

Sushil can you dig deeper into those callsites?

	Sushil Reporter
	Comment 13 • 10 years ago

Yes, we hit the 2 places mentioned in Comment 11 and specifically we are hitting the else block: http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?force=1#2069. So why do we still have the visible full screen Thebes layer in frame? I am again listing the visible layers in Portrait video playback frame (Case 1). HwcComposer2D just drops the bottom 2 Color layers because they are entirely hidden under the full screen opaque Thebes layer above them.

HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800] src=[0 0 480 764] Color=ff000000 Blend=0x100
HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0 0 480 800] Color=ff14120e Blend=0x100
HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800] Tr=0 Blend=0x100
HWC layer[1]::ImageLayerComposite: dst=[0 265 480 535] src=[0 0 320 180] Tr=0 Blend=0x100
E/HWComposer(  215): Frame rendered

This bug should be easily reproducible on v2.0 Flame device. You can debug it on Flame.

	Timothy Nikkel (:tnikkel)
	Comment 14 • 10 years ago

Then you must be getting a different layer tree then the one in the displaylist/layer dump in the second attachment. So it's hard to tell what is going on without the corresponding displaylist/layer for this new situation.

	Sushil Reporter
	Comment 15 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #14)
> Then you must be getting a different layer tree then the one in the
> displaylist/layer dump in the second attachment. So it's hard to tell what
> is going on without the corresponding displaylist/layer for this new
> situation.

The attachment 8461024 [details] mentioned in Comment 10 is for HWC layer list provided by me in Comment 13. I can see these 2 Color layers in b2g layer tree of that attachment:

I/Gecko   (  212):         ColorLayerComposite (0xab39fc00) [shadow-visible=< (x=0, y=0, w=480, h=764); >] [visible=< (x=0, y=0, w=480, h=764); >] [opaqueContent] [color=rgba(0, 0, 0, 1)]
...
I/Gecko   (  212):           ColorLayerComposite (0xad2aa000) [shadow-visible=< (x=0, y=0, w=480, h=800); >] [visible=< (x=0, y=0, w=480, h=800); >] [opaqueContent] [color=rgba(14, 18, 20, 1)]

HwcComposer2D parses the layer tree in b2g process hence it is listing Color layers:
HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800] src=[0 0 480 764] Color=ff000000 Blend=0x100
HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0 0 480 800] Color=ff14120e Blend=0x100

Since these color layers are totally hidden under full screen Thebes layer, HwcComposer2D drops them so they are not a concern. The aim of this bug is: why do we have this full screen Thebes layer, where we could have a Color layer instead ? This bug should be easily reproducible on Flame v2.0 device, if it helps.

	Timothy Nikkel (:tnikkel)
	Comment 16 • 10 years ago

Are we talking about the content or the parent process layer tree? attachment 8461024 [details] only has the display list dump for the content process so that's what I was looking at.

	Sushil Reporter
	Comment 17 • 10 years ago

[Blocking Requested - why for this release]:

	bhavana bajaj [:bajaj]
	Comment 18 • 10 years ago

Blocking as this is raised as a blocker for CAF CC keeping in mind info in comment #0 and given it is easily reproducible in 2.0 per comment #15. Milan, can you please help with an assignee here or NI sushil if we need more information from CAF here.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 19 • 10 years ago

Let's continue the conversation here so that we can get to the answer quicker. Sushil, if you have an answer to comment 16, let us know.  BenWa/Jeff when one of you gets a chance, see if you can reproduce on a Flame and get the data Timothy is asking for (pretty sure he doesn't have a Flame.)

	Sushil Reporter
	Comment 20 • 10 years ago

(In reply to Milan Sreckovic [:milan] from comment #19)
> Sushil, if you have an answer to comment 16, let us know. 

Yes, the display list is from content process and I do not see corresponding display list from b2g process in ADB logs, I only see Layer tree gets dumped by b2g process.

I built with 'export B2G_DUMP_PAINTING=1' and set ("layout.display-list.dump", true);
I don't know why b2g process does not dump display list here, I am not familiar with painting phase code. I am attaching ADB logs, which are collected as soon as Video starts and few seconds of Video playback in Case 1. Please check around line # 2686 in the attachment "Case1_adb_logs.txt".

	Sushil Reporter
	Comment 21 • 10 years ago

Here is attachment "Case1_adb_logs.txt"

	Benoit Girard (:BenWa)
	Comment 22 • 10 years ago

This is the layer tree I'm seeing from trunk which looks worse than what was originally reported.

	Sushil Reporter
	Comment 23 • 10 years ago

Previously reported layer tree in this bug was for simpler use case: Portrait Video playback (when video controls are not visible), which is Case 1 as per Comment 0. Camera has more number of layers, so layer tree has to be bigger.

	Benoit Girard (:BenWa)
	Comment 24 • 10 years ago

Alright I took a broader look at the camera app and the CSS being applied is very complicated. There's 5,000 lines of CSS for just the camera app!

By making changes to the CSS I can get the thebes layer to become a color layer and by making further changes I can get the color layer to be marked opaque and not be composited at all further saving some overdraw.

The CSS is causing unusual sizing of the parent layer with negative (and likely fractional device pixel values). I think the right fix here is to start by auditing the CSS used by the camera app.

To get a color layer instead of a thebes we just have to fix how the background-color is applied. It shouldn't be applied to the html element, just the body element. If this is confusing see http://phrogz.net/css/htmlvsbody.html and https://developer.mozilla.org/en-US/docs/Fast_Graphics_Performance_With_HTML#Make_the_content_of_scrollable_sub_frames_opaque .

diff --git a/apps/camera/style/app.css b/apps/camera/style/app.css
index 6834fae..7047e27 100644
--- a/apps/camera/style/app.css
+++ b/apps/camera/style/app.css
@@ -7,6 +7,9 @@ body {
   padding: 0;
   margin: 0;
   overflow: hidden;
+}
+
+body{
   background-color: black;
 }

But if we simplify the CSS further we can get the color-layer to be marked as non visible which is what we should aim for to reduce overdraw.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 25 • 10 years ago

Taking this to try and make necessary corrections to the CSS.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 26 • 10 years ago

(In reply to Benoit Girard (:BenWa) from comment #24)
> The CSS is causing unusual sizing of the parent layer with negative (and
> likely fractional device pixel values). I think the right fix here is to
> start by auditing the CSS used by the camera app.

Its likely here that you're talking about the <video> element. Unfortunately, there are lots of transforms/positioning styles applied to rotate/scale it to fill the viewport.

> But if we simplify the CSS further we can get the color-layer to be marked
> as non visible which is what we should aim for to reduce overdraw.

Does this mean that the <video> element is *not* completely covering the background? (e.g. <1px around the edges) If that is the case, we could adjust our positioning code to always add an extra 1px width/height to the <video> element to ensure that the background gets *completely* covered.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 28 • 10 years ago

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/e39fbfaa9c4803aa0640588750fa758c7eb578da

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 29 • 10 years ago

(In reply to Justin D'Arcangelo [:justindarc] from comment #26)
> (In reply to Benoit Girard (:BenWa) from comment #24)
> > The CSS is causing unusual sizing of the parent layer with negative (and
> > likely fractional device pixel values). I think the right fix here is to
> > start by auditing the CSS used by the camera app.
> 
> Its likely here that you're talking about the <video> element.
> Unfortunately, there are lots of transforms/positioning styles applied to
> rotate/scale it to fill the viewport.
> 
> > But if we simplify the CSS further we can get the color-layer to be marked
> > as non visible which is what we should aim for to reduce overdraw.
> 
> Does this mean that the <video> element is *not* completely covering the
> background? (e.g. <1px around the edges) If that is the case, we could
> adjust our positioning code to always add an extra 1px width/height to the
> <video> element to ensure that the background gets *completely* covered.

Benoit: If this is the case, let's open up a follow-up bug to ensure the color-layer gets marked as non visible. Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

v2.0: https://github.com/mozilla-b2g/gaia/commit/3cd323212bf4d8868849a9ad25dd975968b6e463

	Sushil Reporter
	Comment 31 • 10 years ago

I checked on v2.0 tip, the issue is still present. The background layer is still coming as Thebes layer instead of Color layer, in all the 5 use cases mentioned in Comment 0. I confirmed, the build has fix from this bug. You can check by dumping the Gecko layer tree in b2g process.

Also observed that there is unnecessary painting of Color/Thebes layer in all these 5 use cases. Since HWC does not compose anything below a full screen Opaque layer, so I did not report in Comment 0. So those layers should never be painted. You can check this too in layer tree in b2g process.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 32 • 10 years ago

(In reply to Sushil from comment #31)
> I checked on v2.0 tip, the issue is still present. The background layer is
> still coming as Thebes layer instead of Color layer, in all the 5 use cases
> mentioned in Comment 0. I confirmed, the build has fix from this bug. You
> can check by dumping the Gecko layer tree in b2g process.
> 
> Also observed that there is unnecessary painting of Color/Thebes layer in
> all these 5 use cases. Since HWC does not compose anything below a full
> screen Opaque layer, so I did not report in Comment 0. So those layers
> should never be painted. You can check this too in layer tree in b2g process.

Does the patch in Bug 1049198 resolve this? We opened Bug 1049198 to follow-up with helping reduce the number of layers in the Camera app. The patch in 1049198 reduces the number of layers significantly, but hasn't landed due to ongoing investigations as to why we're not hitting the HWC.

	Sushil Reporter
	Comment 33 • 10 years ago

(In reply to Justin D'Arcangelo [:justindarc] from comment #32)
> patch in 1049198 reduces the number of layers significantly, but hasn't
> landed due to ongoing investigations as to why we're not hitting the HWC.

OK then, I am closing this bug since it has dependency on newer bugs.

	Seth Miko [:SMiko]
	Comment 34 • 10 years ago

No STR is present to create test case to address bug.