Youtube does not use hardware composer in fullscreen with tiling enabled

RESOLVED FIXED in mozilla32

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: cwiiis)

Tracking

({perf, power})

unspecified
mozilla32
Other
Other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, b2g-v2.0 fixed)

Details

(Whiteboard: [caf priority: p1][CR 669803][c=power p= s=2014.06.06.t u=2.0])

Attachments

(4 attachments, 1 obsolete attachment)

Posted file Layer Tree
We end up with the attached layer tree.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> Created attachment 8397326 [details]
> Layer Tree
> 
> We end up with the attached layer tree.

The tall weird shaped thebes layer is the reason we get tiling. It would be nice if layout could let us not have this layer, but I don't know why it's being created and maintained.
(Q for triage: Are there power consumption implications that would make us block?)
blocking-b2g: --- → 1.4?
One thing I realized is that it seems like once we've created a tiled layer it seems like it will stay tiled even if the conditions that caused it to be scrollable is no longer true.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> 
> The tall weird shaped thebes layer is the reason we get tiling. It would be
> nice if layout could let us not have this layer, but I don't know why it's
> being created and maintained.


I think the tiled ThebesLayer is there because the ColorLayer and ImageLayer that are above it are both position:fixed. We don't subtract from the visible region of scrolled content for these since async scrolling might make different parts of it visible.

This is a special case however, where the fixed contents covers the entire scrollport (and viewport), so the scrolled content could never be visible. We should probably this case, cc'ing Cwiis and tn who worked on this code recently.


(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> One thing I realized is that it seems like once we've created a tiled layer
> it seems like it will stay tiled even if the conditions that caused it to be
> scrollable is no longer true.

Yeah, we should fix that. I think we'd need LayerManager::CreateThebesLayerWithHint to return some extra information that tells us if the hint value resulted in a different layer backing type being created. Then we can make CreateOrRecycleThebesLayer only recycle the old layer if the new hint, and previous one match.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> One thing I realized is that it seems like once we've created a tiled layer
> it seems like it will stay tiled even if the conditions that caused it to be
> scrollable is no longer true.

Chris et al. comments on this?
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(chrislord.net)
Flags: needinfo?(bas)
(In reply to Milan Sreckovic [:milan] from comment #5)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> > One thing I realized is that it seems like once we've created a tiled layer
> > it seems like it will stay tiled even if the conditions that caused it to be
> > scrollable is no longer true.
> 
> Chris et al. comments on this?

I have no idea about the creation process. From the layers API perspective it could certainly be recreated non-tiled without a problem. The content would need to be re-rasterized though!
Flags: needinfo?(bas)
Not a blocker. Youtube is not a video case we do power measurements for. It uses the network so that probably sucks more energy than the delta of GPU and hwc. That having said, we should definitely fix this but I wouldn't block on this.
blocking-b2g: 1.4? → backlog
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> This is a special case however, where the fixed contents covers the entire
> scrollport (and viewport), so the scrolled content could never be visible.
> We should probably this case, cc'ing Cwiis and tn who worked on this code
> recently.

Yes, we should fix this. The patch is reasonably trivial I think, I can have a go.

> (In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> > One thing I realized is that it seems like once we've created a tiled layer
> > it seems like it will stay tiled even if the conditions that caused it to be
> > scrollable is no longer true.
> 
> Yeah, we should fix that. I think we'd need
> LayerManager::CreateThebesLayerWithHint to return some extra information
> that tells us if the hint value resulted in a different layer backing type
> being created. Then we can make CreateOrRecycleThebesLayer only recycle the
> old layer if the new hint, and previous one match.

I think we should fix this too - I occasionally see the Twitter app create a rotated layer instead of a tiled layer for the main scroll area, and when this happens, performance is dire. I'm not sure if it's worth demoting layers from tiles to non-tiles, but it's definitely worth going the other way round.


One question I have about this, why is HWC being disabled anyway, regardless of the tiled layer? It's made up of gralloc surfaces, so can't it continue to use HWC?
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Flags: needinfo?(chrislord.net)
Flags: needinfo?(nical.bugzilla)
(In reply to Chris Lord [:cwiiis] from comment #8)
> 
> One question I have about this, why is HWC being disabled anyway, regardless
> of the tiled layer? It's made up of gralloc surfaces, so can't it continue
> to use HWC?

It seems that TiledContentHost::GetRenderState() and HwcComposer2D::PrepareLayerList() does not support HWC's tiling rendering. when tiling layer is used, hwc's layers number seems to exceed supported hwc layer number. On tther recent b2g devices' case(like QRD8x26), hwc's layers number seems to be within hwc's capacity. I am going to create a bug for it.
> It seems that TiledContentHost::GetRenderState() and
> HwcComposer2D::PrepareLayerList() does not support HWC's tiling rendering.
> when tiling layer is used, hwc's layers number seems to exceed supported hwc
> layer number. On tther recent b2g devices' case(like QRD8x26), hwc's layers
> number seems to be within hwc's capacity. I am going to create a bug for it.

Add Bug 988851 for it.
Untested, but seems pretty trivial to me.
Attachment #8398016 - Flags: review?(tnikkel)
Jeff, think you could see if the patch I attached fixes your issue?
Flags: needinfo?(jmuizelaar)
Comment on attachment 8398016 [details] [diff] [review]
Allow fixed position items to fully occlude displayports

I'm guessing this isn't going to work. The visible region contains things that aren't on screen (the displayport) whereas fixed items are reflowed to the screen size (scroll position clamping scroll port size).
Attachment #8398016 - Flags: review?(tnikkel) → review-
(In reply to Timothy Nikkel (:tn) from comment #13)
> Comment on attachment 8398016 [details] [diff] [review]
> Allow fixed position items to fully occlude displayports
> 
> I'm guessing this isn't going to work. The visible region contains things
> that aren't on screen (the displayport) whereas fixed items are reflowed to
> the screen size (scroll position clamping scroll port size).

I think in this particular case, going on the comments, it'd work - but I agree that this isn't a good solution on the whole, for the reason you stated.

Would it work/be better if we checked if the opaque region covered the bounds of the displayport frame's display item, then set the visible region to empty if it does?
Flags: needinfo?(tnikkel)
Sorry for not replying sooner.

Yeah, it would probably work in cases without scrolling. Can we just use the scroll position clamping scroll port size? That would make the most sense for fixed pos stuff.
Flags: needinfo?(tnikkel)
Annoyed I didn't get to this before PTO. Needinfoing myself in case this isn't picked up again before I get back.

So I think the solution to this would be to look at the scroll position clamping scroll port size of the displayport frame (as suggested in comment #15), and if this is completely occluded by the fixed frame, set the opaque region to empty.
Assignee: chrislord.net → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(chrislord.net)
Keywords: perf
Priority: -- → P2
Whiteboard: [c=power p= s= u=]
Assignee: nobody → chrislord.net
blocking-b2g: backlog → 2.0+
Keywords: power
I'm not sure if the scroll clamping port needs offsetting or not, but I'd have thought not given it should share the same presshell as the display tree root(?)

I haven't tested this patch (other than to make sure it builds), so if you could test this pending review Jeff, I'd be grateful - if it does what I mean it to do, I expect that it would.
Attachment #8398016 - Attachment is obsolete: true
Attachment #8433949 - Flags: review?(tnikkel)
Flags: needinfo?(chrislord.net)
Comment on attachment 8433949 [details] [diff] [review]
Allow fixed position items to fully occlude displayports v2

Can you just use nsRegion::Contains(nsRect) instead of the SubtractFromVisibleRegion dance?
Attachment #8433949 - Flags: review?(tnikkel) → review+
Priority: P2 → P1
Whiteboard: [c=power p= s= u=] → [c=power p= s= u=2.0]
YouTube is refusing to play any videos for me at all, do we have any alternative sites or a reduced test-case for this? Or can someone else test this with the patch applied and report back if it fixes the issue or not?
I also confirmed the problem. I am going to investigate the problem.
(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> I also confirmed the problem. I am going to investigate the problem.

I created a bug 1021183. But after that, it becomes clear that just my one build source seems to be broken.
(In reply to Chris Lord [:cwiiis] from comment #19)
> YouTube is refusing to play any videos for me at all, do we have any
> alternative sites or a reduced test-case for this? Or can someone else test
> this with the patch applied and report back if it fixes the issue or not?

I confirmed that the problem is fixed on master flame.
Layer dump does not have tiled layer.
(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> (In reply to Chris Lord [:cwiiis] from comment #19)
> > YouTube is refusing to play any videos for me at all, do we have any
> > alternative sites or a reduced test-case for this? Or can someone else test
> > this with the patch applied and report back if it fixes the issue or not?
> 
> I confirmed that the problem is fixed on master flame.

Sorry, I checked the above without applying attachment 8433949 [details] [diff] [review].
Somehow, youtube fullscreen does not use tiled layer on master hamachi and on master flame.
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> Created attachment 8435194 [details]
> layer dump since fix
> 
> Layer dump does not have tiled layer.

Sorry, TiledContentHost is present in the log of master flame.
Attachment #8435194 - Attachment description: layer dump since fix → layer dump on master flame.
Sorry, for the confusion. On current master tiled layer is created. From now I will check attachment 8433949 [details] [diff] [review] applied result.
After applying the patch, TiledContentHost is still present. But all of them is [not visible] like the following. And Hw Composer is used for youtube full screen rendering.

>     ThebesLayerComposite (0xad22d000) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible] [componentAlpha]
>        TiledContentHost (0xb0a32c40)
Thanks for the confirmation Sotaro, simplified it as tn suggested and pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/843c1dbab9d6
Flags: needinfo?(jmuizelaar)
https://hg.mozilla.org/mozilla-central/rev/843c1dbab9d6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Duplicate of this bug: 1015332
Inder, can you please check with Preeti for v1.4?
Blocks: 984663
No longer blocks: 1015332
blocking-b2g: 2.0+ → 1.4?
Flags: needinfo?(ikumar)
(In reply to Andreas Gal :gal from comment #7)
> Not a blocker. Youtube is not a video case we do power measurements for. It
> uses the network so that probably sucks more energy than the delta of GPU
> and hwc. That having said, we should definitely fix this but I wouldn't
> block on this.

Per this comment, not a blocker for 1.4.
blocking-b2g: 1.4? → backlog
Flags: needinfo?(ikumar)
Whiteboard: [c=power p= s= u=2.0] → [c=power p= s=2014.06.06.t u=2.0]
Whiteboard: [c=power p= s=2014.06.06.t u=2.0] → [CR 669803][c=power p= s=2014.06.06.t u=2.0]
Whiteboard: [CR 669803][c=power p= s=2014.06.06.t u=2.0] → [caf priority: p1][CR 669803][c=power p= s=2014.06.06.t u=2.0]
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.