Closed
Bug 988511
Opened 11 years ago
Closed 10 years ago
Youtube does not use hardware composer in fullscreen with tiling enabled
Categories
(Core :: Graphics: Layers, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | fixed |
People
(Reporter: jrmuizel, Assigned: cwiiis)
References
Details
(Keywords: perf, power, Whiteboard: [caf priority: p1][CR 669803][c=power p= s=2014.06.06.t u=2.0])
Attachments
(4 files, 1 obsolete file)
We end up with the attached layer tree.
Reporter | ||
Comment 1•11 years ago
|
||
(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.
Comment 2•11 years ago
|
||
(Q for triage: Are there power consumption implications that would make us block?)
blocking-b2g: --- → 1.4?
Reporter | ||
Comment 3•11 years ago
|
||
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.
Comment 4•11 years ago
|
||
(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.
Comment 5•11 years ago
|
||
(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)
Comment 6•11 years ago
|
||
(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)
Comment 7•11 years ago
|
||
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
Assignee | ||
Comment 8•11 years ago
|
||
(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)
Updated•11 years ago
|
Flags: needinfo?(nical.bugzilla)
Comment 9•11 years ago
|
||
(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.
Comment 10•11 years ago
|
||
> 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.
Assignee | ||
Comment 11•11 years ago
|
||
Untested, but seems pretty trivial to me.
Attachment #8398016 -
Flags: review?(tnikkel)
Assignee | ||
Comment 12•11 years ago
|
||
Jeff, think you could see if the patch I attached fixes your issue?
Flags: needinfo?(jmuizelaar)
Comment 13•11 years ago
|
||
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-
Assignee | ||
Comment 14•11 years ago
|
||
(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)
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
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)
Updated•10 years ago
|
Updated•10 years ago
|
Assignee: nobody → chrislord.net
blocking-b2g: backlog → 2.0+
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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+
Updated•10 years ago
|
Priority: P2 → P1
Whiteboard: [c=power p= s= u=] → [c=power p= s= u=2.0]
Assignee | ||
Comment 19•10 years ago
|
||
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?
Comment 20•10 years ago
|
||
I also confirmed the problem. I am going to investigate the problem.
Comment 21•10 years ago
|
||
(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.
Comment 22•10 years ago
|
||
(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.
Comment 23•10 years ago
|
||
Layer dump does not have tiled layer.
Comment 24•10 years ago
|
||
(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].
Comment 25•10 years ago
|
||
Somehow, youtube fullscreen does not use tiled layer on master hamachi and on master flame.
Comment 26•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8435194 -
Attachment description: layer dump since fix → layer dump on master flame.
Comment 27•10 years ago
|
||
Sorry, for the confusion. On current master tiled layer is created. From now I will check attachment 8433949 [details] [diff] [review] applied result.
Comment 28•10 years ago
|
||
Comment 29•10 years ago
|
||
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)
Assignee | ||
Comment 30•10 years ago
|
||
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)
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/843c1dbab9d6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
status-b2g-v2.0:
--- → fixed
Comment 33•10 years ago
|
||
Inder, can you please check with Preeti for v1.4?
Comment 34•10 years ago
|
||
(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
Updated•10 years ago
|
Whiteboard: [c=power p= s= u=2.0] → [c=power p= s=2014.06.06.t u=2.0]
Updated•10 years ago
|
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]
Updated•10 years ago
|
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]
Updated•10 years ago
|
Blocks: CAF-v2.0-FC-metabug
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•