946952 - Camera preview cannot be render with HwcComposer2D

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Sushil]

Observed an inconsistency with number and types of Gecko layers in the Composition tree during Camera preview. I have observed 3 different cases as below:

Case 1. Camera preview after boot-up:
E/HWComposer(  305): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  305): HWC layer[1]::ImageLayerComposite: dst=[15 0 465 800] src=[0 0 1280 720]
E/HWComposer(  305): HWC layer[2]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  305): H/W Composition failed

Case 2. Play a video then start Camera preview:
E/HWComposer(  300): HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  300): HWC layer[1]::ImageLayerComposite: dst=[15 0 465 800] src=[0 0 1280 720]
E/HWComposer(  300): HWC layer[2]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  300): Frame rendered

Case 3. Sometimes:
E/HWComposer(  300): HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  300): HWC layer[1]::ColorLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  300): HWC layer[2]::ImageLayerComposite: dst=[15 0 465 800] src=[0 0 1280 720]
E/HWComposer(  300): HWC layer[3]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  300): Frame rendered

In Case 1, the full screen Black background layer in Camera preview has been rendered as an RGBA Thebes layer but in Case 2, the same Black background layer comes as Color layer in the Composition tree. So the concern is, Case 1 makes the Camera preview frame non suitable for H/W Composition due to a limitation in HAL.

Comment 1

[Diego Wilson [:diego]]

Hi John,

The idea here is we expect a solid black background which translates to a ColorLayer. For some reason that is not always happening and that has some power performance repercussions. Do you know what could be happening?

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sorry, I know nothing about HWComposition.

Vincent may know it. Transfer need info to Vincent.

Comment 3

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

(v1.3+, blocking power goals)

Comment 4

[Sushil]

I have few suggestions on Camera App which should reduce power consumption on Camera preview frame.

1. The full screen Black background layer (HWC layer[0]) in Case 1 and 2 can be avoided if the opaque Image layer (HWC layer[1]) becomes full screen. Hence it will cover whole screen and "Camera UI" layer (HWC layer[2]) is anyway sitting on top of it. So the number of layers will reduce from 3 to 2 and this will reduce power consumption. Currently, destination rectangle of Camera preview layer is [15 0 465 800] so it leaves 15 pixels on left and right:
E/HWComposer(  305): HWC layer[1]::ImageLayerComposite: dst=[15 0 465 800] src=[0 0 1280 720]
Similarly, Camcorder preview layer leaves 33 pixels on top and bottom.

2. We could avoid down scaling operation on Camera preview layer by having source buffer crop of full screen dimensions which will be 480x800 (with Tr=0) or 800x480 (with Tr=4). Currently source crop is [0 0 1280 720]. Down-scaling is an additional operation hence this also contributes to power consumption.

3. 90 degree rotation on Image layer is also an additional operation. Since it depends on the angle at which Camera sensor is mounted on device, so I am not sure if 90 degree transform on Image layer can be avoided here. But if it could be avoided, it would save some power.

Comment 5

[Sushil]

Actually, #1 and #2 needs to be considered together because aspect ratio of Image layer (800/450) should be equal to source Aspect Ratio (1280/720).

Comment 6

[Vincent Liu[:vliu]]

(In reply to Sushil from comment #4)
> I have few suggestions on Camera App which should reduce power consumption
> on Camera preview frame.
> 

For 1 and 2, I think Vincent Lin can have a better answer than me. Transfer the question to him.

> 3. 90 degree rotation on Image layer is also an additional operation. Since
> it depends on the angle at which Camera sensor is mounted on device, so I am
> not sure if 90 degree transform on Image layer can be avoided here. But if
> it could be avoided, it would save some power.

Bug 947956 and bug 932669 added a web API to notify camera sensor angle attribute to gaia. I believe there will have some gaia works including rotation after the patch of this bug is landed.

Comment 7

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #0)

> In Case 1, the full screen Black background layer in Camera preview has been
> rendered as an RGBA Thebes layer but in Case 2, the same Black background
> layer comes as Color layer in the Composition tree. So the concern is, Case
> 1 makes the Camera preview frame non suitable for H/W Composition due to a
> limitation in HAL.

What on earth causes H/W Composition failed in Case 1 ?

I can't see anything special on those layers in Case 1.

Comment 8

[Diego Wilson [:diego]]

(In reply to Vincent Lin[:vilin] from comment #7)
> (In reply to Sushil from comment #0)
> 
> > In Case 1, the full screen Black background layer in Camera preview has been
> > rendered as an RGBA Thebes layer but in Case 2, the same Black background
> > layer comes as Color layer in the Composition tree. So the concern is, Case
> > 1 makes the Camera preview frame non suitable for H/W Composition due to a
> > limitation in HAL.
> 
> What on earth causes H/W Composition failed in Case 1 ?
> 
> I can't see anything special on those layers in Case 1.

HWC can render case 1. But the question is not "can it render it?". The question is "can it render it fast enough?".

Case 2 is better for HWC than case 1 because HWC optimizes color layer. It's renders a color layer much faster than it would a thebes layer with solid color content.

The purpose of this bug is to avoid using a thebes layer (case 1) when we can achieve the same thing more efficiently with a color layer (case 2).

Comment 9

[Vincent Lin[:vilin]]

(In reply to Diego Wilson [:diego] from comment #8)
> (In reply to Vincent Lin[:vilin] from comment #7)
> > (In reply to Sushil from comment #0)
> > 
> > > In Case 1, the full screen Black background layer in Camera preview has been
> > > rendered as an RGBA Thebes layer but in Case 2, the same Black background
> > > layer comes as Color layer in the Composition tree. So the concern is, Case
> > > 1 makes the Camera preview frame non suitable for H/W Composition due to a
> > > limitation in HAL.
> > 
> > What on earth causes H/W Composition failed in Case 1 ?
> > 
> > I can't see anything special on those layers in Case 1.
> 
> HWC can render case 1. But the question is not "can it render it?". The
> question is "can it render it fast enough?".
> 
> Case 2 is better for HWC than case 1 because HWC optimizes color layer. It's
> renders a color layer much faster than it would a thebes layer with solid
> color content.
> 
> The purpose of this bug is to avoid using a thebes layer (case 1) when we
> can achieve the same thing more efficiently with a color layer (case 2).

From the Description, H/W Composition will failed on Case 1.

Case 1. Camera preview after boot-up:
E/HWComposer(  305): HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  305): HWC layer[1]::ImageLayerComposite: dst=[15 0 465 800] src=[0 0 1280 720]
E/HWComposer(  305): HWC layer[2]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800]
E/HWComposer(  305): H/W Composition failed

Case 1 may hit the criteria falling back in MDPCompLowRes::isDoable().

Anyway, inconsistent Layer tree is an issue.

BenWa:
Do you know what caused a ThebesLayerComposite being built with solid color content ?
Why not be a ColorLayer for a solid color rectangular.
I think it's related to certain CSS style which effects the process of Layout.

Comment 10

[Benoit Girard (:BenWa)]

Case 3 is interesting. We're smart enough to only generate one color layers for exact layers *unless* they are coming from different processes. Ideally dumps from flipping layers.dump;true give much more details so it's more handy if you post these. We can tell where the process boundary is in the layer tree.

(In reply to Vincent Lin[:vilin] from comment #9)
> BenWa:
> Do you know what caused a ThebesLayerComposite being built with solid color
> content ?
> Why not be a ColorLayer for a solid color rectangular.
> I think it's related to certain CSS style which effects the process of
> Layout.

A color layer will be promoted to a thebes layer if its not a solid color, or layout doesn't reconize something has being a solid color (can't easily figure it out or just does implement the solid color check correctly). If you can isolate a test case we can investigate it. It's also possible that something that otherwise would hit the thebes layers fall into the background color layer and forces the promotion to a thebes layer. Dumping an image of the surface to would also be useful.

Comment 11

[Sushil]

(In reply to Benoit Girard (:BenWa) from comment #10)
> A color layer will be promoted to a thebes layer if its not a solid color,
> or layout doesn't reconize something has being a solid color (can't easily
> figure it out or just does implement the solid color check correctly). If
> you can isolate a test case we can investigate it.

Camera preview is the test case for Cases 1, 2 and 3.

Comment 12

[Sushil]

(In reply to Vincent Liu[:vliu] from comment #6)
> (In reply to Sushil from comment #4)
> > I have few suggestions on Camera App which should reduce power consumption
> > on Camera preview frame.
> > 
> 
> For 1 and 2, I think Vincent Lin can have a better answer than me. Transfer
> the question to him.
> 
Vincent, we are also looking for some feedback on suggestions 1 and 2 in comment #4.

Comment 13

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #12)
> (In reply to Vincent Liu[:vliu] from comment #6)
> > (In reply to Sushil from comment #4)
> > > I have few suggestions on Camera App which should reduce power consumption
> > > on Camera preview frame.
> > > 
> > 
> > For 1 and 2, I think Vincent Lin can have a better answer than me. Transfer
> > the question to him.
> > 
> Vincent, we are also looking for some feedback on suggestions 1 and 2 in
> comment #4.

1. IMHO. I think display controller like MDP is supposed to design as what you suggested. It should not cost anything on the area of any layer masked by its upper layer unless alpha blending is necessary. It only matters GPU-like composer like CopyBit.

2. Source cropping here is not a good idea. The view angle will be different from the captured image.
   How much power will it save ? Worth to do that ?

This Bug is going to improve power consumption ?
If HWComposer can handle Case 1, I think there'll be no much difference on power consumption compared to other cases.

Comment 14

[Diego Wilson [:diego]]

> This Bug is going to improve power consumption ?
> If HWComposer can handle Case 1, I think there'll be no much difference on
> power consumption compared to other cases.

HWC *could* handle case 1 but it doesn't. If falls back to GPU because the performance penalty (speed) of copying two thebes layer is too much.

Comment 15

[Sushil]

(In reply to Vincent Lin[:vilin] from comment #13)
> 1. IMHO. I think display controller like MDP is supposed to design as what
> you suggested. It should not cost anything on the area of any layer masked
> by its upper layer unless alpha blending is necessary. It only matters
> GPU-like composer like CopyBit.
>

Basically power consumption is almost directly proportional to size of layer buffer contents to be fetched (which is src_width x src_height x bpp) for each layer. GPU and MDP Composition (either H/W Overlay or Copybit) still fetches the buffer contents even though "some" portion of that layer is masked by above layer, like HWC layer[0] and layer[1] in Camera preview case and contributes to power. So suggestion # 1 is to try to drop the black background layer so we can avoid processing 1 full screen layer. For ex, Android Camera preview has 2 layers, full screen preview layer which is RGBX (no scaling, destination = source crop) and full screen UI controls RGBA layer sitting on top of it.

> 2. Source cropping here is not a good idea. The view angle will be different
> from the captured image.
>    How much power will it save ? Worth to do that ?
> 
> This Bug is going to improve power consumption ?

Yes, I am sure this bug will improve power numbers.

Comment 16

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #15)
> (In reply to Vincent Lin[:vilin] from comment #13)
> > 1. IMHO. I think display controller like MDP is supposed to design as what
> > you suggested. It should not cost anything on the area of any layer masked
> > by its upper layer unless alpha blending is necessary. It only matters
> > GPU-like composer like CopyBit.
> >
> 
> Basically power consumption is almost directly proportional to size of layer
> buffer contents to be fetched (which is src_width x src_height x bpp) for
> each layer. GPU and MDP Composition (either H/W Overlay or Copybit) still
> fetches the buffer contents even though "some" portion of that layer is
> masked by above layer, like HWC layer[0] and layer[1] in Camera preview case
> and contributes to power. So suggestion # 1 is to try to drop the black
> background layer so we can avoid processing 1 full screen layer. For ex,
> Android Camera preview has 2 layers, full screen preview layer which is RGBX
> (no scaling, destination = source crop) and full screen UI controls RGBA
> layer sitting on top of it.
> 
> > 2. Source cropping here is not a good idea. The view angle will be different
> > from the captured image.
> >    How much power will it save ? Worth to do that ?
> > 
> > This Bug is going to improve power consumption ?
> 
> Yes, I am sure this bug will improve power numbers.

I'm finding someone who can comment or help on Gaia modification.
After all, cropping source to fit display size will affect preview view angle.

Meanwhile, I think you can hack in HWComposer for verification first ?
I'm just curious how much saving will it gain.

BTW.
I think Android camera preview layer is not full-screen. Can you provide layer info by "adb shell dumpsys SurfaceFlinger" in Android ? There should be an area for UI and it definitely didn't intersect to preview layer. Also you can see camera image size is not equal to screen size and rotation is necessary.

The design is to make view angle consistent and proportional between preview and capture.
I think iOS also follows the same rule.

Comment 17

[Sushil]

(In reply to Vincent Lin[:vilin] from comment #16)
> Meanwhile, I think you can hack in HWComposer for verification first ?
> I'm just curious how much saving will it gain.
> 
Yes, I will try to find a setup tomorrow to check power numbers for cases:
1. Layer[0] as Color layer
2. Layer[0] as Thebes layer
3. Skip layer[0] in composition

> BTW.
> I think Android camera preview layer is not full-screen. Can you provide
> layer info by "adb shell dumpsys SurfaceFlinger" in Android ? There should
> be an area for UI and it definitely didn't intersect to preview layer. Also
> you can see camera image size is not equal to screen size and rotation is
> necessary.
> 

Here is layer configuration on Android Camera preview frame on same reference device:
GLES | b7285460 | 00000000 | 00000000 | 00 | 00100 | 00000002 | [ 0, 0, 480, 800] | [ 0, 0, 480, 800] SurfaceView
GLES | b72a6868 | 00000000 | 00000000 | 00 | 00105 | 00000001 | [ 0, 0, 480, 800] | [ 0, 0, 480, 800] com.android.gallery3d/com.android.camera.CameraLauncher

As compared to Android frame, we have a full-screen extra layer. Plus down-scaling & 90 degree rotation are additional operations on the preview layer.

Comment 18

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #5)
> Actually, #1 and #2 needs to be considered together because aspect ratio of
> Image layer (800/450) should be equal to source Aspect Ratio (1280/720).

In Case 1
E/HWComposer(  305): HWC layer[1]::ImageLayerComposite: dst=[15 0 465 800] src=[0 0 1280 720]

If you can hack ImageLayer in HAL like dst=[0 0 480 800], src=[240, 120, w=800, h=480] and skip layer[0], is it equivalent to #1 and #2 you want ? But as I mensioned, source cropping makes view angle different.

Comment 19

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #17)
> (In reply to Vincent Lin[:vilin] from comment #16)
> Here is layer configuration on Android Camera preview frame on same
> reference device:
> GLES | b7285460 | 00000000 | 00000000 | 00 | 00100 | 00000002 | [ 0, 0, 480,
> 800] | [ 0, 0, 480, 800] SurfaceView
> GLES | b72a6868 | 00000000 | 00000000 | 00 | 00105 | 00000001 | [ 0, 0, 480,
> 800] | [ 0, 0, 480, 800]
> com.android.gallery3d/com.android.camera.CameraLauncher
> 
> As compared to Android frame, we have a full-screen extra layer. Plus
> down-scaling & 90 degree rotation are additional operations on the preview
> layer.

It amazes me. Is it a general case in Android ? How about play a 720p video ? MediaServer or somewhere will transform 720p to 480x800 before queuing to SurfaceFlinger ? No scaling, rotating operation requirement to Compositor ?

Comment 20

[Vincent Lin[:vilin]]

George:
As we've discussed.
Need your help to make full-screen preview window in Gaia.

Comment 21

[Sushil]

Vincent,

I got power numbers for 1 minute Camera preview on reference MDP Copybit device. Here is Average Current value for the following 3 cases:
1. Layer[0] as Thebes layer = 442.74 mA
2. Layer[0] as Color  layer = 433.69 mA
3. Skip layer[0] = 426.37 mA (With Image layer: dst=[0 0 480 800], src=[240, 120, w=800, h=480], Tr=4)
  
There is 16 mA saving between Case 1 and 3.

Comment 22

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #21)
> Vincent,
> 
> I got power numbers for 1 minute Camera preview on reference MDP Copybit
> device. Here is Average Current value for the following 3 cases:
> 1. Layer[0] as Thebes layer = 442.74 mA
> 2. Layer[0] as Color  layer = 433.69 mA
> 3. Skip layer[0] = 426.37 mA (With Image layer: dst=[0 0 480 800], src=[240,
> 120, w=800, h=480], Tr=4)
>   
> There is 16 mA saving between Case 1 and 3.

16/442.74 ~= 3.6%
Not sure how gaia folks will see this trade-off between power saving and preview view angle.

Compare with 1&2
Inconsistent layer tree of this bug makes ~2%(9/433.69) more power.

In GPU-composition path, gecko will drop the fully-covered layer.
Not sure if it also works for HW-composition path, I think HAL can do the same thing.

We will check the inconsistent layer tree issue.

Comment 23

[Sushil]

(In reply to Vincent Lin[:vilin] from comment #22)
> 
> In GPU-composition path, gecko will drop the fully-covered layer.
> Not sure if it also works for HW-composition path, I think HAL can do the
> same thing.

HwcComposer2D also honors the Composition tree in Gecko. So if a layer is fully-covered and visibleRegion of layer is zero then we do not include it in H/W Composition. BTW, current camera app does not have this scenario for layer[0], so we cannot skip it.

Comment 24

[Sushil]

(In reply to Vincent Lin[:vilin] from comment #22)
> 
> We will check the inconsistent layer tree issue.

Thanks Vincent. I guess this should take care of Camcorder preview and recording use-cases as well ?
For the other part "2-layer Camera/Camcorder preview and recording": Let us get feedback from Gaia team on the feasibility and trade-off. Or if they want to provide an optimized camera test app to us, the test team can check and compare power numbers in all use cases of App.

Michael, what do you suggest?

Comment 25

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

(We are no longer blocking v1.3 on this bug but a fix would still be very desirable.  Moving to v1.3? to permit other triage teams to assess if they want to flag as v1.3+)

Comment 26

[Andrew Overholt [:overholt]]

Mike or Sotaro, it seems like this isn't something we'd block 1.3 on but I'm moving the nom to 1.4 so it's considered on an upcoming backlog.  If you disagree, please re-nom.

Comment 27

[Sotaro Ikeda [:sotaro]]

No objection to moving to 1.4.

Comment 28

[Sushil]

For v1.3, it will improve power numbers if you can fix the layer[0] issue (Thebes to Color layer) because Camera preview will start using HWC composition path. Please see Comment 21, there is 9 mA saving between Case 1 and 2.

Comment 29

[Mandyam Vikram]

Please note that in order to meet our power budget numbers for the camera preview case, we need it to be rendered via HWC. So, we should treat this as two different issues and target the issue in https://bugzilla.mozilla.org/show_bug.cgi?id=946952#c28 as a must have for 1.3.

Comment 30

[Diego Wilson [:diego]]

Re-titling to clarify the impact

Comment 31

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

1.3+ as it's blocking bug 942267

Comment 32

[Jet Villegas (inactive)]

(In reply to Benoit Girard (:BenWa) from comment #10)
> > BenWa:
> > Do you know what caused a ThebesLayerComposite being built with solid color
> > content ?
> > Why not be a ColorLayer for a solid color rectangular.
> > I think it's related to certain CSS style which effects the process of
> > Layout.
> 
> A color layer will be promoted to a thebes layer if its not a solid color,
> or layout doesn't reconize something has being a solid color (can't easily
> figure it out or just does implement the solid color check correctly). If
> you can isolate a test case we can investigate it. It's also possible that
> something that otherwise would hit the thebes layers fall into the
> background color layer and forces the promotion to a thebes layer. Dumping
> an image of the surface to would also be useful.

Can someone provide the image of the surface here? 

In FrameLayerBuilder.cpp#2011, the mIsSolidColorInVisibleRegion is only set to false in certain cases:

1. The clipping rect has any rounded corners.
2. the edges of the existing layer and the display item are not pixel-aligned

Are any of these conditions true for this case?

Comment 33

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

Who can get the answer for Jet in comment 32?

Comment 34

[Benoit Girard (:BenWa)]

(In reply to Jet Villegas (:jet) from comment #32)
> Can someone provide the image of the surface here? 

Sounds like something LayersScope would solve. Dan?

> 
> In FrameLayerBuilder.cpp#2011, the mIsSolidColorInVisibleRegion is only set
> to false in certain cases:
> 
> 1. The clipping rect has any rounded corners.
> 2. the edges of the existing layer and the display item are not pixel-aligned
> 
> Are any of these conditions true for this case?

mIsSolidColorInVisibleRegion doesn't imply we get a color layer however. As soon as we've accumulated non solid content the layer will become a thebes layer instead of a color layer.

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1758

We also need an empty valid region to form a color layer.

Comment 35

[u480271]

(In reply to Benoit Girard (:BenWa) from comment #34)
> Sounds like something LayersScope would solve. Dan?

I believe so. LayerScope will log all the layers passed to the compositor each frame.

A brief step-by-step is located at https://wiki.mozilla.org/Platform/GFX/LayerScope.

Example output: http://imgur.com/a/Y0mlM

Comment 36

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

Dan, if you have the right device, and can reproduce the problem, can you try and get the image for this case?

Comment 37

[Sushil]

Attachment: Camera preview - HWC layer[0] - Thebes layer - Full screen black background

Comment 38

[Sushil]

Attachment: Camera preview - HWC layer[1] - Image layer - Preview content

Comment 39

[Sushil]

Attachment: Camera preview - HWC layer[2] - Thebes layer - Camera controls (Full screen, transparent)

I have attached raw layer dumps of the Camera preview frame. Here is the layer configuration:
HWC layer[0]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800] Tr=0 Blending=0x100 Flags=0x40
HWC layer[1]::ImageLayerComposite: dst=[15 0 465 800] src=[0 0 1280 720] Tr=4 Blending=0x100 Flags=0x0
HWC layer[2]::ThebesLayerComposite: dst=[0 0 480 800] src=[0 0 480 800] Tr=0 Blending=0x105 Flags=0x40

Comment 40

[u480271]

(In reply to Milan Sreckovic [:milan] from comment #36)
> Dan, if you have the right device, and can reproduce the problem, can you
> try and get the image for this case?

I'm assuming this is hamachi/buri HW?!

Comment 41

[Vincent Lin[:vilin]]

Solomon will help on this.

Comment 42

[Sri Kasetti]

Product team has triaged this and considers this a blocker

Comment 43

[Preeti Raghunath(:Preeti)]

Sushil,

please provide hw info.

Comment 44

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

It's a reference device.

Comment 45

[Sushil]

Layer configuration and images provided in Comment 39, 38, 37 are from v1.3 reference device with screen resolution [480 x 800].

Comment 46

[Peter Chang[:pchang]]

(In reply to Jet Villegas (:jet) from comment #32)
> (In reply to Benoit Girard (:BenWa) from comment #10)
> > > BenWa:
> > > Do you know what caused a ThebesLayerComposite being built with solid color
> > > content ?
> > > Why not be a ColorLayer for a solid color rectangular.
> > > I think it's related to certain CSS style which effects the process of
> > > Layout.
> > 
> > A color layer will be promoted to a thebes layer if its not a solid color,
> > or layout doesn't reconize something has being a solid color (can't easily
> > figure it out or just does implement the solid color check correctly). If
> > you can isolate a test case we can investigate it. It's also possible that
> > something that otherwise would hit the thebes layers fall into the
> > background color layer and forces the promotion to a thebes layer. Dumping
> > an image of the surface to would also be useful.
> 
> Can someone provide the image of the surface here? 
> 
> In FrameLayerBuilder.cpp#2011, the mIsSolidColorInVisibleRegion is only set
> to false in certain cases:
> 
> 1. The clipping rect has any rounded corners.
> 2. the edges of the existing layer and the display item are not pixel-aligned
> 
> Are any of these conditions true for this case?

In my local device, the mIsSoldColorInVisibleRegion flas is true.
And I found the following condition caused colorlayer didn't create.

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1759
If I removed above condition, I could see the camera preview with colorlayer.

Comment 47

[Peter Chang[:pchang]]

Attachment: layerscope dump for camera preview

Update the layerscope dump for camera preview. And you can see the black background is a thebes layer.

Comment 48

[u459114]

Jet,
Is information in #46 and #47 enough?

Comment 49

[Solomon Chiu [:schiu]]

I also made some test in PopThebesLayerData() as Peter mentioned(force the background layer be a colorLayer), the previewing can be composed by hwcomposer properly. However while the background layer is being fed to composer as a ThebesLayer, the hwcomposer will complain about the layer cannot be draw with copybit.

Comment 50

[Sushil]

Yes, if the background layer is Color layer then Camera preview frame can use hwcomposer. Please see Comment 8.

Comment 51

[Jet Villegas (inactive)]

(In reply to C.J. Ku[:CJKu] from comment #48)
> Jet,
> Is information in #46 and #47 enough?

Yes, mattwoodrow is having a look.

Comment 52

[Matt Woodrow (:mattwoodrow)]

(In reply to peter chang[:pchang][:peter] from comment #46)
 
> In my local device, the mIsSoldColorInVisibleRegion flas is true.
> And I found the following condition caused colorlayer didn't create.
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.
> cpp#1759
> If I removed above condition, I could see the camera preview with colorlayer.

Great, that makes sense.

The back ThebesLayer previously contained content that couldn't be optimized to a ColorLayer, and once it can be, we decide to keep using the ThebesLayer since it's already allocated.

A case where this optimization would really matter would be if we had a solid color, with some other content alternating between visible and hidden every frame. If we keep switching between ThebesLayer and ColorLayer, then we'll be reallocating and drawing the ThebesLayer buffer at 30fps. This was added in bug 727661, which unfortunately doesn't contain any specific details of the case it was fixing.

We definitely don't want to get rid of this optimization entirely, but we also want to bypass it somehow for this gaia use case.

I'll have to think about this more but I have a couple of ideas so far:

- We could work around this in gaia. The GetValidRegion().IsEmpty() check happens after invalidation, so if something caused the entire layer to be invalidated, then we'd get past it. Something simple like changing the background color to a slightly different shade of black (rgb(1,1,1) maybe?) would probably work.

- Add a heuristic to count how many times we attempt to convert a ThebesLayer to a ColorLayer in succession, and allow it to succeed if it gets higher than N. This lets us detect that we've stabilised to a situation that requires a ColorLayer, and we no longer want to hang on to the ThebesLayer. I'm not sure how to pick N for this though. Low values are a weak definition of stabilised, and could have poor performance for the flashing content example if the flashing rate was slightly higher than N. High values for N mean that we waste battery on b2g for that many frames.

- Let FrameLayerBuilder know that ColorLayers are significantly more valuable on b2g because of HWC. We might be able to combine this with the second idea to improve it.

Comment 53

[Solomon Chiu [:schiu]]

clear NI regards to comment#49

Comment 54

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

Vivien, thoughts about idea #1 from Comment 52?

Comment 55

[Matt Woodrow (:mattwoodrow)]

Attachment: Count number of times we try optimize to a ColorLayer

Implementation of idea 2.

I don't know enough about the original android bug to know if N=5 risks regressing that though :(

Adding ni?roc to see if he has any better ideas.

Comment 56

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For media stuff I trust djf.

Comment 57

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

Assigning to Matt (for now) as he's actively working on this.

Comment 58

[David Flanagan [:djf]]

(In reply to Milan Sreckovic [:milan] from comment #54)
> Vivien, thoughts about idea #1 from Comment 52?

Vivien passed this question on to me. I don't understand this bug at all, but have no problem with changing the camera background color if that would help.  If you need someone on the media team to do this, ask Diego Marcos, Wilson Page or Justin d'Arcangelo

Comment 59

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #52)
> - Add a heuristic to count how many times we attempt to convert a
> ThebesLayer to a ColorLayer in succession, and allow it to succeed if it
> gets higher than N. This lets us detect that we've stabilised to a situation
> that requires a ColorLayer, and we no longer want to hang on to the
> ThebesLayer. I'm not sure how to pick N for this though. Low values are a
> weak definition of stabilised, and could have poor performance for the
> flashing content example if the flashing rate was slightly higher than N.
> High values for N mean that we waste battery on b2g for that many frames.

How about, every T seconds we set a flag that allows the next FrameLayerBuilder run to be "slow"? In a slow run, we allow conversion from a ThebesLayer to a ColorLayer.

Comment 60

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to David Flanagan [:djf] from comment #58)
> (In reply to Milan Sreckovic [:milan] from comment #54)
> > Vivien, thoughts about idea #1 from Comment 52?
> 
> Vivien passed this question on to me. I don't understand this bug at all,
> but have no problem with changing the camera background color if that would
> help.  If you need someone on the media team to do this, ask Diego Marcos,
> Wilson Page or Justin d'Arcangelo

I can't said that I understand everything from this bug either. From what I understand there is theorically 2 ways to fix the issue. One of them is possibly to workaround the core issue in Gaia by changing the background color at runtime from rgb(0,0,0) to rgb(1,1,1)  which should invalidate some layers.

Due to the tight deadline I would say that if someone from the Media team can spend one hour speaking to Matt on IRC to make sure of the exact steps to follow to workaround the issue in Gaia then it may be the fastest and safiest thing for 1.3.

Comment 61

[Hema Koka [:hema]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #60)
> (In reply to David Flanagan [:djf] from comment #58)
> > (In reply to Milan Sreckovic [:milan] from comment #54)
> > > Vivien, thoughts about idea #1 from Comment 52?
> > 
> > Vivien passed this question on to me. I don't understand this bug at all,
> > but have no problem with changing the camera background color if that would
> > help.  If you need someone on the media team to do this, ask Diego Marcos,
> > Wilson Page or Justin d'Arcangelo
> 
> I can't said that I understand everything from this bug either. From what I
> understand there is theorically 2 ways to fix the issue. One of them is
> possibly to workaround the core issue in Gaia by changing the background
> color at runtime from rgb(0,0,0) to rgb(1,1,1)  which should invalidate some
> layers.
> 
> Due to the tight deadline I would say that if someone from the Media team
> can spend one hour speaking to Matt on IRC to make sure of the exact steps
> to follow to workaround the issue in Gaia then it may be the fastest and
> safiest thing for 1.3.

Justindarc, could you please take this on?

Thanks
Hema

Comment 62

[Justin D'Arcangelo [:justindarc]]

Sure, I'll ping Matt on IRC when he's available.

Comment 63

[Matt Woodrow (:mattwoodrow)]

Do we have an easy way of determining if HWC is being used? If not, could we have the HWC draw some sort of indicator (maybe just a small square of coloured pixels in a corner), so we can tell if we're hitting the fast path without using a debugger.

An option like that would make a fix for this bug really easy to verify. e.g.

Before fix: Load camera preview, no coloured square visible.
After fix: Load camera preview, coloured square visible.

Comment 64

[Matt Woodrow (:mattwoodrow)]

Adding ni?pchang for the above question.

Comment 65

[Matt Woodrow (:mattwoodrow)]

I can't see anything like that anyway. Should be fairly easy, add a pref and developer settings option (like paint flashing), and get a bool value to pass into HwcComposer2D::TryRender.

If true, then add an entry to the layer list just like we do for a ColorLayer, clipped to a small area.

The only problem I see is if this extra layer causes us to go over the layer limit. Dunno what the best behaviour there is.

Comment 66

[Peter Chang[:pchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #63)
> Do we have an easy way of determining if HWC is being used? If not, could we
> have the HWC draw some sort of indicator (maybe just a small square of
> coloured pixels in a corner), so we can tell if we're hitting the fast path
> without using a debugger.
> 
> An option like that would make a fix for this bug really easy to verify. e.g.
> 
> Before fix: Load camera preview, no coloured square visible.
> After fix: Load camera preview, coloured square visible.

If you want to check HWC is using or not, you can just enable the fps number from setting. And then you won't see the number if you are using HWC.

Does this answer your question?

By the way, I always saw the background as thebeslayer when first launch camera preview when using GPU composition only. Therefore, I think the problem is we are using colorlayer as optimization not related to HWC is using or not.

Comment 67

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #52)
> > - Add a heuristic to count how many times we attempt to convert a
> > ThebesLayer to a ColorLayer in succession, and allow it to succeed if it
> > gets higher than N. This lets us detect that we've stabilised to a situation
> > that requires a ColorLayer, and we no longer want to hang on to the
> > ThebesLayer. I'm not sure how to pick N for this though. Low values are a
> > weak definition of stabilised, and could have poor performance for the
> > flashing content example if the flashing rate was slightly higher than N.
> > High values for N mean that we waste battery on b2g for that many frames.
> 
> How about, every T seconds we set a flag that allows the next
> FrameLayerBuilder run to be "slow"? In a slow run, we allow conversion from
> a ThebesLayer to a ColorLayer.

I don't see how that improves over my suggestion? It could be worse, since with the android example, we might hit a 'slow' paint the very first frame.

Comment 68

[Peter Chang[:pchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #67)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #52)
> > > - Add a heuristic to count how many times we attempt to convert a
> > > ThebesLayer to a ColorLayer in succession, and allow it to succeed if it
> > > gets higher than N. This lets us detect that we've stabilised to a situation
> > > that requires a ColorLayer, and we no longer want to hang on to the
> > > ThebesLayer. I'm not sure how to pick N for this though. Low values are a
> > > weak definition of stabilised, and could have poor performance for the
> > > flashing content example if the flashing rate was slightly higher than N.
> > > High values for N mean that we waste battery on b2g for that many frames.
> > 
> > How about, every T seconds we set a flag that allows the next
> > FrameLayerBuilder run to be "slow"? In a slow run, we allow conversion from
> > a ThebesLayer to a ColorLayer.
> 
> I don't see how that improves over my suggestion? It could be worse, since
> with the android example, we might hit a 'slow' paint the very first frame.

How about we convert to colorlayer for the first time no matter vildregion is empty or not?
When the validregion is updated and the content is not solidcolor(maybe only color changed), then we keep using thebeslayer.

Comment 69

[Justin D'Arcangelo [:justindarc]]

Attachment: Patch to change background color

Sushil/David: I'm not sure how to easily verify if this patch resolves the issue. This simply implemented the idea proposed here regarding changing the background color after the app has initialized.

Comment 70

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #67)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #52)
> > > - Add a heuristic to count how many times we attempt to convert a
> > > ThebesLayer to a ColorLayer in succession, and allow it to succeed if it
> > > gets higher than N. This lets us detect that we've stabilised to a situation
> > > that requires a ColorLayer, and we no longer want to hang on to the
> > > ThebesLayer. I'm not sure how to pick N for this though. Low values are a
> > > weak definition of stabilised, and could have poor performance for the
> > > flashing content example if the flashing rate was slightly higher than N.
> > > High values for N mean that we waste battery on b2g for that many frames.
> > 
> > How about, every T seconds we set a flag that allows the next
> > FrameLayerBuilder run to be "slow"? In a slow run, we allow conversion from
> > a ThebesLayer to a ColorLayer.
> 
> I don't see how that improves over my suggestion? It could be worse, since
> with the android example, we might hit a 'slow' paint the very first frame.

We can ensure there's an upper bound on the time where we're not using HWC when we could be, which is independent of the frame rate, which would especially matter if the content process stops invalidating, which might even be the case in this bug. (Obviously we'd need to track when we had taken a short-cut in a "fast" transaction and schedule layer tree update off a timer if so.) Also, it would be global to the FrameLayerBuilder instead of per-layer which is a bit simpler. But it's not much different.

Comment 71

[Sushil]

(In reply to Justin D'Arcangelo from comment #69)
> Created attachment 8366868 [details] [review]
> Patch to change background color
> 
> Sushil/David: I'm not sure how to easily verify if this patch resolves the
> issue. This simply implemented the idea proposed here regarding changing the
> background color after the app has initialized.

Not fixed. With this patch, black background in Camera preview is still coming as Thebes layer.

Comment 72

[Justin D'Arcangelo [:justindarc]]

Sushil: Ok, thanks for verifying. Is there any other way to resolve this from the Gaia side?

Comment 73

[Sushil]

I am not familiar with Gaia. HwcComposer2D compose the Gecko layers present in Composition tree.

Comment 74

[Justin D'Arcangelo [:justindarc]]

Assigning back to Matt to investigate a solution on the Gecko side.

Comment 75

[Preeti Raghunath(:Preeti)]

Jet,

ni to make sure this is on your radar. This is a QC CS bug and will need immediate attention.

Comment 76

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #70)

> We can ensure there's an upper bound on the time where we're not using HWC
> when we could be, which is independent of the frame rate, which would
> especially matter if the content process stops invalidating, which might
> even be the case in this bug. (Obviously we'd need to track when we had
> taken a short-cut in a "fast" transaction and schedule layer tree update off
> a timer if so.) Also, it would be global to the FrameLayerBuilder instead of
> per-layer which is a bit simpler. But it's not much different.

That's a good point, I'd very much hope that we're not doing invalidating paints when only the camera image is changing.

I think ideally we'd want something similar to LayerActivityTracker where each transition between being a solid color and real content resets the timer for that layer only.

That may be unnecessarily complex for this though. I'll have a go at your suggestion then, since it's fairly simple.

Comment 77

[Matt Woodrow (:mattwoodrow)]

Attachment: Testcase that reproduces the issue

I think this matches the conditions of the bug, it causes the same outcome at least.

You'll need your own video file though.

Comment 78

[Matt Woodrow (:mattwoodrow)]

Attachment: Run a 'slow' paint up to 1 second after skipping the ColorLayer optimization

Fixes the bug with my standalone test case.

Comment 79

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

Comment on attachment 8367717 [details] [diff] [review]
Run a 'slow' paint up to 1 second after skipping the ColorLayer optimization

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

Looks good modulo those issues.

::: layout/base/FrameLayerBuilder.cpp
@@ +1807,5 @@
>      layer->SetClipRect(nullptr);
> +    // If we're a solid color, but didn't optimize to a ColorLayer, then request
> +    // a delayed repaint to make this change.
> +    if (data->mIsSolidColorInVisibleRegion) {
> +      const_cast<nsIFrame*>(data->mReferenceFrame)->SchedulePaint(nsIFrame::PAINT_DELAYED_SLOW);

Why not just do this in InSlowMode if it returns false?

::: layout/base/FrameLayerBuilder.h
@@ +592,5 @@
>      return mContainingThebesLayer;
>    }
>  
> +  void SetSlowMode() { mInSlowMode = true; }
> +  bool InSlowMode() { return mInSlowMode; }

I don't think we should call this "slow mode". We never want to be slow and it doesn't reflect what we're trying to do. How about "layer tree compression mode"?

Comment 80

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

Hmm, also, can we avoid storing state on the presshell? What if the FrameLayerBuilder managed the timer itself, and the timer firing sets the mode on the FrmaeLayerBuilder and does a regular SchedulePaint?

Comment 81

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #79)
> Comment on attachment 8367717 [details] [diff] [review]
> Run a 'slow' paint up to 1 second after skipping the ColorLayer optimization
> 
> Review of attachment 8367717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good modulo those issues.
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1807,5 @@
> >      layer->SetClipRect(nullptr);
> > +    // If we're a solid color, but didn't optimize to a ColorLayer, then request
> > +    // a delayed repaint to make this change.
> > +    if (data->mIsSolidColorInVisibleRegion) {
> > +      const_cast<nsIFrame*>(data->mReferenceFrame)->SchedulePaint(nsIFrame::PAINT_DELAYED_SLOW);
> 
> Why not just do this in InSlowMode if it returns false?

It seemed like a weird side effect, but changing the function name might make it more obvious.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #80)
> Hmm, also, can we avoid storing state on the presshell? What if the
> FrameLayerBuilder managed the timer itself, and the timer firing sets the
> mode on the FrmaeLayerBuilder and does a regular SchedulePaint?

FrameLayerBuilder objects aren't retained between paints.

Comment 82

[Matt Woodrow (:mattwoodrow)]

Attachment: Run a 'slow' paint up to 1 second after skipping the ColorLayer optimization v2

Comment 83

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

Comment on attachment 8367771 [details] [diff] [review]
Run a 'slow' paint up to 1 second after skipping the ColorLayer optimization v2

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

::: layout/generic/nsIFrame.h
@@ +2338,3 @@
>      PAINT_DEFAULT = 0,
> +    PAINT_COMPOSITE_ONLY,
> +    PAINT_DELAYED_COMPRESSED

Call this "PAINT_DELAYED_COMPRESS" instead of "COMPRESSED" since it's in the imperative mood.

Comment 84

[Ryan VanderMeulen [:RyanVM]]

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

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9737a63a47cf