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

NEW
Unassigned

Status

()

defect
8 years ago
3 years ago

People

(Reporter: cjones, Unassigned)

Tracking

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open after inbound merge], )

Attachments

(7 attachments, 1 obsolete attachment)

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.
I should add, this is slow on the Galaxy S II, which has a high-end GPU and tons of memory bandwidth.
Confirmed that replacing the lock screen contents with a bare pink screen fixes perf problem.
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.
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.
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.
(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...
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.
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.
Forgot to add, the speculative patch in comment 8 makes the on-device lockscreen fast enough to keep up with input events.
Component: General → Layout
QA Contact: general → layout
Summary: Tracking: Make gaia lock screen fast → Absolutely positioned div containing text is unexpectedly invalidated inside an outer transformed frame
Posted file 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.
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).
Posted patch part 1: fix (obsolete) — Splinter Review
Assignee: nobody → roc
Attachment #593336 - Flags: review?(matspal)
(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 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.
Attachment #593336 - Attachment is obsolete: true
Attachment #593336 - Flags: review?(matspal)
(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.
Hi Mats, do you have an ETA on reviews here?  We critically need these fixes, see bug 724189 comment 16.

Updated

8 years ago
Attachment #593348 - Flags: review?(matspal) → review+

Updated

8 years ago
Attachment #593624 - Flags: review?(matspal) → review+

Comment 23

8 years ago
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
Attachment #593625 - Flags: review?(matspal) → review+

Updated

8 years ago
Attachment #593361 - Flags: review?(matspal) → review+

Updated

8 years ago
Attachment #593362 - Flags: review?(matspal) → review+

Comment 24

8 years ago
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.
(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?
Whiteboard: [leave open after inbound merge]

Comment 27

8 years ago
> 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?

Updated

8 years ago
Depends on: 725535
Comment on attachment 593348 [details] [diff] [review]
Part 2: test

The test file is chock full of \r.

Updated

7 years ago
Depends on: 727601

Comment 30

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

r- until comment 27 is addressed (to avoid Bugzilla review reminders)
Attachment #593363 - Flags: review?(matspal) → review-
Let's not do anything more here until DLBI has landed.

Updated

7 years ago
No longer blocks: b2g-demo-phone
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?
Looks like there's a test from bug 724189 (attachment 594634 [details] [diff] [review]) that we wanted to land along with this.
Assignee: roc → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.