988511 - Youtube does not use hardware composer in fullscreen with tiling enabled

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Layer Tree

We end up with the attached layer tree.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

(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

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

(Q for triage: Are there power consumption implications that would make us block?)

Comment 3

[Jeff Muizelaar [:jrmuizel]]

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

[Matt Woodrow (:mattwoodrow)]

(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

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

(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?

Comment 6

[Bas Schouten (:bas.schouten)]

(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!

Comment 7

[Andreas Gal :gal]

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.

Comment 8

[Chris Lord [:cwiiis]]

(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?

Comment 9

[Sotaro Ikeda [:sotaro]]

(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

[Sotaro Ikeda [:sotaro]]

> 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.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Allow fixed position items to fully occlude displayports

Untested, but seems pretty trivial to me.

Comment 12

[Chris Lord [:cwiiis]]

Jeff, think you could see if the patch I attached fixes your issue?

Comment 13

[Timothy Nikkel (:tnikkel)]

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).

Comment 14

[Chris Lord [:cwiiis]]

(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?

Comment 15

[Timothy Nikkel (: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.

Comment 16

[Chris Lord [:cwiiis]]

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.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Allow fixed position items to fully occlude displayports v2

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.

Comment 18

[Timothy Nikkel (:tnikkel)]

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?

Comment 19

[Chris Lord [:cwiiis]]

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

[Sotaro Ikeda [:sotaro]]

I also confirmed the problem. I am going to investigate the problem.

Comment 21

[Sotaro Ikeda [:sotaro]]

(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

[Sotaro Ikeda [:sotaro]]

(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

[Sotaro Ikeda [:sotaro]]

Attachment: layer dump on master flame.

Layer dump does not have tiled layer.

Comment 24

[Sotaro Ikeda [:sotaro]]

(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

[Sotaro Ikeda [:sotaro]]

Somehow, youtube fullscreen does not use tiled layer on master hamachi and on master flame.

Comment 26

[Sotaro Ikeda [:sotaro]]

(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.

Comment 27

[Sotaro Ikeda [:sotaro]]

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

[Sotaro Ikeda [:sotaro]]

Attachment: layer dump after applying the patch

Comment 29

[Sotaro Ikeda [:sotaro]]

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)

Comment 30

[Chris Lord [:cwiiis]]

Thanks for the confirmation Sotaro, simplified it as tn suggested and pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/843c1dbab9d6

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/843c1dbab9d6

Comment 33

[Sushil]

Inder, can you please check with Preeti for v1.4?

Comment 34

[Jason Smith [:jsmith]]

(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.