1252711 - Headers scrolling on digg.com are not smooth

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

1) Open http://www.digg.com
2) perform a fling
3) header and footer flicker on and off

Nexus 5 running Android 6.0, this is smooth on Aurora

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can reproduce this easily. WebIDE says the header is position:fixed, so something has gone horribly wrong. I wonder if the AGR budget stuff would have affected this?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

gfx regression with fixed position layers

Comment 3

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

OK, let's get the regression range.

Comment 4

[Teodora Vermesan (:TeoVermesan)]

regression range:
good build: 25-01
bad build: 26-01

pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=67c66c2878aed17ae3096d7db483ddbb2293c503&tochange=c0ba5835ca489d15f8f170d5deb01f8dad92709a

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Bug 1231538 seems like the likely culprit.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I can't repro this either on current nightly, current aurora, or the march 02 nightly. Maybe the site changed. Kevin/Snorp, can you still repro? If so, can you save the page (assuming it still repro's there) and attach it?

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

Yes I can still reproduce. https://www.youtube.com/watch?v=dEOBbPtcr_Q

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Can you save the page and repro it on that?

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

Attachment: 1252711-digg-save.zip

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

Yes. I can repro after pushing that folder to /sdcard/. I'll see what I can do about minimizing it some.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I can't repro using that zip either. I unpacked it to http://people.mozilla.org/~kgupta/bug/1252711/ to try it out. At this point I'm thinking it has to do the with layerizing budget, on my smaller-screen Nexus 4 we can layerize the things properly but on higher-resolution devices we don't layerize the same way and that's causing issues.

Comment 12

[Jamie Nicol [:jnicol] out of office until 6th Jan]

The AGR budget only affects AGRs which are created due to animated offset or margin properties, not position:fixed. I highly doubt it's making a difference here.

FWIW I can reproduce this on my 720x1280 Sony Z5 compact.

Comment 13

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Seeing as I can reproduce I'll take a deeper look at this.

Comment 14

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Oops. Snorp, Kevin: had you guys reduced your layers.max-active value? Mine was at 3 but resetting it to 20 "fixed" this for me.

This page ideally would only have 3 layers though - the main layer, the header, and the footer. But instead each news story is getting its own layer, so I'll have a look into why.

Comment 15

[Jamie Nicol [:jnicol] out of office until 6th Jan]

(Sorry for comment spam.)

But the bigger phone screen on my nexus 6 means that there are more than 20 stories on the page, so we reach the 20 limit anyway, so don't layerize the header and footer. That explains why Kats cannot reproduce on his Nexus 4, but why it does affect larger phones.

Comment 16

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Ugh, the card css class has transform-style:preserve-3d, so we need to layerize each one separately. Perhaps since they're not actually 3d transformed we could use inactive layers, however? Matt?

If not then I'm not sure what to do about this ticket - We could raise layers.max-active. I'm unsure if enforcing layers.max-active (bug 1231818) actually helped in terms of OOMs very much. We were talking about lowering it (bug 1252929), which might help with memory usage but will also make problems like this much more common.

Comment 17

[Matt Woodrow (:mattwoodrow)]

(In reply to Jamie Nicol [:jnicol] from comment #16)
> Ugh, the card css class has transform-style:preserve-3d, so we need to
> layerize each one separately. Perhaps since they're not actually 3d
> transformed we could use inactive layers, however? Matt?

Letting preserve-3d be inactive sounds fine, and probably what we want for things that aren't actually changing.

If a subtree of preserve-3d contains only 2d transforms, then preserve-3d isn't actually doing anything, so we should be fine to flatten that subtree.

If we don't have a a 3d transform on a given nsDisplayTransform, and all descendants are all 2d (RequiredLayerStateForChildren checks this), then we should be able to be inactive and flatten at that point.

We'd need to remove the check for mIsTransformSeparator in GetLayerState too.

Thinker, does that sound right to you?

Comment 18

[Matt Woodrow (:mattwoodrow)]

For reference, the document looks like the standard two sided 'card' setup:

<div perspective>
  <div preserve-3d>
    <div 'front' preserve-3d>
      // Front content
    </div>
    <div 'back' preserve-3d rotateY(180deg) backface-hidden>
      // Back content
    </div>
  </div>
</div>


I'm not sure what this is actually for, they don't seem to use a flip animation for anything I've tried.

We build a lot of layers for this, and they never get used for anything (and never have 3d), so we should not do this.

Comment 19

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> (In reply to Jamie Nicol [:jnicol] from comment #16)
> > Ugh, the card css class has transform-style:preserve-3d, so we need to
> > layerize each one separately. Perhaps since they're not actually 3d
> > transformed we could use inactive layers, however? Matt?
> 
> Letting preserve-3d be inactive sounds fine, and probably what we want for
> things that aren't actually changing.
> 
> If a subtree of preserve-3d contains only 2d transforms, then preserve-3d
> isn't actually doing anything, so we should be fine to flatten that subtree.
> 
> If we don't have a a 3d transform on a given nsDisplayTransform, and all
> descendants are all 2d (RequiredLayerStateForChildren checks this), then we
> should be able to be inactive and flatten at that point.
> 
> We'd need to remove the check for mIsTransformSeparator in GetLayerState too.
> 
> Thinker, does that sound right to you?

It looks right.  We may need to change GetLayerState() of nsDisplayPerspective and nsDisplayOpacity, too.  I am also thinking doing it without flattening items if nsDisplayTransform::Paint() is implemented by manipulating CTM of DrawTarget and painting children.

Comment 20

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Matt, Thinker, I tried to implement what you suggested in comment 17, but after doing that none of the cards were visible any more. I could see in the display list dump that we did start using basic layers to render them, but also a lot of the layers were marked [not visible].

Maybe I just didn't follow your instructions properly, but any ideas why that might be the case?

Comment 21

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Okay, I have worked out why nothing gets rendered using inactive layers - the visibility of a nested preserve3d nsDisplayTransform will not get computed, due to https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#6150. So it's treated as invisible and is not painted.

Commenting out that line means that we render the cards, but presumably that line serves a purpose. Can/should that condition be changed given that the condition in nsDisplayTransform::GetLayerState() is being changed?

If we can resolve that then we can paint the front face of the cards without crazy layerization. However, because the back face of the cards has a 3D transform they must be actively layerized. That makes sense to me. But because of RequiredLayerStateForChildren we end up not just layerizing the card backface div, but also its parent the card div. I don't understand why this is necessary, do we need to layerize all ancestors of a 3D transform? is this deliberate or a bug? If deliberate: is this too early to determine that the backface layer will be invisible, because backface-visibility=hidden, so we can therefore not layerize the parent.

Comment 22

[Thinker Li [:sinker]]

Jamie, do you ever try to change this line https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#5978?  Remove Combines3D*() and mIsTransformSeparator.  I wrote this line to be more defensive at that time.  I think we can change it, now.

We can not set any inactive layer in between two active layers on the tree of the same 3D rendering context.  If we do so, the transform of an active layer that rotate 90 degree along x or y would hide the content that is rotated to visible by another active layer.

Comment 23

[Jamie Nicol [:jnicol] out of office until 6th Jan]

(In reply to Thinker Li [:sinker] from comment #22)
> Jamie, do you ever try to change this line
> https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#5978?  Remove Combines3D*() and mIsTransformSeparator. 

Sorry, I should have been clearer. I already have made that change based on comment 17. Doing that caused nothing to get rendered, however. This is because of the line of code I linked to in comment 21, the nsDisplayTransform (and all its children) get computed as invisible so never get rendered. Commenting that line out causes the page to be rendered correctly again. Is that line still necessary, or is there a way around that?

Comment 24

[Thinker Li [:sinker]]

Could you point out which line checks the visible of an item, and remove the item from being painted?
For items with active layers, this line https://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?case=true&from=FrameLayerBuilder.cpp#3858 makes sure layers participating preserve-3d context are always drawn.  You need some similar code to force painting/or build inactive layer for items.

Consider a container layer with a transform of rotation along X-axis for 90 degree.  It's bounds would be empty, but one of its ancestors may rotate it back.  So, you will not get correct bounds for some cases.

Comment 25

[Matt Woodrow (:mattwoodrow)]

(In reply to Jamie Nicol [:jnicol] from comment #21)
> Okay, I have worked out why nothing gets rendered using inactive layers -
> the visibility of a nested preserve3d nsDisplayTransform will not get
> computed, due to
> https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#6150. So it's treated as invisible and is not painted.
> 
> Commenting out that line means that we render the cards, but presumably that
> line serves a purpose. Can/should that condition be changed given that the
> condition in nsDisplayTransform::GetLayerState() is being changed?

As thinker pointed out, the 'true' bounds of an intermediate node in a preserve-3d might be empty when represented as a 2d rect, but not be empty in 3d.

Since we can't necessarily come up with a reasonable result for GetBounds(), we always just return empty and then make sure FrameLayerBuilder and the layers code know to treat it as visible regardless.

Check to see if we're missing a call to Layer::Extend3DContext() in the inactive layer rendering path?


> 
> If we can resolve that then we can paint the front face of the cards without
> crazy layerization. However, because the back face of the cards has a 3D
> transform they must be actively layerized. That makes sense to me. But
> because of RequiredLayerStateForChildren we end up not just layerizing the
> card backface div, but also its parent the card div. I don't understand why
> this is necessary, do we need to layerize all ancestors of a 3D transform?
> is this deliberate or a bug? If deliberate: is this too early to determine
> that the backface layer will be invisible, because
> backface-visibility=hidden, so we can therefore not layerize the parent.

Why does the back face have a 3d transform? Shouldn't it be fully flipped, so it's just reversed in 2d?

If any layer is active, then all of its ancestors must be active too. We can only have inactive within active, not the reverse.

Comment 26

[Jamie Nicol [:jnicol] out of office until 6th Jan]

> Check to see if we're missing a call to Layer::Extend3DContext() in the inactive layer rendering path?

BasicLayerManager::PaintSelfOrChildren() checks Layer::IsVisible() which checks Layer::Extend3DContext() so that's not the problem. We do try to paint the layer. The problem is that FrameLayerBuilder::DrawPaintedLayer() does not call RecomputeVisibilityForItems() for inactive layers, because the visibility should already have been computed. But because we use an empty bounds, the item has an empty visibility.

Comment 27

[Matt Woodrow (:mattwoodrow)]

(In reply to Jamie Nicol [:jnicol] from comment #26)
> > Check to see if we're missing a call to Layer::Extend3DContext() in the inactive layer rendering path?
> 
> BasicLayerManager::PaintSelfOrChildren() checks Layer::IsVisible() which
> checks Layer::Extend3DContext() so that's not the problem. We do try to
> paint the layer. The problem is that FrameLayerBuilder::DrawPaintedLayer()
> does not call RecomputeVisibilityForItems() for inactive layers, because the
> visibility should already have been computed. But because we use an empty
> bounds, the item has an empty visibility.

Right, so can we make FrameLayerBuilder::PaintItems bypass the visibility test if the item is a preserve-3d one?

Comment 28

[Jamie Nicol [:jnicol] out of office until 6th Jan]

> Right, so can we make FrameLayerBuilder::PaintItems bypass
> the visibility test if the item is a preserve-3d one?

We could do this, but the nsDisplayItem::Paint() implementation will probably not work correctly because its visible rect is wrong.

We could make FrameLayerBuilder::DrawPaintedLayer() call RecomputeVisibilityForItems() when aLayer->GetParent()->Combines3DTransformWithAncest1ors(). That seems to work.

To go back to an earlier question you asked:
> Why does the back face have a 3d transform?
> Shouldn't it be fully flipped, so it's just reversed in 2d?

Matrix4x4::Is2D() treats this transform as 3d because _33 == -1.0f instead of 1.0f. Changing that condition to "abs(_33) != 1.0f" works. Should this be done?

With those changes the page renders, but the card backface is also rendered. I suppose we will now also need to handle backface-visibility=hidden somewhere.

Comment 29

[Matt Woodrow (:mattwoodrow)]

(In reply to Jamie Nicol [:jnicol] from comment #28)
> > Right, so can we make FrameLayerBuilder::PaintItems bypass
> > the visibility test if the item is a preserve-3d one?
> 
> We could do this, but the nsDisplayItem::Paint() implementation will
> probably not work correctly because its visible rect is wrong.
> 
> We could make FrameLayerBuilder::DrawPaintedLayer() call
> RecomputeVisibilityForItems() when
> aLayer->GetParent()->Combines3DTransformWithAncest1ors(). That seems to work.

Shouldn't it only be the intermediate nsDisplayTransform items that have empty visible regions?

nsDisplayTransform paints using PaintInactiveLayer, it doesn't have a ::Paint() function at all.

> 
> To go back to an earlier question you asked:
> > Why does the back face have a 3d transform?
> > Shouldn't it be fully flipped, so it's just reversed in 2d?
> 
> Matrix4x4::Is2D() treats this transform as 3d because _33 == -1.0f instead
> of 1.0f. Changing that condition to "abs(_33) != 1.0f" works. Should this be
> done?
> 
> With those changes the page renders, but the card backface is also rendered.
> I suppose we will now also need to handle backface-visibility=hidden
> somewhere.

Ah, right. That's a little harder, we've always treated 3d as active and we have reftests that rely on this to be useful.

Comment 30

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

Jamie, do you know, or can you find out if this is realistically a "bug fix", or a fundamental problem that will require significant effort and code change?

Comment 31

[Jamie Nicol [:jnicol] out of office until 6th Jan]

I am struggling a bit with it, but as far as I can tell this should be fixable.

Comment 32

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Digg have changed their website so this is no longer an issue, so I'm going to close this as worksforme.

However, I've opened bug 1274528 for ensuring too many active layers doesn't break async scrolling. And bug 1274532 for using inactive layers for 2d preserve3d transforms.