876321 - use the layer system for handling dynamic changes of top/left/right/bottom

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should consider using the layer system for handling dynamic changes of the top, left, right, and bottom CSS properties, much like we do for transforms -- i.e., marking layers active when these properties are animating.

We could perhaps even use the same transform layer that we use for transforms, record on the frame some sort of origin based on an initial state of these properties, and then translate the transform to reflect the change, while being sure to correctly handle elements with a transform and offset properties.

This would be more valuable after bug 745485 lands, and I think bug 745485 is probably higher priority, but it's largely independent from a technical perspective.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And if we do choose to do this, we could consider doing off main thread animations as a followup... though I'm substantially less sure if that would be worth the work involved.)

Comment 2

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

Perhaps all we need to do to implement this is to make nsLayoutUtils::GetActiveScrolledRootFor treat abs-pos/rel-pos frames with animated left/top as active scrolled roots.

Comment 3

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

Attachment: fix

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 810333 [details] [diff] [review]
fix

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

::: layout/base/ActiveLayerTracker.cpp
@@ +50,5 @@
>    uint8_t mTransformRestyleCount;
> +  uint8_t mLeftRestyleCount;
> +  uint8_t mTopRestyleCount;
> +  uint8_t mRightRestyleCount;
> +  uint8_t mBottomRestyleCount;

Do we actually need to keep track of these separately?

It seems that we either update all of them at once, or update a single one but set it to 0xFF.

We only ever check them together, and they're either all identical, or some are at 0xFF. It wouldn't affect the logic to have just one.

Comment 5

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

It seemed a little less confusing to store them individually.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb9ea77d858

Comment 7

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

Backed out due to reftest failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb9ea77d858
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=47c7022e4e11

Looks like Windows needs a little fuzzing and Android seems to need a lot.

Comment 8

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

Instead of fuzzing Android (a lot) I decided to investigate why these differences occur. They didn't occur on B2G.

Comment 9

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

I spent a lot of time digging into this.

The reftest failures on try seem to show that the internal pixels of a moving abs-pos layer are resampled slightly, as if the layer's top/left is not perfectly integer-aligned.

The failures on my Nexus S are similar but different. Careful study of the failures suggests that the internal pixels of *all* layers are resampled slightly, as if the top/left is not integer aligned. The error is mostly consistent so doesn't itself cause reftest failures as long as the layer configuration stays the same. However, when tiling is enabled I see different patterns of errors in each tile, and disabling tiling shows yet another different pattern of errors.

Dumping program parameters in CompositorOGL::DrawQuad (and DrawQuads) doesn't show anything interesting. All parameters look as expected and there are no fractional values in the floating-point parameters (including texture coordinates, transforms, etc).

Forcing the layer resampling filter to GL_NEAREST in CompositorOGL::DrawQuad fixes the problem on my Nexus S, which seems to confirm the resampling hypothesis.

The layers on my Nexus S use a 565 buffer. I tried switching to 888 but it made no difference.

Comment 10

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

It looks like it could be something to do with CompositorOGL::PrepareViewport!

Comment 11

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

No, false hope.

Comment 12

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

Matt suggested, plausibly, that the usage of lowp for all floats in fragment shaders might be a problem. But it doesn't seem to make a difference for me. Filed bug 937987 with a patch, anyway. I'll give it a run on try. https://tbpl.mozilla.org/?tree=Try&rev=92c3fe5d8913

Comment 13

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

I think I can work around this problem, whatever's causing it, by having CompositorOGL::DrawQuad detect when we're doing a pixel-aligned compositing operation and switch to GL_NEAREST in that case. Given I've already spent a couple of days on this, I think that's worth doing. If someone with more GL knowledge than me wants to chase the underlying problem further, they're welcome to do so. I would if I had any clue where to look next (and I was confident the problem was in our code, not a driver).

Comment 14

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Matt suggested, plausibly, that the usage of lowp for all floats in fragment
> shaders might be a problem. But it doesn't seem to make a difference for me.
> Filed bug 937987 with a patch, anyway. I'll give it a run on try.
> https://tbpl.mozilla.org/?tree=Try&rev=92c3fe5d8913

Doesn't help on try either.

Comment 15

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

Try push with the NEAREST workaround: https://tbpl.mozilla.org/?tree=Try&rev=599dc22c63c5

Comment 16

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

That works well on try. It actually makes a number of currently failing tests pass.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8511ef9264fa

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8511ef9264fa