722923 - Absolutely positioned div containing text is unexpectedly invalidated inside an outer transformed frame

Status: NEW

(Core :: Layout, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The lock screen is fairly simple: an absolutely positioned <div> with an image and some text in it.  It's moved around on screen with CSS translations in response to touchmove events.

The lock screen was horrifically slow initially but got better with ajuma's fixes for texture uploading on EGL.  We still weren't able to repaint quickly enough to keep up with input events, so there was some lag.

Recently we added a gradient (ohnoes!) to the lock screen and that slowed the framerate down.  It's regressed around to the point it was before the texture-upload fix.

This shouldn't be the case!  In the meantime we've landed two optimizations that should apply perfectly to the lock screen
 - if an active transformed frame is the size of the screen or smaller, speculatively paint its entire computed size instead of just its visible region.
   * The lock screen is transformed and, well, the size of the screen.
 - don't reflow transformed frames on transform changes (--> don't invalidate)
   * The lock screen is only ever transformed.

I'm going to poke around and see what's slow in here.  This is one of the simplest possible cases to optimize.  We should just be blitting a prerendered texture at the position specified by the CSS transform.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I should add, this is slow on the Galaxy S II, which has a high-end GPU and tons of memory bandwidth.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Confirmed that replacing the lock screen contents with a bare pink screen fixes perf problem.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

While the frame is moving around, I see

Painting --- before optimization (dirty 0,0,28800,48000):

which is 480x800 pixels.  We *are* hitting the prerender-partially-visible-frame optimization, but it's likely hurting us because we're invalidating the entire screen on each paint.

Will try to see how far I can get in figuring out what's overinvalidating.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Another small bit of relevant information

WIDGET: dirty <x=0,y=207,w=480,h=593>
Painting --- before optimization (dirty 0,0,28800,48000):

The visible area of the "lock screen", as it moves over the "home screen", is being dirtied in nsWindow.  This is unexpected after bug 524925 (to me!) because layout of the transformed frame can't change.  By the time get to nsLayoutUtils::PaintFrame, we've picked up the rest of the screen area to invalidate.  That might be because there's a <canvas> under the lock screen.  Poking some more.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With a plain-pink HTML background, instead of the canvas, same category of result

WIDGET: dirty <x=0,y=111,w=480,h=689>
Painting --- before optimization (dirty 0,0,28800,48000):

and still slow.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Painting --- before optimization (dirty 0,0,28800,48000):

That's not the area that's invalid in the widget. It's the area we're building display lists for, which is the whole window. That should be renamed...

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What's happening is that we get to

void
nsIFrame::InvalidateTransformLayer()
{
  NS_ASSERTION(mParent, "How can a viewport frame have a transform?");

  bool hasLayer =
      FrameLayerBuilder::GetDedicatedLayer(this, nsDisplayItem::TYPE_TRANSFORM) != nsnull;

but |hasLayer| is false o_O.  The layer tree begs to differ.  Poking some more.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

So,

-  bool hasLayer =
-      FrameLayerBuilder::GetDedicatedLayer(this, nsDisplayItem::TYPE_TRANSFORM) != nsnull;
+  bool hasOrWillHaveLayer =
+    AreLayersMarkedActive(nsChangeHint_UpdateTransformLayer);

results in the behavior I expect: one paint when the frame goes active and we layerize, and nothing thereafter.  Still groping around in the dark somewhat.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Forgot to add, the speculative patch in comment 8 makes the on-device lockscreen fast enough to keep up with input events.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Small testcase

While the transition is active, the text within the transformed frame is invalidated and repainted.  Commenting out the rule marked

/* Comment out this rule, and the extra repaints stop. */

makes the extra repaints stop, as you might expect.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

More info for my records.

BAD display list (unexpected invalidation)

REPAINTING THEBES <x=0,y=0,w=73,h=19>
Painting --- after optimization:
Clip 0x4364cb58(FrameOuter(browser)(4)) (0,0,28800,48000)(0,0,28800,48000) opaque
    OwnLayer 0x43f2e808(Viewport(-1)) (0,0,28800,48000)(0,0,28800,48000) opaque layer=0x40c3dda0
        Clip 0x43f29408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,28800,48000) opaque
            Background 0x43f43e58(HTMLScroll(body)(2)) (0,0,28800,48000)(0,0,28800,39312) opaque uniform layer=0x436a5c00
            nsDisplayTransform 0x43f29408(HTMLScroll(div)(1)) (0,39312,28800,48001)(0,39312,28800,8688) opaque layer=0x40c3dae0
                Background 0x43f29408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,4380,1140) opaque uniform layer=0x4393b800
                Clip 0x43f29810(Block(div)(1)) (0,0,28800,48000)(0,0,4339,1140)
                    Text 0x43f8d7d0(Text(0)"Hello kitty") (0,0,4339,1140)(0,0,4339,1140) layer=0x4393b800


GOOD display list (no invalidation)

Painting --- after optimization:
Clip 0x4364cb58(FrameOuter(browser)(4)) (0,0,28800,48000)(0,0,28800,48000) opaque
    OwnLayer 0x43821808(Viewport(-1)) (0,0,28800,48000)(0,0,28800,48000) opaque layer=0x40c3dda0
        Clip 0x43f41408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,28800,48000) opaque
            Background 0x43f2ce58(HTMLScroll(body)(2)) (0,0,28800,48000)(0,0,28800,48000) opaque uniform layer=0x436a5800
            nsDisplayTransform 0x43f41408(HTMLScroll(div)(1)) (0,20927,28800,48001)(0,20927,28800,27073) layer=0x40c3dae0
                Background 0x43f41408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,28800,48000) opaque uniform layer=0x4393b800
                Clip 0x0() (0,0,28800,48000)(0,0,4339,1140)
                    Text 0x43f8d6e0(Text(0)"Hello kitty") (0,0,4339,1140)(0,0,4339,1140) layer=0x4393b800


The only major difference is "Clip 0x43f29810(Block(div)(1))" (bad) vs. "Clip 0x0()" (good).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1: fix

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: test

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Created attachment 593336 [details] [diff] [review]
> part 1: fix

Makes the slow go away on device.  Thanks!

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 3:Rename UpdateViewsForFrame and document it properly. r=matspal

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Don't walk continuation chain in SyncViewsAndInvalidateDes cendants since DoApplyRenderingChangeToTree already does that

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: more cleanup

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 593336 [details] [diff] [review]
part 1: fix

Tragically this patch isn't right.

A frame F with 'opacity' can have a placeholder descendant whose out-of-flow frame is not a descendant of F. In that situation, this patch will cause us to not update the out-of-flow frame correctly. I will write a test for this.

A similar situation might apply with a transformed inline that contains a float placeholder. Need to test since I have no idea how that works currently :-).

What we really need here is a way to quickly identify the situation where a frame F has no placeholder descendants whose out-of-flows are outside F. That's the only situation where UpdateViewsForTree is needed when nsChangeHint_SyncFrameView is not present. Currently, when we have only nsChangeHint_RepaintFrame, it walks the entire frame subtree checking for this one rare situation.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Comment on attachment 593336 [details] [diff] [review]
> part 1: fix
> 
> Tragically this patch isn't right.
> 
> A frame F with 'opacity' can have a placeholder descendant whose out-of-flow
> frame is not a descendant of F. In that situation, this patch will cause us
> to not update the out-of-flow frame correctly. I will write a test for this.
> 
> A similar situation might apply with a transformed inline that contains a
> float placeholder. Need to test since I have no idea how that works
> currently :-).

Transforms are only supposed to apply to block-level or atomic-inline elements, which means they must be the containing blocks for any float descendants. And they're defined to be the containing block for any positioned descendants too, so transforms are fine here.

For now, I'll fix the patch to only handle transforms and leave opacity with sometimes-unnecessary invalidation.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 0: reftest for the opacity invalidation I broke

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1 v2

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi Mats, do you have an ETA on reviews here?  We critically need these fixes, see bug 724189 comment 16.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 593625 [details] [diff] [review]
Part 1 v2

> layout/base/nsCSSFrameConstructor.cpp
>+    // We don't need to update transforms in UpdateViewsForTree, because
>+    // there can't be any out-of-flows or popups that need to be transformed;
>+    // all out-of-flow descendants of the transformed element must also be
>+    // descendants of the transformed frame.

IOW, an element with a transform makes it an abs.pos. container.

Could we assert that the assumption holds first in ApplyRenderingChangeToTree() ?
Something like:
  NS_ASSERTION(!(aChange & nsChangeHint_UpdateTransformLayer) ||
               aFrame->GetStyleDisplay()->HasTransform(), "");

r=mats

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 593363 [details] [diff] [review]
Part 5: more cleanup

>+      // skip out of flows, they'll be handled as we traverse through placeholders

nit: s/out of flows/out-of-flows/ and make it less than 80 columns

>-          if ((child->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER) &&
>-              (aChange & nsChangeHint_RepaintFrame)) {
>-            FrameLayerBuilder::InvalidateThebesLayerContents(child,
>-              child->GetVisualOverflowRectRelativeToSelf());
>-          }
>...
>@@ -7724,16 +7724,17 @@ DoApplyRenderingChangeToTree(nsIFrame* a
>...
>+      FrameLayerBuilder::InvalidateThebesLayersInSubtree(aFrame);

SyncViewsAndInvalidateDescendants calls DoApplyRenderingChangeToTree for
each descendant frame, but InvalidateThebesLayersInSubtree also recurse
on frame descendants in InternalInvalidateThebesLayersInSubtree so this
seems sub-optimal.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren [:mats] from comment #24)
> SyncViewsAndInvalidateDescendants calls DoApplyRenderingChangeToTree for
> each descendant frame,

Only for descendant out-of-flows and popups.

> but InvalidateThebesLayersInSubtree also recurse
> on frame descendants in InternalInvalidateThebesLayersInSubtree so this
> seems sub-optimal.

Only for descendants that have their own ContainerLayers.

SyncViewsAndInvalidateDescendants only needs to call DoApplyRenderingChangeToTree for out-of-flows that aren't descendants of the original frame. Maybe I should add a check for that?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed06563619f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6784eefe33a6

Please leave open after inbound landing, since more patches need to land.

Comment 27

[Mats Palmgren (inactive)]

> Only for descendant out-of-flows and popups.

Yes, we trampoline off the placeholder and skip all the out-of-flows
so we won't do them twice, but DoApplyRenderingChangeToTree calls
SyncViewsAndInvalidateDescendants(aFrame), so for this frame tree:

top
  placeholder-1 => abs-pos-1
  abs-pos-1
     placeholder-2 => abs-pos-2
     abs-pos-2

we do these calls:
DoApplyRenderingChangeToTree(abs-pos-1)
  SyncViewsAndInvalidateDescendants(abs-pos-1)
    DoApplyRenderingChangeToTree(abs-pos-2)
      SyncViewsAndInvalidateDescendants(abs-pos-2)
      InvalidateThebesLayersInSubtree(abs-pos-2)
  InvalidateThebesLayersInSubtree(abs-pos-1)


InvalidateThebesLayersInSubtree recurse all descendants unconditionally:
http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1951
so InvalidateThebesLayersInSubtree(abs-pos-1) will process abs-pos-2.

> SyncViewsAndInvalidateDescendants only needs to call
> DoApplyRenderingChangeToTree for out-of-flows that aren't descendants of
> the original frame.

Hmm, how do we process UpdateOpacityLayer for abs-pos-1 if we skip calling
DoApplyRenderingChangeToTree for it?

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ed06563619f6
https://hg.mozilla.org/mozilla-central/rev/6784eefe33a6

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 593348 [details] [diff] [review]
Part 2: test

The test file is chock full of \r.

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 593363 [details] [diff] [review]
Part 5: more cleanup

r- until comment 27 is addressed (to avoid Bugzilla review reminders)

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's not do anything more here until DLBI has landed.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The test invalidates well for me, but I don't remember all the work we starting doing here.  Is there anything we want to keep post-DLBI?

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looks like there's a test from bug 724189 (attachment 594634 [details] [diff] [review]) that we wanted to land along with this.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll land parts 3 and 4.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a1e0e44245
https://hg.mozilla.org/integration/mozilla-inbound/rev/329f3f03ee1c

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3a1e0e44245
https://hg.mozilla.org/mozilla-central/rev/329f3f03ee1c