1049198 - Camera/Video should use mark Color layer as [not visible] when covered by video

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Benoit Girard (:BenWa)]

+++ This bug was initially created as a clone of Bug #1040952 +++

Comment 1

[Benoit Girard (:BenWa)]

Can you also look into this issue. The original patch in bug 1040952 was incomplete. Let's continue the effort and build a proper patch here.

Comment 2

[Justin D'Arcangelo [:justindarc]]

(In reply to Benoit Girard (:BenWa) from comment #1)
> Can you also look into this issue. The original patch in bug 1040952 was
> incomplete. Let's continue the effort and build a proper patch here.

Sounds good. Let me try and understand the issue though. Is the problem that the <video> element is not covering the background completely?

Comment 3

[Benoit Girard (:BenWa)]

No, I don't think its that. I wasn't even looking into the video element.

Like I posted in bug 1040952 some styles are set on both HTML and BODY elements that should be there. Overall the CSS for the app is overly complex and is preventing a lot of platform optimizations. We need to simplify the positioning so that we can build a simpler easier layer tree.

Running with layers dump from the Developper Settings will help. The first thing to do is to remove the style that causing the root layer container layer to not match the screen exactly, then simplify the CSS causing color layer to not be mark [not visisble].

Comment 4

[Justin D'Arcangelo [:justindarc]]

Ok, I will take a look and see what we can do. The thing that jumped out at me as being a potential issue is the fact that we have to apply a CSS transform to our <video> element (or its container, I can't remember offhand) to rotate/position it to fill the screen. In some cases, however, its possible that there is a rounding issue (e.g. 799.5px width) when positioning the viewfinder. I think that may be at least a part of the issue here.

Unfortunately, most of the complexity in the CSS is necessary to meet UX requirements. But, I think we can probably do a better job at simplifying some of it. If we need to, I may ping someone from UX to see if we can relax some of the constraints (e.g.: the zoom bar needs to be XX pixels away from the mode switch in landscape orientation). Also, some of the complexity stems from the fact that we have to lock the app orientation to portrait and manually rotate the on-screen controls to preserve their on-screen locations.

Comment 5

[Benoit Girard (:BenWa)]

I don't know if we need to make user visible changes here. A quick look at the CSS showed that we had a lot of complexity to provide something that visually looked like it could be achieved through simpler CSS. I think we can achieve the same rendering while doing some simplification. At the very least in this bug we should try to get the same rendering but simplify the layer tree.

On the flame what I see visually is just a video layer + a top overlay + bottom overlay with no background visible. We should be able to get this exact rendering using only 2 layers (video + single overlay) or 3 layers (video, top overlay, bottom overlay)*.

* Counting from the app's RefLayer, excluding non visible layers & container layers.

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Benoit Girard (:BenWa) from comment #5)
> I don't know if we need to make user visible changes here. A quick look at
> the CSS showed that we had a lot of complexity to provide something that
> visually looked like it could be achieved through simpler CSS. I think we
> can achieve the same rendering while doing some simplification. At the very
> least in this bug we should try to get the same rendering but simplify the
> layer tree.
> 
> On the flame what I see visually is just a video layer + a top overlay +
> bottom overlay with no background visible. We should be able to get this
> exact rendering using only 2 layers (video + single overlay) or 3 layers
> (video, top overlay, bottom overlay)*.
> 
> * Counting from the app's RefLayer, excluding non visible layers & container
> layers.

I agree. I'll take a look and see what I can do.

Just so I understand, I just need to enable the "layers dump" in the dev settings? I assume that this dumps the info out to the logcat then? If so, is there any way to filter the logcat to just see the layer info? For example, when debugging JS, I can filter out everything but the JS console by running `adb logcat GeckoConsole:V *:S`

Comment 7

[Benoit Girard (:BenWa)]

I normally do adb logcat | grep Gecko. It's not too noisy I find. It's a bit hard to deal with the platform black box and how DOM+CSS is turned into a layer tree but we sadly don't have any better tools and getting this right is important for good performance.

Let me know if you're having trouble and I can walk you through some stuff on vidyo since it's not intuitive.

Comment 8

[David Flanagan [:djf]]

Justin: in addition to adding Math.ceil() to the code that calculates the CSS transform for the video element so that it covers the screen, you might also add a CSS clip attribute or something to cut out the parts of the video that would be off the screen. I've got no idea if that would help at all, but I have heard that having elements larger than the screen can cause performance issues.

Also remember that there is the fadein/fade out code on the viewfinder. Perhaps the fact that the opacity is animated causes the background color layer to remain visible. If you just turn off that fade code, does that fix the layers issue?  Is there a will-change attribute that you could remove?

Comment 9

[Preeti Raghunath(:Preeti)]

[Blocking Requested - why for this release]:

Comment 10

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

BenWa: Would it be possible for you to take a look and see what the improvement is with this patch? (still learning how to use the "dump layer tree" and not sure if i'm reading correctly)

Comment 11

[Justin D'Arcangelo [:justindarc]]

Attachment: layer-tree (master)

Comment 12

[Justin D'Arcangelo [:justindarc]]

Attachment: layer-tree (patch)

Comment 13

[Justin D'Arcangelo [:justindarc]]

BenWa: I've attached a before/after of the layer tree dumps for you to compare.

Comment 14

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8468737 [details] [review]
pull-request (master)

David: Please review. I've done as much as I can to reduce the layers. There's still one ThebesLayerComposite that I can't seem to get rid of. However, this patch should help reduce the overall power consumption and load on the GPU. Thanks!

Comment 15

[David Flanagan [:djf]]

Attachment: Use devicePixelRatio and Math.ceil() in the scaleToFill function

Justin, 

Do you think this patch would help to ensure that the video layer never ends up one device pixel smaller than the screen?  The layer dump shown in the parent bug looks like it has an image layer that starts at y=1 instead of y=0. This might help.

Comment 16

[David Flanagan [:djf]]

Attachment: layer tree with my scaling patch

If I run with the patch I attached but without Justin's scaling patch then the thebes layer at line 21 of justin's layer trees goes away.

Justin tried applying my patch with his patch and found that it did not make the layer go away.

So now I'm just kind of confused about all this.

Comment 17

[David Flanagan [:djf]]

Benoit: could you help out here with analysis of these three layer trees?

Comment 18

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #16)
> Created attachment 8468892 [details]
> layer tree with my scaling patch
> 
> If I run with the patch I attached but without Justin's scaling patch then
> the thebes layer at line 21 of justin's layer trees goes away.
> 
> Justin tried applying my patch with his patch and found that it did not make
> the layer go away.
> 
> So now I'm just kind of confused about all this.

It's a black box mystery. Ideally, we could have a patch that reduces the layer count *and* gets rid of the one Thebes layer (line 21). If for some reason we can't, I guess we just need to know which "fix" yields a larger improvement.

David: If we can get someone to test the battery/power consumption between the two patches, maybe that would be the best way to figure this out. I'm not sure if it makes it easier for CAF to test if you can put your patch in a GitHub PR?

Comment 19

[David Flanagan [:djf]]

Update: if I leave the overflow:none on the head and body and add it to hud.css like Justin's patch does, and then use my scaling patch, I get a hidden Thebes layer and a color layer. It seems that if I remove the overflow:hidden on the head and body, that undoes the benefit of my scaling patch.

It does seem weirdly intermittent, however, sometimes I get a visible Thebes layer and sometimes I get a visible color layer.  

Having some github troubles right now, but I'll attach a pull request.

Comment 20

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #19)
> Update: if I leave the overflow:none on the head and body and add it to
> hud.css like Justin's patch does, and then use my scaling patch, I get a
> hidden Thebes layer and a color layer. It seems that if I remove the
> overflow:hidden on the head and body, that undoes the benefit of my scaling
> patch.
> 
> It does seem weirdly intermittent, however, sometimes I get a visible Thebes
> layer and sometimes I get a visible color layer.  
> 
> Having some github troubles right now, but I'll attach a pull request.

Interesting. I was also seeing some intermittent layers so I tried only capturing the layer tree that seemed to be occurring the most frequent. Also, if you were using the rear camera, it's likely that when the auto-focus got triggered that you may have seen the layer used for the focus ring animation. Since that focus ring element is only added to the DOM when the camera is actually focusing, I tried capturing the layer tree when it was in-between focuses or just switched to the front camera (it doesn't have focus capabilities).

Comment 21

[David Flanagan [:djf]]

Attachment: scaling patch plus overflow:none in hud.css

Here's my scaling patch plus the line from Justin's patch that adds overflow:none to hud.js.  This patch (at least some of the time) changes the visible Thebes layer into a non-visible Thebes layer followed by a visible Color layer. I'm assuming that is a good thing, but I don't know for sure.

Comment 22

[David Flanagan [:djf]]

Attachment: layer tree produced by the pull request above

Comment 23

[David Flanagan [:djf]]

Jon,

The parent bug of this one (bug 1049086) is an urgent performance bug that we need to fix by the end of the day on Thursday. Could you help us out by measuring the power consumption of the camera (with just the viewfinder running and when recording video) on the master branch and on the master branch with each of the two pull requests attached here?

We're hoping that one or both of the PRs here have simplified the layer tree enough that they will have a non-trivial impact on power usage, but we can't really say without actually measuring it.

(Also setting needinfo for Mike since he knows the camera app and may also have access to a power harness.)

Comment 24

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8469033 [details] [review]
scaling patch plus overflow:none in hud.css

David, can you also remove the `will-change` declarations from my patch in your patch as well? It may help reduce those intermittent Thebes layers you're seeing. Also, in the cases where I removed them, the elements were fairly small in size so there was not any noticeable degradation in animation performance. I did keep the one in viewfinder for opacity because that's a full-screen element and it is already a layer because of the video.

Comment 25

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8469034 [details]
layer tree produced  by the pull request above

This layer tree looks good to me. There is one additional visible Thebes layer that I wasn't able to avoid either that is present here. However, it is not a full-screen layer so it shouldn't be too costly (I think it stems from the new slideable toggle switch styles).

Comment 26

[David Flanagan [:djf]]

Here are some more notes from my investigations of this bug:

1) All of my tests were while recording a video because I think that is what the parent bug was about.

2) In all the layer trees I've seen, there is a RefLayer->ContainerLayer->ThebesLayer hierarchy, and it is the Thebes layer (usually at line 21 of the tree dump) that I've been focusing on.

3) Justin's PR simplifies the layer tree a lot but the Thebes layer remains visible. We have:

             ThebesLayerComposite (0xaa08fc00) [shadow-visible=< (x=0, y=0, w=480, h=854); >] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=854); >]
               TiledContentHost (0xaa816200)

4) My patch seems to have most of the same simplifications, but converts the Thebes layer so that it is not visible. But it adds a visible ColorLayer as a sibling of the Thebes layer. I don't know if that is better or worse:

             ThebesLayerComposite (0xb02cc000) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible]
             ColorLayerComposite (0xb097c000) [shadow-visible=< (x=0, y=0, w=480, h=854); >] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [color=rgba(0, 0, 0, 1)]

4a) If I use my scaling patch without Justin's patch at all, then I get a non-visible ThebesLayer, and a visible color layer as above, but the container layer that contains extends beyond the screen.

4b) If I used my scaling patch along with Justin's patch then the Thebes layer seems to come back. Removing the overflow:none from the head and body somehow undoes the effect of my scaling patch.

5) I tried removing background-color on the body element in app.css, and I think that made the ColorLayer go away. But it also meant that when the viewfinder faded in or out we saw a white background instead of black, which is unacceptable UX.  I tried setting the black background on the html element instead of the body, but this just made the color layer come back, I think.

6) Around this time in my investigation I started seeing a lot of transient behavior: my patch would work, then not work (i.e. give a visible ThebesLayer). Then it would start working again. I never got this figured out.

7) Removing all the opacity code from viewfinder.css didn't seem to make a difference, or maybe it even made the visible Thebes layer come back, but this was confusing because of the intermittent nature of the results I was seeing at this point.

8) I tried testing whether tap-to-focus and putting the focus ring off the edge of the screen caused layer problems, but couldn't test that reliably.

Comment 27

[Justin D'Arcangelo [:justindarc]]

David: It's getting very late for me but this may help answer some of your questions.

- Color layers are ok (from what I understand) because they are very cheap compared to a Thebes layer.

- Thebes layers that are "not visible" are also ok because they aren't drawn

- The size of the Thebes layer matters. This is I think what the root of the issue was. We had several Thebes layers that were literally the entire size of the screen. A perfectly optimized Camera app should have 2 of these "full screen" Thebes layers: one for the viewfinder and one for all the UI overlaid on top of it.

There's a third Thebes layer showing up in our trees that is rather small (150x75 I think) -- this may or may not be optimizable, but I'm not as concerned about it right now as I am about the big ones. I think this has to do with an "overflow: hidden" declaration on our slideable toggle switch. It makes sense because we have to draw content twice there in order to achieve the proper visual effect (opacity, rounded corners).

Comment 28

[David Flanagan [:djf]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #24)
> Comment on attachment 8469033 [details] [review]
> scaling patch plus overflow:none in hud.css
> 
> David, can you also remove the `will-change` declarations from my patch in
> your patch as well? It may help reduce those intermittent Thebes layers
> you're seeing. Also, in the cases where I removed them, the elements were
> fairly small in size so there was not any noticeable degradation in
> animation performance. I did keep the one in viewfinder for opacity because
> that's a full-screen element and it is already a layer because of the video.

I based my code on an old version of your patch and didn't see the will-change changes. I've updated my PR to include those.

Comment 29

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8469033 [details] [review]
scaling patch plus overflow:none in hud.css

Conditional R+ as I'd like you to take my GH comment into consideration about also applying your rounding changes to the "fit" function. Based on the layer tree you're getting with this (and it might even be a little better now without some of those will-change declarations), I'd say this is probably about as good as we're gonna get.

We should wait and see what the power consumption tests look like with each of these patches and then make a decision on which one to land. I'll hold off on setting the R+ flag until we figure that out so there's no confusion when it comes time to uplift this to v2.0.

Looks good though. Thanks David!

Comment 30

[David Flanagan [:djf]]

I actually just reverted that last change to my patch, so now it does not remove the will-change declarations the way yours does. Adding those made the Thebes layer visible again, or maybe I'm just hitting the weird intermittent thing, but I tried multiple times and my patch did not work right with the will-change things removed, and it does work correctly, at least some of the time as it is.

Comment 31

[David Flanagan [:djf]]

Justin,

I'm happy to make the change to the fit function as well as fill. But I'm kind of going crazy with this sometimes it works sometimes it doesn't business. Have you been able to verify the layer tree that I'm reporting with my patch? Have you been testing while recording or just while the viewfinder is running? Can you get my patch to work if you remove the will-change declarations?

Anyway, hopefully Jon or Mike can try some power measurements and then we'll know how to proceed.

Comment 32

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to David Flanagan [:djf] from comment #23)

> (Also setting needinfo for Mike since he knows the camera app and may also
> have access to a power harness.)

Alas, I only have a harness for Helix. Jon, do you have any extra Flame harnesses lying around? If so, can I get one?

Comment 33

[Jon Hylands [:jhylands]]

I'm working on doing the power analysis this morning. I should have results by early afternoon.

I do have a spare Flame harness, but no way to get it to you today.

Comment 34

[Markus Stange [:mstange]]

One reason for a ThebesLayer not getting promoted to a ColorLayer intermittently could be bug 987030, which basically makes the changes from bug 946952 ineffective. One way to verify whether that's your problem is to go to this function: http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/layout/base/FrameLayerBuilder.cpp#l1997 and replace the line "return aLayerBuilder->CheckInLayerTreeCompressionMode();" with "return true;".

Comment 35

[Jon Hylands [:jhylands]]

Okay, these are the numbers I get right now:

Baseline, latest master

Camcorder preview:	669 mA
Camcorder recording:	796 mA

DJF's PR

Camcorder preview:	668 mA
Camcorder recording:	801 mA

Justin's PR

Camcorder preview:	628 mA
Camcorder recording:	825 mA 

The numbers from DJF's PR says it doesn't make any difference to the power profile. Justin's PR seems to be contradictory, in that the preview numbers go down significantly, the recording numbers go up.

Note that all this has been done against latest master (as requested), not v2.0

Comment 36

[David Flanagan [:djf]]

Thanks, Jon. I guess it is back to the drawing board...

Comment 37

[Hema Koka [:hema]]

Blocking because of increased power consumption; see bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1049086

Comment 38

[David Flanagan [:djf]]

Since I can't measure power consumption myself, I'm going to try using the overdraw percentage (3rd number displayed when the frame rate indicator is turned on) as a proxy for power consumption.

With the unpatch camera app on master, I find:

front camera preview: 492 
front camera video preview: 542 
front camera video recording: 433
front camera after stopping recording 451 (substantially lower!)
front camera when returning to camera mode: 601 (substantially higher! Layer not being destroyed?)

# kill and restart the camera at this point
rear camera preview: 497 
rear video preview:  747
rear video recording: 633
rear after recording stopped 660 (lower!)
rear when returning to camera: 606 (higher!)

Now I'll try the same measurements with Justin's patch from this bug.


front camera preview: 396
front camera video preview: 446
front camera video recording:333
front camera after stopping recording:347
front camera when returning to camera mode: 498
tap to preview the video poster image: 300
return to camera: 398

# kill and restart the camera at this point
rear camera preview: 470
rear video preview:  720
rear video recording: 538
rear after recording stopped: 560
rear when returning to camera: 510
rear after going to preview and back: 405

If the overdraw numbers are a good proxy for power consumption then this patch seems to improve things across the board.

Unfortunately, I see a graphics bug when I record a video, return to camera mode, preview the video (without playing it) and then go back to camera. I'll attach a screenshot.

Comment 39

[David Flanagan [:djf]]

Attachment: bug after viewing video poster image and returning to viewfinder

Comment 40

[David Flanagan [:djf]]

Comment on attachment 8468737 [details] [review]
pull-request (master)

If you can fix the black boxes bug and add comments to the important CSS changes, then let's get this landed.

I'm setting r+, but if you'd like me to look again before landing, let me know.
I found that looking at the overdraw % number gave very stable and consistent results, so you might try my methodology in the comment above to see what you get with the black box bug fixed.

Comment 41

[David Flanagan [:djf]]

Baseline overdraw numbers for 2.0:

rear camera: 291
rear video preview: 441
rear video recording: 529
recording stopped: 447
return to camera: 297

restart app

front camera: 286
front video: 236
front video record 328
recording stopped 242
return to camera: 292

Comment 42

[David Flanagan [:djf]]

I've filed bug 1050468 about the black boxes bug that I attached a screenshot of. It doesn't seem to reproduce on 2.0, so maybe we can just get Justin's CSS fixes landed there now and worry about the bug later.

Comment 43

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v2.0)

Patch for v2.0 branch -- Carrying R+ over from the patch for master

Comment 44

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8469519 [details] [review]
pull-request (v2.0)

Jon, can you measure the power consumption of this patch on v2.0? Thanks!

Comment 45

[David Flanagan [:djf]]

Testing the 2.0 version of the patch to see how the overdraw numbers compare with the baseline in comment 41:


rear camera: 365 (worse)
rear video preview: 515 (worse)
rear video recording: 429 (better)
recording stopped: 521 (worse)
return to camera: 371 (worse)

restart app

front camera: 292 (about the same)
front video: 242 (about the same)
front video record 228 (big improvement)
recording stopped 248
return to camera: 298

Comment 46

[David Flanagan [:djf]]

I want to get a layer tree in 2.0 for this new patch. But turning on the dump layers option in settings does not seem to do anything on my build.  Is there something else I need to do?  Setting needinfo for Milan because I don't know who else to ask.

Comment 47

[Jon Hylands [:jhylands]]

Patch on v2.0:

Camcorder preview:	597 mA
Camcorder recording:	825 mA

Comment 48

[David Flanagan [:djf]]

Attachment: layer tree for the 2.0 patch for basic camera viewfinder preview

Comment 49

[David Flanagan [:djf]]

Now trying overdraw numbers for 2.0 with only the recording timer will-change line from Justin's patch to see if we can get the recording immprovements without making the other stuff worse.

rear camera photo preview: 297
video preview: 447
recording 529

front camera photo:286
video:236
recording:328

That's just what I got for 2.0 by itself so no change with that one line.

Comment 50

[David Flanagan [:djf]]

Attachment: layer tree for unpatched 2.0 branch at the camera preview

Comment 51

[David Flanagan [:djf]]

Comparing the last two layer trees shows that the patch replaces a visible ThebesLayer with an opaque color layer, which is a big win.

That improvement does not show up in my overpaint percentage numbers because presumably we're still painting the same number of pixels. But it should be more efficient to paint those pixels, so hopefully the actual power consumption numbers will show that this makes a difference.

Comment 52

[Jon Hylands [:jhylands]]

I redid the numbers with an almost-fully charged battery, and the numbers I get are:

v2.0:

Camcorder preview:	572 mA
Camcorder recording:	764 mA

With patch on v2.0:

Camcorder preview:	578 mA
Camcorder recording:	793 mA

I don't think the layer tree complexity is contributing much to power usage, at least not in this case.

Comment 53

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

Once the overdraw is small enough, and we end up in the hardware composer, we should see the power difference (I think.)  And hardware composer does like color layers (rather then thebes) so there is a better chance of ending up there with the layer changes.

Comment 54

[David Flanagan [:djf]]

Attachment: unpatched v2.0 layer tree for video preview

Comment 55

[David Flanagan [:djf]]

Attachment: patched 2.0 layer tree for video preview

The last two attachments are layer trees for the rear camera video preview. They show that this patch removes the problematic Thebes layer, so it seems surprising that we're not seeing a reduction in power consumption.  I worry now that we've been chasing a red herring.

Comment 56

[David Flanagan [:djf]]

Attachment: patched 2.0 layer tree for video recording

Comment 57

[David Flanagan [:djf]]

Attachment: unpatched v2.0 layer tree for video recording

Comment 58

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8469519 [details] [review]
pull-request (v2.0)

Please note, there should be 3 Thebes layers with this patch when recording because of the suggestion from this comment in the parent bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1049086#c12

On master, separating the recording button and the recording timer into their own layers resulted in a power decrease (~15mA IIRC).

Comment 59

[Justin D'Arcangelo [:justindarc]]

Attachment: alternate pull-request (v2.0)

Alternate patch for v2.0 that **DOES NOT** separate the recording timer onto its own layer.

Comment 60

[David Flanagan [:djf]]

Comment on attachment 8469614 [details] [review]
alternate pull-request (v2.0)

Jon,

Would you try out the power consumption of this patch when recording video?

Comment 61

[Jon Hylands [:jhylands]]

I'm getting 778 mA, as averaged over 30 seconds, with the v2.0 alt patch.

Comment 62

[David Flanagan [:djf]]

(In reply to Jon Hylands [:jhylands] from comment #61)
> I'm getting 778 mA, as averaged over 30 seconds, with the v2.0 alt patch.

That's almost exactly half way between the unpatched 2.0 number and the number you got with the original patch.

Comment 63

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #62)
> (In reply to Jon Hylands [:jhylands] from comment #61)
> > I'm getting 778 mA, as averaged over 30 seconds, with the v2.0 alt patch.
> 
> That's almost exactly half way between the unpatched 2.0 number and the
> number you got with the original patch.

Kinda makes sense maybe. Could be the difference between one layer and two layers?

Comment 64

[David Flanagan [:djf]]

In addition to working at the CSS level, I wonder if we also need to look at the structure of the HTML.  What Sushil keeps asking for at the HWC level is one full-screen layer for the video and one overlay with all of the controls in it.  I wonder if there is a way to organize the HTML so that we have a video element and a div and all of the various views (hud, controls, grid, indicators, timer, recording timer) go in that one div.  How big a change do you think that would be to the existing structure?  (Since the app doesn't have static HTML, I'm not actually sure how the HTML is all organized.)

Also: we should find out more about what we need to do when transforming the preview stream to ensure that we can stay on the HWC path.  We have to be sure that it is exactly the size of the screen in device pixels so no background shows through, of course. And we need to know about cropping. Since the preview streams don't match the screen aspect ratio, does that force us off the HWC path? Is overflow:none enough for the cropping, or would it help to use a CSS:clip declaration?  Or do we need to request a preview stream from the camera that matches the screen size?

Maybe it would be easier if we tried this on hamachis first because they have a device pixel ratio of 1 instead of 1.5.

The master branch seems a little flaky right now, and our CSS patches were pretty different for the two branches, so I think we should focus exclusively on the 2.0 branch for now, and then take what we learn here and make it work on master.

And, since CAF keeps referring to the E/HWComposer lines in their logcat, we need to know how to turn those on in our gecko builds.  I see that the Developer menu in Settings has a checkbox for Hardware composer, so presumably that should remain on.  I wonder if any of the other check boxes (Tiling, Simple tiling) are relevant. Milan or Benoit: could you explain how to get HWComposer logging in our logcat output?

Comment 65

[David Flanagan [:djf]]

Here's what I think might be a useful approach to this bug:

1) get a gecko build with the HWC logging in it. (Sushil has sent email with the necessary patches to HWCComposer2D.cpp)

2) create a very simple test app that is just a full-screen <video> element and  a full-screen transparent <div> right on top of it. 

3) Play a video in the video element and put a couple of buttons inside the div and see if it goes on HWC.  (If it doesn't I'd consider switching to a Hamachi and trying again because of the simpler devicePixelRatio on that device).

4) If it does do HWC, try changing the video element to actually use mozSrcObject instead of src.  I forget whether getUserMedia() uses mozSrcObject. If so, then I'd probably just do that instead of messing with mozCamera. At this point I wouldn't care about distortion in the video stream: I'd force the video element to be exactly the size of the screen.

5) If we're still on HWC, then I'd try adding a transform to the <video> element to make it bigger than the screen like we do in camera.  If that pushes us off of the HWC path, then I'd see what gets us back on. Is overflow:none enough? If not, then I'd try a  CSS clip property or something.  This is where we might need to be really careful about the device pixel ratio on Flame, so this might be easier to test on Hamachi.

6) If all this works, then I'd start adding more UI inside the transparent <div> to make sure we can continue to use HWC with a more complicated overlay.  I'd be careful not to use will-change on any of these elements, of course.

7) Finally, I'd also try adding UI elements outside of the transparent div, as siblings of the <video> element instead of as children of the <div>.  I think this is what we do in the camera code,  and we need to know whether we can get away with it or if we do need to put all of the ui elements into the same container.  (Note that the grid UI turns out to be problematic: when we turn it on, the overdraw percentage goes up by exactly 100, which means that it is causing another complete full-screen layer to be drawn.)  If we can get away with adding elements indepenently and still have them create just two layers on the HWC, then we're good.  But I suspect we we won't be able to do that and fear that we are going to have to change the HTML layout of the camera so that all UI is in a single transparent div.  (And that transparent div is probably the only element that should have a will-change on it.)

8) If need be we can reduce the scope here to only make these UI changes for video recording, since that appears to be the only case that CAF is actually blocking on.  But ideally, we'd fix this power issue throughout the camera.

Comment 66

[Hema Koka [:hema]]

(In reply to David Flanagan [:djf] from comment #64)
 Milan or Benoit: could you explain how
> to get HWComposer logging in our logcat output?

Adding Benoit, Milan is ooo (I think)

Comment 67

[Justin D'Arcangelo [:justindarc]]

Attachment: Barebones Camera App

Milan: This is a *barebones* Camera app. It simply has a background-color on the <body>, a <video> element for a viewfinder (no complex positioning/rotating), and a very simple "UI overlay" to display a capture button and a thumbnail. In this stripped-down app, I still do not see that Gecko is utilizing the HWC. Therefore, I think there is a regression somewhere on the platform side. It may possibly be related to: https://bugzilla.mozilla.org/show_bug.cgi?id=1052095

Comment 68

[Justin D'Arcangelo [:justindarc]]

Blocked by Bug 1052095 -- Seems to be a regression causing high overdraw which is likely the reason we are not getting the HWC.

Comment 69

[Justin D'Arcangelo [:justindarc]]

Attachment: layer-tree (v2.0+patch) - Viewfinder

Updated the v2.0 patch to go back to HWC. Overdraw still needs reduced, but this is the new layer tree when using Attachment 8469519 [details].

Comment 70

[Justin D'Arcangelo [:justindarc]]

Attachment: layer-tree (v2.0+patch) - Video Recording

Updated the v2.0 patch to go back to HWC. Overdraw still needs reduced, but this is the new layer tree when using Attachment 8469519 [details].

Comment 71

[Hema Koka [:hema]]

While we working on reducing overdraw numbers, please test and provide feedback for this patch: https://github.com/mozilla-b2g/gaia/pull/22641 (see comment 69/70)

Thanks
Hema

Comment 72

[bhargavg1]

(In reply to Hema Koka [:hema] from comment #71)
> While we working on reducing overdraw numbers, please test and provide
> feedback for this patch: https://github.com/mozilla-b2g/gaia/pull/22641 (see
> comment 69/70)
> 
> Thanks
> Hema

Hema we already have tested this patch and see that its falling back to GPU composition so there is no way that this gonna help. Did we update the patch in the bug from last time we tested

Comment 73

[Inder]

Bhargav -- yes its an updated patch. Can you please try it out.

Comment 74

[Justin D'Arcangelo [:justindarc]]

(In reply to bhargavg1 from comment #72)
> (In reply to Hema Koka [:hema] from comment #71)
> > While we working on reducing overdraw numbers, please test and provide
> > feedback for this patch: https://github.com/mozilla-b2g/gaia/pull/22641 (see
> > comment 69/70)
> > 
> > Thanks
> > Hema
> 
> Hema we already have tested this patch and see that its falling back to GPU
> composition so there is no way that this gonna help. Did we update the patch
> in the bug from last time we tested

Yes, I updated this patch when I posted Comment 69. It is no longer falling back to the GPU for me on Flame KK.

Comment 75

[bhargavg1]

(In reply to Justin D'Arcangelo [:justindarc] from comment #74)
> (In reply to bhargavg1 from comment #72)
> > (In reply to Hema Koka [:hema] from comment #71)
> > > While we working on reducing overdraw numbers, please test and provide
> > > feedback for this patch: https://github.com/mozilla-b2g/gaia/pull/22641 (see
> > > comment 69/70)
> > > 
> > > Thanks
> > > Hema
> > 
> > Hema we already have tested this patch and see that its falling back to GPU
> > composition so there is no way that this gonna help. Did we update the patch
> > in the bug from last time we tested
> 
> Yes, I updated this patch when I posted Comment 69. It is no longer falling
> back to the GPU for me on Flame KK.

Thanks for the confirmation will check this further

Comment 76

[Sushil]

I checked with gaia patch: https://github.com/mozilla-b2g/gaia/pull/22641. It is using HWC. So the big translucent RGBA layer has been split into 2 small layers for Timer & Button. So this should improve data fetch.

You can further improve this layer tree:
1. Use Color layer for Background layer (HWC layer[0] in composed layers). It will avoid unnecessary painting in framework and unnecessary RGBA data fetch in display driver (Bug 1040952).
2. Is is possible to avoid painting bottom 2 Color layers in the layer tree. They are never considered for composition because they are hidden under full screen opaque Thebes layer.

Here is layer tree configuration with this patch:
E/HWComposer(  212): HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800] src=[0 0 480 764] E/HWComposer(  212): HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]

Layers composed on screen:
E/HWComposer(  212): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800] E/HWComposer(  212): HWC layer[1]::ImageLayerComposite: dst=[0 1 480 800] src=[0 24 719 456]
E/HWComposer(  212): HWC layer[2]::ThebesLayerComposite: dst=[21 26 171 101] src=[0 0 150 75] E/HWComposer(  212): HWC layer[3]::ThebesLayerComposite: dst=[164 632 317 785] src=[164 630 317 783]
E/HWComposer(  212): TryRender: Frame rendered

Comment 77

[Justin D'Arcangelo [:justindarc]]

(In reply to Sushil from comment #76)
> I checked with gaia patch: https://github.com/mozilla-b2g/gaia/pull/22641.
> It is using HWC. So the big translucent RGBA layer has been split into 2
> small layers for Timer & Button. So this should improve data fetch.
> 
> You can further improve this layer tree:
> 1. Use Color layer for Background layer (HWC layer[0] in composed layers).
> It will avoid unnecessary painting in framework and unnecessary RGBA data
> fetch in display driver (Bug 1040952).
> 2. Is is possible to avoid painting bottom 2 Color layers in the layer tree.
> They are never considered for composition because they are hidden under full
> screen opaque Thebes layer.
> 
> Here is layer tree configuration with this patch:
> E/HWComposer(  212): HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800]
> src=[0 0 480 764] E/HWComposer(  212): HWC layer[0]::ColorLayerComposite:
> dst=[0 0 480 800] src=[0 0 480 800]
> 
> Layers composed on screen:
> E/HWComposer(  212): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800]
> src=[0 0 480 800] E/HWComposer(  212): HWC layer[1]::ImageLayerComposite:
> dst=[0 1 480 800] src=[0 24 719 456]
> E/HWComposer(  212): HWC layer[2]::ThebesLayerComposite: dst=[21 26 171 101]
> src=[0 0 150 75] E/HWComposer(  212): HWC layer[3]::ThebesLayerComposite:
> dst=[164 632 317 785] src=[164 630 317 783]
> E/HWComposer(  212): TryRender: Frame rendered

We are still working on some additional bugs to help optimize this further. In the meantime, can you please test this patch to see how it affects our power utilization?

Comment 78

[Justin D'Arcangelo [:justindarc]]

(In reply to Sushil from comment #76)
> I checked with gaia patch: https://github.com/mozilla-b2g/gaia/pull/22641.
> It is using HWC. So the big translucent RGBA layer has been split into 2
> small layers for Timer & Button. So this should improve data fetch.
> 
> You can further improve this layer tree:
> 1. Use Color layer for Background layer (HWC layer[0] in composed layers).
> It will avoid unnecessary painting in framework and unnecessary RGBA data
> fetch in display driver (Bug 1040952).
> 2. Is is possible to avoid painting bottom 2 Color layers in the layer tree.
> They are never considered for composition because they are hidden under full
> screen opaque Thebes layer.
> 
> Here is layer tree configuration with this patch:
> E/HWComposer(  212): HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800]
> src=[0 0 480 764] E/HWComposer(  212): HWC layer[0]::ColorLayerComposite:
> dst=[0 0 480 800] src=[0 0 480 800]
> 
> Layers composed on screen:
> E/HWComposer(  212): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800]
> src=[0 0 480 800] E/HWComposer(  212): HWC layer[1]::ImageLayerComposite:
> dst=[0 1 480 800] src=[0 24 719 456]
> E/HWComposer(  212): HWC layer[2]::ThebesLayerComposite: dst=[21 26 171 101]
> src=[0 0 150 75] E/HWComposer(  212): HWC layer[3]::ThebesLayerComposite:
> dst=[164 632 317 785] src=[164 630 317 783]
> E/HWComposer(  212): TryRender: Frame rendered

I just updated the patch right now and I've been able to reduce one additional layer. If you could, please test it out and let me know if this is any better:

https://github.com/mozilla-b2g/gaia/pull/22641

Here's the relevant part of the layer tree in picture/viewfinder mode:

I/Gecko   (  746):         RefLayerComposite (0xb179c000) [shadow-clip=(x=0, y=0, w=480, h=854)] [shadow-visible=< (x=0, y=0, w=480, h=854); >] [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [id=6]
I/Gecko   (  746):           ContainerLayerComposite (0xb0e9e800) [shadow-visible=< (x=0, y=0, w=480, h=854); >] [visible=< (x=0, y=0, w=480, h=854); >] [componentAlpha] [metrics={ cb=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) sr=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) s=(x=0.000000, y=0.000000) dp=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) cdp=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) scrollId=3 z=1.500 }]
I/Gecko   (  746):             ThebesLayerComposite (0xb109a000) [not visible] [opaqueContent]
I/Gecko   (  746):               ContentHost (0xab645e80) [buffer-rect=(x=0, y=0, w=480, h=854)] [buffer-rotation=(x=0, y=0)]
I/Gecko   (  746):                 GrallocTextureHostOGL (0xadbc7220) [size=(w=480, h=854)] [format=SurfaceFormat::B8G8R8X8] [flags=]
I/Gecko   (  746):             ContainerLayerComposite (0xb109d000) [shadow-clip=(x=0, y=0, w=480, h=854)] [shadow-transform=[ 0 -1; 1 0; -80.3125 853.812; ]] [shadow-visible=< (x=0, y=80, w=854, h=481); >] [clip=(x=0, y=0, w=480, h=854)] [transform=[ 0 -1; 1 0; -80.3125 853.812; ]] [visible=< (x=0, y=80, w=854, h=481); >]
I/Gecko   (  746):               ImageLayerComposite (0xb109e800) [shadow-transform=[ 1.33438 0; 0 1.33333; 0 0; ]] [shadow-visible=< (x=0, y=60, w=640, h=361); >] [transform=[ 1.33438 0; 0 1.33333; 0 0; ]] [visible=< (x=0, y=60, w=640, h=361); >] [opaqueContent]
I/Gecko   (  746):                 ImageHost (0xafdfb920) [picture-rect=(x=0, y=0, w=640, h=480)]
I/Gecko   (  746):                   GrallocTextureHostOGL (0xadbc7340) [size=(w=640, h=480)] [format=SurfaceFormat::R8G8B8A8] [flags=]
I/Gecko   (  746):             ThebesLayerComposite (0xb1fe2800) [shadow-visible=< (x=0, y=8, w=480, h=119); (x=0, y=127, w=464, h=712); >] [visible=< (x=0, y=8, w=480, h=119); (x=0, y=127, w=464, h=712); >] [componentAlpha] [valid=< (x=0, y=8, w=480, h=831); >]
I/Gecko   (  746):               ContentHost (0xaeafdf60) [buffer-rect=(x=0, y=8, w=480, h=831)] [buffer-rotation=(x=0, y=0)]
I/Gecko   (  746):                 GrallocTextureHostOGL (0xadbd2100) [size=(w=480, h=831)] [format=SurfaceFormat::B8G8R8A8] [flags=]
I/Gecko   (  746): 

As you can see, the bottom-most Thebes layer is marked "not visible", so there are two layers drawn: ImageLayer -> ThebesLayer

And here's the relevant part of the layer tree while recording:

I/Gecko   (  746):         RefLayerComposite (0xb179c000) [shadow-clip=(x=0, y=0, w=480, h=854)] [shadow-visible=< (x=0, y=0, w=480, h=854); >] [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [id=6]
I/Gecko   (  746):           ContainerLayerComposite (0xb0e9e800) [shadow-visible=< (x=0, y=0, w=480, h=854); >] [visible=< (x=0, y=0, w=480, h=854); >] [componentAlpha] [metrics={ cb=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) sr=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) s=(x=0.000000, y=0.000000) dp=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) cdp=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) scrollId=3 z=1.500 }]
I/Gecko   (  746):             ThebesLayerComposite (0xb109a000) [not visible] [opaqueContent]
I/Gecko   (  746):               ContentHost (0xab645e80) [buffer-rect=(x=0, y=0, w=480, h=854)] [buffer-rotation=(x=0, y=0)]
I/Gecko   (  746):                 GrallocTextureHostOGL (0xadbc7220) [size=(w=480, h=854)] [format=SurfaceFormat::B8G8R8X8] [flags=]
I/Gecko   (  746):             ContainerLayerComposite (0xb109d000) [shadow-clip=(x=0, y=0, w=480, h=854)] [shadow-transform=[ 0 -1; 1 0; -0.300003 853.8; ]] [shadow-visible=< (x=0, y=0, w=854, h=481); >] [clip=(x=0, y=0, w=480, h=854)] [transform=[ 0 -1; 1 0; -0.300003 853.8; ]] [visible=< (x=0, y=0, w=854, h=481); >]
I/Gecko   (  746):               ImageLayerComposite (0xb109e800) [shadow-transform=[ 0.667188 0; 0 0.666667; 0 0; ]] [shadow-visible=< (x=0, y=0, w=1280, h=722); >] [transform=[ 0.667188 0; 0 0.666667; 0 0; ]] [visible=< (x=0, y=0, w=1280, h=722); >] [opaqueContent]
I/Gecko   (  746):                 ImageHost (0xafdfb920) [picture-rect=(x=0, y=0, w=1280, h=720)]
I/Gecko   (  746):                   GrallocTextureHostOGL (0xab6257c0) [size=(w=1280, h=720)] [format=SurfaceFormat::R8G8B8A8] [flags=]
I/Gecko   (  746):             ContainerLayerComposite (0xadcefc00) [shadow-visible=< (x=21, y=25, w=150, h=76); >] [visible=< (x=21, y=25, w=150, h=76); >] [componentAlpha]
I/Gecko   (  746):               ThebesLayerComposite (0xadcf2800) [shadow-visible=< (x=21, y=26, w=150, h=75); >] [visible=< (x=21, y=26, w=150, h=75); >] [componentAlpha] [valid=< (x=21, y=26, w=150, h=75); >]
I/Gecko   (  746):                 ContentHost (0xab64e390) [buffer-rect=(x=21, y=26, w=150, h=75)] [buffer-rotation=(x=0, y=0)]
I/Gecko   (  746):                   GrallocTextureHostOGL (0xab6b1160) [size=(w=150, h=75)] [format=SurfaceFormat::B8G8R8A8] [flags=]
I/Gecko   (  746):             ThebesLayerComposite (0xb1fe2800) [shadow-visible=< (x=164, y=686, w=153, h=153); >] [visible=< (x=164, y=686, w=153, h=153); >] [valid=< (x=0, y=8, w=480, h=831); >]
I/Gecko   (  746):               ContentHost (0xaeafdf60) [buffer-rect=(x=0, y=8, w=480, h=831)] [buffer-rotation=(x=0, y=0)]
I/Gecko   (  746):                 GrallocTextureHostOGL (0xadbd2160) [size=(w=480, h=831)] [format=SurfaceFormat::B8G8R8A8] [flags=]
I/Gecko   (  746): 

Again, the bottom-most Thebes layer is marked "not visible" so in this case, there are three layers drawn: ImageLayer -> ThebesLayer -> ThebesLayer

Let me know if this updated patch is better/worse for you. I intentionally did not squash the commits this time so we can revert today's changes if necessary. Thanks!

Comment 79

[Benoit Girard (:BenWa)]

These layer trees look go to me.

Comment 80

[Inder]

Bhargav/Sushil -- can you please try to get the numbers with this patch again https://github.com/mozilla-b2g/gaia/pull/22641.

Comment 81

[Sushil]

(In reply to Justin D'Arcangelo [:justindarc] from comment #78)
> Let me know if this updated patch is better/worse for you. I intentionally
> did not squash the commits this time so we can revert today's changes if
> necessary. Thanks!

Yes, I picked today's commit on top of yesterday's commit, the updated gaia patch is better in terms of reduced painting and data fetch because we do not get background layer as Thebes now. Here is layer tree configuration as seen in HWC:

Painted (but not composed):
E/HWComposer(  213): HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800] src=[0 0 480 764]

Composed layers by HWC:
E/HWComposer(  213): HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  213): HWC layer[1]::ImageLayerComposite: dst=[0 1 480 800] src=[0 24 719 456]
E/HWComposer(  213): HWC layer[2]::ThebesLayerComposite: dst=[21 26 171 101] src=[0 0 150 75]
E/HWComposer(  213): HWC layer[3]::ThebesLayerComposite: dst=[164 632 317 785] src=[164 630 317 783]

Comment 82

[Sushil]

Justin,

Though it is not so critical but is it possible to:
1. Get rid of unnecessary Color layer being painted as per layer tree in Comment 81.
2. Fix the 1 pixel shift (from top) in ImageLayerComposite so that I can skip layer[0] (in composed layers) from HWC Composition because opaque Video layer will cover entire screen.

Comment 83

[Sushil]

Since Color layer is very cheap through-out the display pipeline, so I would not worry about Comment 82 (if it needs too much of work on App side).

Comment 84

[Hema Koka [:hema]]

(In reply to Sushil from comment #81)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #78)
> > Let me know if this updated patch is better/worse for you. I intentionally
> > did not squash the commits this time so we can revert today's changes if
> > necessary. Thanks!
> 
> Yes, I picked today's commit on top of yesterday's commit, the updated gaia
> patch is better in terms of reduced painting and data fetch because we do
> not get background layer as Thebes now. Here is layer tree configuration as
> seen in HWC:
> 
> Painted (but not composed):
> E/HWComposer(  213): HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800]
> src=[0 0 480 764]
> 
> Composed layers by HWC:
> E/HWComposer(  213): HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800]
> src=[0 0 480 800]
> E/HWComposer(  213): HWC layer[1]::ImageLayerComposite: dst=[0 1 480 800]
> src=[0 24 719 456]
> E/HWComposer(  213): HWC layer[2]::ThebesLayerComposite: dst=[21 26 171 101]
> src=[0 0 150 75]
> E/HWComposer(  213): HWC layer[3]::ThebesLayerComposite: dst=[164 632 317
> 785] src=[164 630 317 783]

Sushil, thanks so much for testing! Do we have better power numbers with the patch now (since we are back on HWC and reduced layers, we assume it does)?

THanks
Hema

Comment 85

[Sushil]

I think Bhargav should have the info on improvement in power numbers with yesterday's changes. I believe today's changes have not been power profiled yet.

Comment 86

[bhargavg1]

(In reply to Hema Koka [:hema] from comment #84)
> (In reply to Sushil from comment #81)
> > (In reply to Justin D'Arcangelo [:justindarc] from comment #78)
> > > Let me know if this updated patch is better/worse for you. I intentionally
> > > did not squash the commits this time so we can revert today's changes if
> > > necessary. Thanks!
> > 
> > Yes, I picked today's commit on top of yesterday's commit, the updated gaia
> > patch is better in terms of reduced painting and data fetch because we do
> > not get background layer as Thebes now. Here is layer tree configuration as
> > seen in HWC:
> > 
> > Painted (but not composed):
> > E/HWComposer(  213): HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800]
> > src=[0 0 480 764]
> > 
> > Composed layers by HWC:
> > E/HWComposer(  213): HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800]
> > src=[0 0 480 800]
> > E/HWComposer(  213): HWC layer[1]::ImageLayerComposite: dst=[0 1 480 800]
> > src=[0 24 719 456]
> > E/HWComposer(  213): HWC layer[2]::ThebesLayerComposite: dst=[21 26 171 101]
> > src=[0 0 150 75]
> > E/HWComposer(  213): HWC layer[3]::ThebesLayerComposite: dst=[164 632 317
> > 785] src=[164 630 317 783]
> 
> Sushil, thanks so much for testing! Do we have better power numbers with the
> patch now (since we are back on HWC and reduced layers, we assume it does)?
> 
> THanks
> Hema

Yes we are doing better but would like to see the updated ones with todays patch and see where we are, will update after that.

Comment 87

[Justin D'Arcangelo [:justindarc]]

(In reply to bhargavg1 from comment #86)
> Yes we are doing better but would like to see the updated ones with todays
> patch and see where we are, will update after that.

Any update on these power numbers yet? Can we land this patch and close this bug?

Comment 88

[Justin D'Arcangelo [:justindarc]]

NI? for Comment 87

Comment 89

[David Flanagan [:djf]]

(In reply to Sushil from comment #82)

> 2. Fix the 1 pixel shift (from top) in ImageLayerComposite so that I can
> skip layer[0] (in composed layers) from HWC Composition because opaque Video
> layer will cover entire screen.

I suspect that the 1 pixel shift has to do with the conversion of CSS pixels to device pixels, and that we need to insert a Math.ceil() call when multiplying CSS pixels by device pixel ratio. And then also possibly avoid truncating when converting device pixels back to CSS pixels. We want to be sure that the video element has its height set to 533.333 pixels and not to 533 pixels.

Comment 90

[Inder]

Comment on attachment 8471982 [details]
layer-tree (v2.0+patch) - Viewfinder

Transferring the feedback to Sushil.

Comment 91

[Inder]

Comment on attachment 8471983 [details]
layer-tree (v2.0+patch) - Video Recording

Transferring the feedback to Sushil.

Comment 92

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #89)
> (In reply to Sushil from comment #82)
> 
> > 2. Fix the 1 pixel shift (from top) in ImageLayerComposite so that I can
> > skip layer[0] (in composed layers) from HWC Composition because opaque Video
> > layer will cover entire screen.
> 
> I suspect that the 1 pixel shift has to do with the conversion of CSS pixels
> to device pixels, and that we need to insert a Math.ceil() call when
> multiplying CSS pixels by device pixel ratio. And then also possibly avoid
> truncating when converting device pixels back to CSS pixels. We want to be
> sure that the video element has its height set to 533.333 pixels and not to
> 533 pixels.

If you look at the layer tree, the bottom-most ThebesLayer is marked "not visible" because it is completely covered by the <video> element. If for some reason HWC is still trying to draw it, then that's likely a separate issue.

Comment 93

[Sushil]

Comment on attachment 8471982 [details]
layer-tree (v2.0+patch) - Viewfinder

I provided feedback on latest patch in Comment 81 and 82.

Comment 94

[Sushil]

Comment on attachment 8471983 [details]
layer-tree (v2.0+patch) - Video Recording

I provided feedback on latest patch in Comment 81 and 82.

Comment 95

[Justin D'Arcangelo [:justindarc]]

(In reply to Sushil from comment #82)
> Justin,
> 
> Though it is not so critical but is it possible to:
> 1. Get rid of unnecessary Color layer being painted as per layer tree in
> Comment 81.

I no longer see a color layer in the layer tree. This was previously there so the Camera app would startup with a black background. Now, after the viewfinder is displayed, I remove the black background. Now, the bottom-most layer in my layer tree is a ThebesLayer marked "not visible".

> 2. Fix the 1 pixel shift (from top) in ImageLayerComposite so that I can
> skip layer[0] (in composed layers) from HWC Composition because opaque Video
> layer will cover entire screen.

I do not have a 1 pixel shift in my layer tree which is why my bottom-most ThebesLayer is marked "not visible". The <video> element (ImageLayer) is covering the entire screen.

Comment 96

[Sushil]

Yes, Thebes layer is marked as "not visible" and it is not drawn. I never commented on Thebes layer. I was talking about this painted (but not composed) Color layer in layer tree in Attachment #8471983 [details]:

I/Gecko   ( 1851):           ColorLayerComposite (0xb03ac800) [shadow-visible=< (x=0, y=0, w=480, h=854); >] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [color=rgba(0, 0, 0, 1)]

As per Comment 83, I am not worried about Color layer right now. It is very cheap anyway.

Comment 97

[Justin D'Arcangelo [:justindarc]]

(In reply to Sushil from comment #96)
> Yes, Thebes layer is marked as "not visible" and it is not drawn. I never
> commented on Thebes layer. I was talking about this painted (but not
> composed) Color layer in layer tree in Attachment #8471983 [details]:
> 
> I/Gecko   ( 1851):           ColorLayerComposite (0xb03ac800)
> [shadow-visible=< (x=0, y=0, w=480, h=854); >] [visible=< (x=0, y=0, w=480,
> h=854); >] [opaqueContent] [color=rgba(0, 0, 0, 1)]
> 
> As per Comment 83, I am not worried about Color layer right now. It is very
> cheap anyway.

That layer tree is obsolete. See updated layer tree in my Comment 78.

Comment 98

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/5bd72883a5f0c902b0791390d395478863fe723f

Note to sheriff(s): Do not uplift to v2.0 yet. Waiting on CAF to provide feedback on power consumption numbers. Thanks!

Also, if/when this does get uplifted, the proper v2.0 patch is in Attachment 8469519 [details].

Comment 99

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8469033 [details] [review]
scaling patch plus overflow:none in hud.css

Removing r? since we have already landed a patch for this on master.

Comment 100

[Justin D'Arcangelo [:justindarc]]

Landed on v2.0 (as per CAF's go-ahead in Bug 1049086):

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