Closed Bug 946952 Opened 6 years ago Closed 6 years ago

Camera preview cannot be render with HwcComposer2D

Categories

(Core :: Layout, defect, P2, major)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: sushilchauhan, Assigned: mattwoodrow)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [fxos:media])

Attachments

(8 files, 1 obsolete file)

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.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(johu)
Sorry, I know nothing about HWComposition.

Vincent may know it. Transfer need info to Vincent.
Flags: needinfo?(johu) → needinfo?(vliu)
(v1.3+, blocking power goals)
Blocks: 942267
blocking-b2g: --- → 1.3+
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.
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 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.
Flags: needinfo?(vliu) → needinfo?(vlin)
(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.
Flags: needinfo?(vlin)
(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).
(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.
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
(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.
(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.
Flags: needinfo?(vlin)
(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.
Flags: needinfo?(vlin)
> 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.
(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.
(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.
(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.
(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.
(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 ?
George:
As we've discussed.
Need your help to make full-screen preview window in Gaia.
Flags: needinfo?(gduan)
Component: Gaia::Camera → Graphics
Product: Firefox OS → Core
Component: Graphics → Layout
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.
(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.
(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.
(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?
Flags: needinfo?(mvines)
Priority: -- → P2
Keywords: regression
(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+)
No longer blocks: 942267
blocking-b2g: 1.3+ → 1.3?
Flags: needinfo?(mvines)
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.
blocking-b2g: 1.3? → 1.4?
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(mlee)
No objection to moving to 1.4.
Flags: needinfo?(sotaro.ikeda.g)
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.
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.
Re-titling to clarify the impact
Summary: Layer tree during Camera preview is inconsistent. → Camera preview cannot be render with HwcComposer2D
blocking-b2g: 1.4? → 1.3?
Blocks: 942267
1.3+ as it's blocking bug 942267
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(bugs)
Flags: needinfo?(mlee)
(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?
Flags: needinfo?(bugs)
Who can get the answer for Jet in comment 32?
Flags: needinfo?(vlin)
Flags: needinfo?(sushilchauhan)
Flags: needinfo?(bgirard)
(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.
Flags: needinfo?(bgirard) → needinfo?(dglastonbury)
(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
Flags: needinfo?(dglastonbury)
Dan, if you have the right device, and can reproduce the problem, can you try and get the image for this case?
Flags: needinfo?(sushilchauhan)
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
(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?!
Solomon will help on this.
Flags: needinfo?(vlin) → needinfo?(schiu)
Product team has triaged this and considers this a blocker
Sushil,

please provide hw info.
Flags: needinfo?(sushilchauhan)
It's a reference device.
Flags: needinfo?(sushilchauhan)
Layer configuration and images provided in Comment 39, 38, 37 are from v1.3 reference device with screen resolution [480 x 800].
(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.
Update the layerscope dump for camera preview. And you can see the black background is a thebes layer.
Jet,
Is information in #46 and #47 enough?
Flags: needinfo?(bugs)
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.
Yes, if the background layer is Color layer then Camera preview frame can use hwcomposer. Please see Comment 8.
(In reply to C.J. Ku[:CJKu] from comment #48)
> Jet,
> Is information in #46 and #47 enough?

Yes, mattwoodrow is having a look.
Flags: needinfo?(bugs) → needinfo?(matt.woodrow)
(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.
clear NI regards to comment#49
Flags: needinfo?(schiu)
Vivien, thoughts about idea #1 from Comment 52?
Flags: needinfo?(21)
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.
Flags: needinfo?(matt.woodrow) → needinfo?(roc)
For media stuff I trust djf.
Flags: needinfo?(21) → needinfo?(dflanagan)
Assigning to Matt (for now) as he's actively working on this.
Assignee: nobody → matt.woodrow
(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
Flags: needinfo?(dflanagan) → needinfo?(justindarc)
(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.
Flags: needinfo?(roc)
(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.
(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
Whiteboard: [fxos:media]
Sure, I'll ping Matt on IRC when he's available.
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.
Adding ni?pchang for the above question.
Flags: needinfo?(pchang)
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.
(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.
Flags: needinfo?(pchang)
(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.
(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.
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.
Assignee: matt.woodrow → jdarcangelo
Attachment #8366868 - Flags: review?(dflanagan)
Flags: needinfo?(justindarc) → needinfo?(sushilchauhan)
(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.
(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.
Flags: needinfo?(sushilchauhan)
Sushil: Ok, thanks for verifying. Is there any other way to resolve this from the Gaia side?
I am not familiar with Gaia. HwcComposer2D compose the Gecko layers present in Composition tree.
Assignee: jdarcangelo → matt.woodrow
Assigning back to Matt to investigate a solution on the Gecko side.
Jet,

ni to make sure this is on your radar. This is a QC CS bug and will need immediate attention.
Flags: needinfo?(bugs)
(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.
I think this matches the conditions of the bug, it causes the same outcome at least.

You'll need your own video file though.
Fixes the bug with my standalone test case.
Attachment #8367717 - Flags: review?(roc)
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"?
Attachment #8367717 - Flags: review?(roc) → review-
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?
(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 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.
Attachment #8367771 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/c1a6fdd3fa01
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 968042
Attachment #8366868 - Flags: review?(dflanagan)
Blocks: 960372
Depends on: 987030
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap-
Depends on: 1125746
You need to log in before you can comment on or make changes to this bug.