1139541 - HwcComposer2D doesn't render Camera or Video frames on Flame v2.2

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Diego Wilson [:diego]]

Camera and Video are the key hardware composer use cases as it improved power performance.

When I load v2.2 on Flame I see camera preview and video playback falls back to GPU rendering.

Comment 1

[Diego Wilson [:diego]]

It seems that HWC itself still works. This is likely a regression on either layer refactoring or changes on the camera and video apps layouts.

Comment 2

[Mike Lee [:mlee]]

Hi Hema & Milan,

Can you have someone on your teams look into this fxOS 2.2 CAF issue? Per Diego Wilson's comment 1 this could be either an issue with Layer handling or changes in the Camera and Video apps.

Thanks,
Mike

Comment 3

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

Let's get a regression range; No-Jun, I don't know if you have time, but it would help.  FPS display trick is probably the best way to test if we're using HWC.

Comment 4

[No-Jun Park [:njpark]]

Note to QAnalysts for finding regression range (within 2.2):
STR:
- Go to Settings -> Developer -> Develope HUD -> select Frames Per Second
(Now you'll see the pink counter on the top left corner of the screen.  This is shown only when we are NOT using Hardwarecomposer.  When Camera app or Video app is running in full screen without any overlays, these pink numbers should not appear.  IF they do, then they are using OpenGL, and it is a bug.)
- Open camera app, and check viewfinder. IF the counter appears even when the AF circle is not present, then there is a bug.  IF the counter does not appear, then there isn't
- Open Videoplayer app, and play video in fullscreen mode with no overlay. If the counter appears, then there is a bug, If the counter does not appear, then it's working properly.

Thanks!

Comment 5

[Hema Koka [:hema]]

We have not made changes to camera and video app layout in 2.2

Lets wait for regression window to narrow down the cause and we can have the right folks look into it.

Comment 6

[Diego Wilson [:diego]]

There was also some VSync changes on the display/hwc side that landed in January

Comment 7

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression should block

Comment 8

[Brogan Zumwalt [Inactive]]

Central Regression Window:

Rough regression window below. Will be able to provide Mozilla-Inbound window tomorrow morning. Leaving regressionwindow-wanted keyword until then.

Last working central build:
Device: Flame 3.0
Build ID: 20150126132403
Gaia: b02ec9713e6de8d96c6954d2c0dfd0442b0656ac
Gecko: c3a90afa2dee
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First broke central build:
Device: Flame 3.0
Build ID: 20150126133400
Gaia: b02ec9713e6de8d96c6954d2c0dfd0442b0656ac
Gecko: a6f037b538ed
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Working Gaia with Broken Gecko issue DOES reproduce:
Gaia: b02ec9713e6de8d96c6954d2c0dfd0442b0656ac
Gecko: a6f037b538ed

Working Gecko with Broken Gaia issue does NOT reproduce:
Gaia: b02ec9713e6de8d96c6954d2c0dfd0442b0656ac
Gecko: c3a90afa2dee


Central Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c3a90afa2dee&tochange=a6f037b538ed

Comment 10

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

Maybe bug 1116588?

Comment 11

[Brogan Zumwalt [Inactive]]

Mozilla-Inbound Regression Window:

Last working Mozilla-Inbound build: 
Device: Flame 3.0
BuildID: 20150126062631
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: 0bec74187553
Version: 38.0a1 (3.0)
Firmware: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First broken Mozilla-Inbound build: 
Device: Flame 3.0
BuildID: 20150126065334
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: babd56077826
Version: 38.0a1 (3.0)
Firmware: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Working Gaia with Broken Gecko issue DOES reproduce:
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: babd56077826

Working Gecko with Broken Gaia issue does NOT reproduce:
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: 0bec74187553


Mozilla-Inbound Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0bec74187553&tochange=babd56077826


Issue appears to be occurring due to changes made in bug 1116588

Comment 12

[Kartikaya Gupta (email:[e-mail])]

The patch in the regression range was backed out and replaced with a different patch. Nonetheless it might still be caused by the second version of that patch, I'm investigating.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: A tale of four layer trees

So there's a couple of issues here. One is that yes, bug 1116588 changed the layer tree structure significantly so now we have a bunch more layers in the tree. However, all the new layers are opacity=0 and so presumably we can just ignore them in HWC. However, even if I do that (or if I back out bug 1116588 entirely) this still fails, because the layer tree has changed in another way since bug 1116588 landed. In particular there is now another PaintedLayerComposite at the top of the screen which is 1 pixel tall. This appears to be due to strange things being done in Gaia's CSS. Still tracking that down.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

The 1 pixel tall layer is coming from rounding error. Here is the display item:

nsDisplayTransform p=0xb1439810 f=0xabf59630(Block(div)(0)) z=5 bounds(0,-1228,19200,1229) layerBounds(0,-1228,19200,1229) visible(0,0,19200,1) componentAlpha(0,0,0,0) clip(0,0,19200,34160) [ 1 0; 0 0.682; 0 -30.69; ] layer=0xabef3680

Note that the bounds are at y=-1228 and the height is 1229 (this app units). The height and transform in this display item are coming from the CSS at [1] which is some magic I don't really understand. Regardless, if I hard-code the height to 30px and the transform to translateY(-30px) then this layer goes offscreen. kgrandon, any suggestions on this?

I have a small patch to make HWComposer ignore opacity=0 layers, but we'll need somebody on the Gaia side to take a look at this CSS to see what can be done. We need to fix both issues in order to fix this bug.

[1] https://github.com/mozilla-b2g/gaia/blob/b9d08564a61084949d53283d4ac6f00931dc4fe8/apps/system/style/chrome/chrome.css#L4-L5

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: HWC patch

Comment 16

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8573488 [details] [diff] [review]
HWC patch

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

Looks good.

Comment 17

[Kevin Grandon :kgrandon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> Note that the bounds are at y=-1228 and the height is 1229 (this app units).
> The height and transform in this display item are coming from the CSS at [1]
> which is some magic I don't really understand. Regardless, if I hard-code
> the height to 30px and the transform to translateY(-30px) then this layer
> goes offscreen. kgrandon, any suggestions on this?

These CSS values should be coming from here: https://github.com/mozilla-b2g/gaia/blob/b9d08564a61084949d53283d4ac6f00931dc4fe8/apps/system/style/definitions.css

We've had to do many tricks to try to achieve a smoothly shrinking/expanding rocketbar, and this might be one of those. This definition looks like it's for the fully expanded rocketbar, and the transform changes it into the shrunk version and hides it.

I'm assuming the issue is only for fullscreen apps as the goal seems to be to get the item to go fully offscreen. Being that this is for fullscreen apps, we should apply the following rules for height: https://github.com/mozilla-b2g/gaia/blob/b9d08564a61084949d53283d4ac6f00931dc4fe8/apps/system/style/chrome/chrome.css#L36

I believe then, inside a fullscreen app like camera we should have the following rules on the chrome: 

top: 0px;
height: 3rem;
transform: translateY(-3rem);

I'm not sure why hard-coding the height/translateY would be different than the CSS variables. Not sure off the top of my head, so will leave NI? for now and continue to investigate with you when I have time.

Comment 18

[Kevin Grandon :kgrandon]

Kats - do we need to solve the gaia problem in this bug, or should it be its own bug? Also it's unlikely that anything in the app chrome has changed since bug 1116588, so this was either broken for a long time, or some platform code broke it?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kevin Grandon :kgrandon from comment #17)
> We've had to do many tricks to try to achieve a smoothly shrinking/expanding
> rocketbar, and this might be one of those. This definition looks like it's
> for the fully expanded rocketbar, and the transform changes it into the
> shrunk version and hides it.
> 
> I'm assuming the issue is only for fullscreen apps as the goal seems to be
> to get the item to go fully offscreen. Being that this is for fullscreen
> apps, we should apply the following rules for height:
> https://github.com/mozilla-b2g/gaia/blob/
> b9d08564a61084949d53283d4ac6f00931dc4fe8/apps/system/style/chrome/chrome.
> css#L36
> 
> I believe then, inside a fullscreen app like camera we should have the
> following rules on the chrome: 
> 
> top: 0px;
> height: 3rem;
> transform: translateY(-3rem);

Hm, from what I remember looking in WebIDE it wasn't picking up that rule. WebIDE shows some nonsense and self-contradictory values when deal with calc() and var() but in layout at least the height was coming out to 1229 app units, which is 20.4833 CSS pixels. The transform matrix was [ 1 0; 0 0.682; 0 -30.69; ] which I believe is equivalent to scaleY(0.682) translateY(-20.46). (The -30.69 is in layout device pixels, so you have to divide by 1.5 to get CSS pixels).

> I'm not sure why hard-coding the height/translateY would be different than
> the CSS variables. Not sure off the top of my head, so will leave NI? for
> now and continue to investigate with you when I have time.

Pretty sure it's just rounding error with the way the scale is getting multiplied in. In layout it's off by only a single app unit which is 1/60 of a CSS pixel.

(In reply to Kevin Grandon :kgrandon from comment #18)
> Kats - do we need to solve the gaia problem in this bug, or should it be its
> own bug? Also it's unlikely that anything in the app chrome has changed
> since bug 1116588, so this was either broken for a long time, or some
> platform code broke it?

Hm, that's a good point. I looked at the layer dumps in attachment 8573467 [details] again and it looks like this was always a problem. It's just that before bug 1116588 landed the layer that was created would get a gralloc texture whereas now it does not - and that's the reason HwcComposer is bailing out. So that smells like a platform change that is recent (i.e. after bug 1116588). I can try to track it down. It would still be nice to get rid of the layer entirely by fixing that rounding error and/or gaia CSS, but I guess we can tackle that in a separate bug. Thanks for the help!

Comment 20

[Kartikaya Gupta (email:[e-mail])]

The m-c regression range for the Gralloc -> TiledContentHost switchover is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3094601af679&tochange=81f979b17fbd

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Inbound range is https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7cfd35c55c30&tochange=07479758ab68

Presumably bug 1127066 caused the hint to CreatePaintedLayerWithHint to change from non-scrollable to scrollable, which resulted in a tiled layer getting generated. I don't know if this is easy to fix - we might have to fix the gaia side thing after all (which would be a better fix anyway, since it will remove the layer from consideration completely).

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> Presumably bug 1127066 caused the hint to CreatePaintedLayerWithHint to
> change from non-scrollable to scrollable, which resulted in a tiled layer
> getting generated. I don't know if this is easy to fix

To be clear I mean that this might be expected behaviour now that we have APZ-scrollable things in the parent process, and in order to "fix" this we would have to do some ugly special-case hack.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> > I believe then, inside a fullscreen app like camera we should have the
> > following rules on the chrome: 
> > 
> > top: 0px;
> > height: 3rem;
> > transform: translateY(-3rem);
> 
> Hm, from what I remember looking in WebIDE it wasn't picking up that rule.

Ah, I was wrong. I might have been looking at it with the Camera app in the background. The computed style ends up being:

top: 0px
height: 30px
transform: matrix(1, 0, 0, 0.682, 0, -20.46)

which in theory should hide the thing completely offscreen, but doesn't because of the rounding. :(

Comment 24

[Diego Wilson [:diego]]

Comment on attachment 8573488 [details] [diff] [review]
HWC patch

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

LGTM

Comment 25

[Kartikaya Gupta (email:[e-mail])]

.... and I updated my B2G checkout to latest code and this problem seems to have gone away. I don't know what changed, but my patch by itself seems sufficient to fix the problem. I'll land it once the trees reopen.

Comment 26

[Diego Wilson [:diego]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> .... and I updated my B2G checkout to latest code and this problem seems to
> have gone away. I don't know what changed, but my patch by itself seems
> sufficient to fix the problem. I'll land it once the trees reopen.

Cool. Are you using the tip of v2.2?

Comment 27

[Kartikaya Gupta (email:[e-mail])]

No, tip of master.

Comment 28

[Diego Wilson [:diego]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> No, tip of master.

Any way you can try out the patch on v2.2?

Comment 29

[Kartikaya Gupta (email:[e-mail])]

I tried it on 2.2, and the layer tree there looks different. There's another tiled layer which is prevent HWC from running. I'll need to investigate it more. Regardless I think we should land this patch on master and uplift, as it removes at least one source of problems.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

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

Comment 31

[Phil Ringnalda (:philor)]

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

Comment 32

[Kartikaya Gupta (email:[e-mail])]

I'm investigating to see what's going on in 2.2. It appears to have a different regression window from what I can tell.

Comment 33

[Michael Vines [:m1] [:evilmachines]]

Re-opening as the issue originally reported in this bug on v2.2 has not been fixed, and per comment 32 it seems like it will not be fixed (only) by an uplift of attachment 8573488 [details] [diff] [review] to v2.2.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> .... and I updated my B2G checkout to latest code and this problem seems to
> have gone away. I don't know what changed

What changed was in this range: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=22a5ec5d76ee&tochange=0d8272920c72, so bug 1135805. We will likely need to uplift that to 2.2 along with a couple of other things, but let me verify.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

We will also need bug 1132371 and bug 1118051 uplifted as they both affect the layer tree here.

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Here's a try push based on b2g37 which includes the three gecko patches (bug 1132371, bug 1118051, and bug 1139541): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd0a2584b64

It doesn't include the gaia change since I'm not sure how to do that in a try push. This push is just to help verify uplifting the gecko patches still passes tests.

Comment 37

[Mike Lee [:mlee]]

Kevin,

Can you or someone else on the Gaia team help Kats with a try run of the gaia changes he mentioned in comment 36?

Thanks,
Mike

Comment 38

[Kartikaya Gupta (email:[e-mail])]

I don't think that's necessary. For one thing, doing a try push with the gaia change isn't going to prove or disprove anything, since the tests don't check to see if HWC is getting used or not. And for another, bug 1135805 just got uplifted to 2.2 so once the gecko patches are uplifted it should be trivial to verify.

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Now that the dependencies have been uplifted all that we need to do to fix this bug on 2.2 is uplift this patch (I verified this locally). So I'm returning this bug to the resolved fixed state, and we can use the usual uplift process to get this bug fixed on 2.2.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8573488 [details] [diff] [review]
HWC patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1116588
User impact if declined: When in the camera app the hardware composer doesn't get used when it should, resulting in reduced battery life.
Testing completed: on master, and locally on 2.2
Risk to taking this patch (and alternatives if risky): small patch which is pretty safe. It might help use HWC more in other scenarios as well.
String or UUID changes made by this patch: none

Comment 41

[Michael Vines [:m1] [:evilmachines]]

Ta. :diego, can you please check the v2.2 patches over here as well?

Comment 42

[Diego Wilson [:diego]]

With attachment 8573488 [details] [diff] [review] on top of the latest v2.2 HWC renders camera preview and video playback!

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f2a97dec765

Comment 44

[Brogan Zumwalt [Inactive]]

Issue still reproduces on Flame 3.0

Using simple check outlined in comment 4, the fps overlay always displays over camera viewfinder when camera is open. However fps overlay does not display while playing full screen video in video app.

Device: Flame 3.0
Build ID: 20150428010206
Gaia: 0636405f0844bf32451a375b2d61a2b16fe33348
Gecko: caf25344f73e
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0


Issue verified fixed in Flame 2.2

Using simple check in comment 4, the fps overlay does not display in camera or while playing a full screen video in the video app.

Device: Flame 2.2
Build ID: 20150428002500
Gaia: 9f6b1b9082662ba2c14168fc66bb02b4df3141e5
Gecko: e79c19bf19bf
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 45

[KTucker [:KTucker][Inactive 3/4/2016]]

The issue mentioned in Comment 44 is written in bug 1152461.