Closed Bug 876321 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: dbaron, Assigned: roc)

References

(Blocks 3 open bugs)

Details

(Keywords: perf)

Attachments

(1 file)

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.
(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.)
Blocks: 302909
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.
Assignee: nobody → roc
Attached patch fixSplinter Review
Attachment #810333 - Flags: review?(matt.woodrow)
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.
Attachment #810333 - Flags: review?(matt.woodrow) → review+
It seemed a little less confusing to store them individually.
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.
Instead of fuzzing Android (a lot) I decided to investigate why these differences occur. They didn't occur on B2G.
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.
It looks like it could be something to do with CompositorOGL::PrepareViewport!
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
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).
(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.
That works well on try. It actually makes a number of currently failing tests pass.
https://hg.mozilla.org/mozilla-central/rev/8511ef9264fa
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.