Closed Bug 1381753 Opened 2 years ago Closed 2 years ago

Google login box progress bar is flickering

Categories

(Core :: Web Painting, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: shinglyu, Assigned: mattwoodrow, NeedInfo)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Screen recording: https://www.youtube.com/watch?v=KSu0EJncH1U&feature=youtu.be

STR:
* Log out of Google
* Re-login into google (I'm logging into Google Drive with mozilla.com email

Expected: 
The orange progress bar on the login box has smooth animation.

Actual: 
The organe progress bar is choppy and flickering.
I saw that the other day too but I also saw it with stylo disabled.
I just tried the Google sign in page again and no longer see the issues with the animation.  Shing, is it fixed for you now too?
Flags: needinfo?(shing.lyu)
(In reply to Cameron McCormack (:heycam) from comment #1)
> I saw that the other day too but I also saw it with stylo disabled.

I can reproduce this bug with stylo disabled on latest Nightly. No such issue on firefox 54.0.1 (64-bit). So it is not stylo related bug.
This seems to be a regression of bug 1373479.
Thanks for searching for the regressing bug, Adolfo.  Matt or Markus, could you take a look?
Flags: needinfo?(mstange)
Flags: needinfo?(matt.woodrow)
Summary: stylo: Google login box progress bar is flickering → Google login box progress bar is flickering
No longer blocks: stylo-site-issues
Assignee: nobody → matt.woodrow
Status: NEW → ASSIGNED
Component: CSS Parsing and Computation → Layout: Web Painting
Flags: needinfo?(matt.woodrow)
(In reply to Adolfo Jayme from comment #4)
> This seems to be a regression of bug 1373479.

Are you sure about the regression range?

I reverted the change locally and I can still reproduce it.
This is the same as bug 1373479, except the harder version, where we have nested async transform animations.

When computing the visible region of a transformed Layer we:

* Call GetBounds on the nsDisplayTransform, which collects the bounds of all descendant items, and then transforms into post-transform coordinate space.

* Intersect this with the visible area for the transform item, though we skip this when the transform is going to be async.

* Inverse transform the result back into pre-transform coords.

The resulting visible region isn't dependent on the current transform, so we can change the transform async without issues.


When we have nested async nsDisplayTransform, the GetBounds call on the outer item recurses into the GetBounds call on the inner. The result of this inner call *is* dependent on the current transform value, and gets included into the visible area for the outer item.

When we async transform the inner item, the visible region for the outer item is no longer correct, and we get invalidation issues.

It seems like the easiest thing to do would be to have an alternate version of GetBounds that takes this into account, and when it finds the async transformed descendant, it uses the clip instead of the bounds (since that inner item could occupy any area within the clip at some point).

This is somewhat similar to GetClippedBoundsWithRespectToASR, and could possibly be shared once the merging of AGRs and ASRs is completed.


What do you think Markus?
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> This is somewhat similar to GetClippedBoundsWithRespectToASR, and could
> possibly be shared once the merging of AGRs and ASRs is completed.

Exactly what I was thinking.

I think you proposal sounds good, but I realized that I don't understand this bug completely any more. Can you remind me why this only started causing bugs after bug 1361970? It's the invalidation on the compositor side that's going wrong, right? Shouldn't LayerManagerComposite have enough information to do the right thing, since it knows about the current transform?
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #9)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > This is somewhat similar to GetClippedBoundsWithRespectToASR, and could
> > possibly be shared once the merging of AGRs and ASRs is completed.
> 
> Exactly what I was thinking.
> 
> I think you proposal sounds good, but I realized that I don't understand
> this bug completely any more. Can you remind me why this only started
> causing bugs after bug 1361970? It's the invalidation on the compositor side
> that's going wrong, right? Shouldn't LayerManagerComposite have enough
> information to do the right thing, since it knows about the current
> transform?

The issue I'm trying to fix is that content computes a visible rect for the transform layer that doesn't account for async transform changes in its descendants. You're correct in that this isn't a new bug, nor a regression from bug 1361970.

The regression is that LayerManagerComposite::PostProcessLayers used to recompute the visible regions for all ContainerLayers in the tree from scratch, so it would overwrite the incorrect visible regions with correct ones, taking the current value of the async transform into account. The new PostProcessLayers only updates visible regions for ContainerLayers that have an intermediate surface (since this is the coordinate space the effective transforms of the leaves transforms into), and doesn't modify other ContainerLayers (since their visible region isn't used by rendering).

When the outer transform changes, LayerTreeInvalidation uses the visible region to compute an invalid region, so where this previously invalidated the right area, now it does not.

Getting LayerTreeInvalidation to only use effective transforms would also fix this, but we sometimes want to generate invalidations for subdoc ContainerLayers (which might not have an intermediate surface). I haven't figured out a sane way to resolve that yet, and I think getting the right region on the client side is worth fixing regardless.
Thanks!
Actually, that doesn't work :(

Trimmed excerpt from the testcase:

nsDisplayTransform p=0x131788398 f=0x12f37c380(Block(div)(2) class:MyvhI TKVRUb) bounds(0,0,27000,240) layerBounds(0,0,27000,240) visible(0,0,802,240) componentAlpha(0,0,0,0) clip(0,0,27000,240) asr(<0x10f5605b0>) clipChain(<0,0,27000,240> [0x10f5605b0]) ref=0x12f33aa30 agr=0x12f37c380[ 1 0; 0 1; -937.27 0; ] layer=0x12df30400
    nsDisplayTransform p=0x1317881c8 f=0x12e16ecb8(Block(span)(0) class:l3q5xe SQxu9c) bounds(0,0,0,0) layerBounds(0,0,0,0) visible(8272,0,10456,240) componentAlpha(0,0,0,0) clip() asr(<0x10f5605b0>) clipChain() ref=0x12f37c380 agr=0x12e16ecb8[ 0.244988 0; 0 1; 339.756 0; ] layer=0x12f0ca400

The inner transform item has no clip (or clip chain) :(

The outer transform item is also async, so we can't take the clip from there and convert it into the inner coord space.

As far as I can tell, the only 'correct' visible region for the outer transform ContainerLayer that correctly takes into account that both inner and outer can move asynchronously is an infinite region, which we don't support.
Flags: needinfo?(shing.lyu)
How do you feel about doing this?

This is just accepting that the content provided visible regions aren't valid for ContainerLayers, and makes sure we recompute them.
Attachment #8895206 - Flags: review?(mstange)
Comment on attachment 8895206 [details] [diff] [review]
Recompute visible regions for intermediate ContainerLayers

Review of attachment 8895206 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, it's a bit unfortunate but no worse than what we had before.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +730,5 @@
> +    return LayerIntRect();
> +  }
> +
> +  Rect rect(aRect.x, aRect.y, aRect.width, aRect.height);
> +  rect = aTransform.TransformAndClipBounds(rect, Rect::MaxIntRect());

Hmm, doesn't look like we have a coordinate-space-aware version of this. Ok.

@@ +760,5 @@
> +    // surface, but otherwise we need to do it now.
> +    mShadowVisibleRegion.SetEmpty();
> +
> +    for (Layer* l = GetFirstChild(); l; l = l->GetNextSibling()) {
> +      AddTransformedRegion(mShadowVisibleRegion, l->GetLocalVisibleRegion(), l->GetLocalTransform());

long line
Attachment #8895206 - Flags: review?(mstange) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8bb2bc1ac0
Recompute visible regions for ContainerLayers without intermediate surfaces when needed for invalidation. r=mstange
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78676939447f
Recompute visible regions for ContainerLayers without intermediate surfaces when needed for invalidation. r=mstange
https://hg.mozilla.org/mozilla-central/rev/78676939447f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1373479
Keywords: regression
I can see this issue again on latest Nightly
Depends on: 1451302
You need to log in before you can comment on or make changes to this bug.