If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[css3-transitions] Scale transition visually jumpy over long time and short distance

VERIFIED FIXED in Firefox 20

Status

()

Core
Graphics
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: rob, Assigned: roc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 wontfix, firefox19 wontfix, firefox20 verified, b2g18 wontfix)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 538845 [details]
Testcase showing jumpy scale transition

Applying a slow and short scale transition causes a visual jumpy effect. However, tests with the rotate transition seem perfectly smooth.

Another example of bug can be found here (from the original reporter): http://jsbin.com/ibiwo6. This one is absolutely smooth in Chrome on the Mac.
Weird. Bug 637852 doesn't seem to fix it. Probably something to do with the rounding of effective transforms that we do.
Depends on: 637852
Blocks: 667928

Comment 2

6 years ago
Animating "transform: scale()" values above 1 is _very_ slow on firefox (mac and pc). Chrome (mac and pc) and IE9 are running smooth no matter how high the value is.

Values under 1 is working flawless!

Tested with css-transition and scriptet transitions.
Have you got a testcase for that? Probably should be a new bug.
Blocks: 680703

Comment 4

6 years ago
For web developers there's an easy workaround: adding any amount of rotate() to the transition will smooth it out, even if the amount is too small to produce a noticeable effect:

http://jsfiddle.net/mennovanslooten/rNBGW/
That's really unusual.

Do we know this is definitely a graphics bug?

Comment 6

6 years ago
If I take the jsfiddle from comment #4 and increase the transform time to 10 minutes (600s), and zoom in with Mac OS's screen zoom (hold ctrl and scroll up/down with the touchpad), it almost looks like it's scaling alternatingly along the horizontal and vertical axes, and after each step the 'previous' axis along which scaling happened 'snaps' back. You can see, for example, that the razor-like shape of the fox's hair on the left side disappears under the edge of the jsfiddle frame border (making for a smooth border from the jsfiddle grey to the fox orange), and then reappears (with the white inbetween the hair of the fox).

No idea *why* it'd do that, though.
It's the snapping that we do in Layer::SnapTransform that causes it.

Comment 8

6 years ago
Not sure if this is related, but for people testing the demo at https://developer.mozilla.org/en-US/demos/detail/the-box/launch, they get max one frame per second

Updated

5 years ago
Blocks: 770810

Comment 9

5 years ago
(In reply to Timothy Nikkel (:tn) from comment #7)
> It's the snapping that we do in Layer::SnapTransform that causes it.

Can we detect the transition/animation cases and only snap at 0% and 100%?
I think there's something more subtle going on than just the fact we're snapping. In the testcase in comment #0, as the black box grows there are cases where a top row of pixels is painted black and then painted white again for a moment before returning to black. That should not happen even with snapping.

Comment 11

5 years ago
I just spotted this bug in Gaia. Adding a "rotate(0.01deg)" makes the animations much smoother.
Blocks: 633097
http://www.mikematas.com/preventi/ seems to suffer from this
Created attachment 689442 [details] [diff] [review]
Part 1: When determining whether the scale factors have changed, we need to use GetBaseTransform for the old scale factors, because that's what stores the old transform that we computed scale factors
Assignee: nobody → roc
Attachment #689442 - Flags: review?(matt.woodrow)
Created attachment 689443 [details] [diff] [review]
Part 2: Move TransformRectToRect from nsCSSRendering to gfxUtils
Attachment #689443 - Flags: review?(matt.woodrow)
Created attachment 689445 [details] [diff] [review]
Part 3: Refactor layer transform snapping to distinguish translation-snapping from rect-snapping
Attachment #689445 - Flags: review?(matt.woodrow)
Attachment #689442 - Flags: review?(matt.woodrow) → review+
Attachment #689443 - Flags: review?(matt.woodrow) → review+
Comment on attachment 689445 [details] [diff] [review]
Part 3: Refactor layer transform snapping to distinguish translation-snapping from rect-snapping

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

Looks good!
Attachment #689445 - Flags: review?(matt.woodrow) → review+
https://tbpl.mozilla.org/?tree=Try&rev=6d7b85bce0cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/270391fca858
https://hg.mozilla.org/integration/mozilla-inbound/rev/d89ae7f40549
https://hg.mozilla.org/integration/mozilla-inbound/rev/55d5f3cd5c85
Backed out for assertions (or at least this looked like the most likely candidate in the push):
eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=17707718&tree=Mozilla-Inbound#error0

https://hg.mozilla.org/integration/mozilla-inbound/rev/df91b13737cf
Sigh ... that assertion did not, of course, show up in my try run.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=55d5f3cd5c85
Just part 1:
https://tbpl.mozilla.org/?tree=Try&rev=9e06137399de
The Android test failures appear to be due to canvas not rendering at all on Android...
I wonder whether it's a compiler bug.
Try disabling optimizations in a couple of functions:
https://tbpl.mozilla.org/?tree=Try&rev=d7e120ec11e3
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Just part 1:
> https://tbpl.mozilla.org/?tree=Try&rev=9e06137399de

This part triggers the assertions in 598726-1.html. Weeird.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> Try disabling optimizations in a couple of functions:
> https://tbpl.mozilla.org/?tree=Try&rev=d7e120ec11e3

Compilation failed. Trying again: https://tbpl.mozilla.org/?tree=Try&rev=ff813fd1b88f
OK, those pragmas did not help.
The part 1 assertion is
###!!! ASSERTION: GL should never need to update ThebesLayers in an empty transaction: 'Error', file ../../../gfx/layers/opengl/ThebesLayerOGL.cpp, line 879
The assertion happens when the transform in the testcase is updated by nsIFrame::TryUpdateTransformOnly. The transform's scale has changed from a scale of 1.0 to 1.0 + epsilon, but TryUpdateTransformOnly does a FuzzyEqual check and decides to allow it anyway, we update the transform and do an empty transaction. Then in ThebesLayerOGL we discover that the transform is not a 1.0 scale so we set PAINT_WILL_RESAMPLE (which was not true before!). The visible and valid regions do not completely fill the buffer (they are a single rectangle, but smaller than the buffer). Then we hit this code:

    if ((aFlags & PAINT_WILL_RESAMPLE) &&
        (!neededRegion.GetBounds().IsEqualInterior(destBufferRect) ||
         neededRegion.GetNumRects() > 1)) {
...
      // We need to validate the entire buffer, to make sure that only valid
      // pixels are sampled
      neededRegion = destBufferRect;

That means we suddenly need to render parts of the buffer that weren't already rendered, but this is an empty transaction, so we can't.
Created attachment 691208 [details] [diff] [review]
Part 0.5: Mark layers that could have their transforms changed via off-main-thread animations or empty transactions, and treat all ThebesLayerOGL descendants of such layers as potentially resampled
Attachment #691208 - Flags: review?(matt.woodrow)
Comment on attachment 691208 [details] [diff] [review]
Part 0.5: Mark layers that could have their transforms changed via off-main-thread animations or empty transactions, and treat all ThebesLayerOGL descendants of such layers as potentially resampled

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

This ended up being a lot simpler than I imagined it would be :)

::: gfx/layers/Layers.h
@@ +580,5 @@
> +     * This indicates that the transform may be changed on during an empty
> +     * transaction where there is no possibility of redrawing the content, so the
> +     * implementation should be ready for that.
> +     */
> +    CONTENT_WILL_CHANGE_TRANSFORM = 0x08

CONTENT_MAY_CHANGE_TRANSFORM?
Attachment #691208 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #31)
> CONTENT_MAY_CHANGE_TRANSFORM?

OK
Created attachment 691213 [details] [diff] [review]
Part 0.5 v2

We actually do need to do this for all layer managers.
Attachment #691208 - Attachment is obsolete: true
Attachment #691213 - Flags: review?(matt.woodrow)
Attachment #691213 - Flags: review?(matt.woodrow) → review+
Landed parts 0.5 and 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/febf7b3ad731
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b11f6769b27
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/febf7b3ad731
https://hg.mozilla.org/mozilla-central/rev/4b11f6769b27
Comment on attachment 689442 [details] [diff] [review]
Part 1: When determining whether the scale factors have changed, we need to use GetBaseTransform for the old scale factors, because that's what stores the old transform that we computed scale factors

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Off-main-thread-animations
User impact if declined: Bad rendering of CrabJump app in B2G (bug 811157 needs part 1 and part 0.5 of this bug)
Testing completed (on m-c, etc.): Just landed on m-c
Risk to taking this patch (and alternatives if risky): No alternative. Both patches are pretty safe though.
String or UUID changes made by this patch: none
Attachment #689442 - Flags: review-
Attachment #689442 - Flags: review+
Attachment #689442 - Flags: approval-mozilla-b2g18?
Attachment #689442 - Flags: approval-mozilla-aurora?
BTW on beta desktop builds, the bug fixed by these patches might be a performance regression for FF18 so we might want to fix it on beta too. But I don't have a good example of that in the wild.
Attachment #689442 - Flags: review- → review+
Comment on attachment 689442 [details] [diff] [review]
Part 1: When determining whether the scale factors have changed, we need to use GetBaseTransform for the old scale factors, because that's what stores the old transform that we computed scale factors

[Triage Comment]
Approving for Aurora and B2G18 given the impact to a Marketplace app and how ugly this is on an Unagi. Also reproduces on desktop, but we're worried that we're so close to release that we may not get feedback on even worse regressions. On desktop this only really amounts to flickers. We can hopefully eat that for FF18.
Attachment #689442 - Flags: approval-mozilla-b2g18?
Attachment #689442 - Flags: approval-mozilla-b2g18+
Attachment #689442 - Flags: approval-mozilla-aurora?
Attachment #689442 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d1c82b347e8c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6f2f48519b62
https://hg.mozilla.org/releases/mozilla-aurora/rev/770ac28816ef
https://hg.mozilla.org/releases/mozilla-aurora/rev/9a03dbb0be38
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> https://tbpl.mozilla.org/?tree=Try&rev=ff813fd1b88f

These Android reftest failures are strange. They look like canvases (2D and WebGL) aren't being drawn at all. I downloaded the build and tested it on my phone and canvases do render fine in the browser, including the testcases that show up as failures in the logs. So somehow the patch (in part 3 presumably) causes canvases to not be rendered by reftests only.

I did find one bug ... in Layer::SnapTransform, we could fail to initialize *aResidualTransform if snappedMatrix is singular. That shouldn't happen, but who knows ... re-pushed: https://tbpl.mozilla.org/?tree=Try&rev=5b73ba0ce35b
Reftests didn't run for some reason. repushing: https://tbpl.mozilla.org/?tree=Try&rev=e4938cc3ba1a
Tests are still not running. Tried with slightly different options: https://tbpl.mozilla.org/?tree=Try&rev=2b9cf7a1348b
OK, the reftest still fails with those patches.

Fixed another uninitialized *aResidualTransform bug, and re-pushed with some logging of CanvasLayer::mEffectiveTransform: https://tbpl.mozilla.org/?tree=Try&rev=b321e02dc251
Another: https://tbpl.mozilla.org/?tree=Try&rev=91a34b7bfd55
Experimentally disabling stuff: https://tbpl.mozilla.org/?tree=Try&rev=d67f31f957b5
more: https://tbpl.mozilla.org/?tree=Try&rev=a809ac8aad3c
more:  https://tbpl.mozilla.org/?tree=Try&rev=9da0bc8d3827
I suspect we were getting into trouble dividing by zero when aSnapRect.width/height is 0, although I'm still not sure how that would lead to nothing being rendered when aSnapRect is not zero.
https://tbpl.mozilla.org/?tree=Try&rev=f9b785f42577
Yeah, so that seems to be the key.
https://tbpl.mozilla.org/?tree=Try&rev=5b817fbd3882
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b5ec3ba8d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/76eb5642d77b
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/59b5ec3ba8d3
https://hg.mozilla.org/mozilla-central/rev/76eb5642d77b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
status-b2g18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
Not fixed on 19 or 18. I don't think we should land these patches there, that's unnecessary risk.
status-b2g18: fixed → affected
status-firefox18: --- → affected
status-firefox19: fixed → affected
How about wontfix then?
status-b2g18: affected → wontfix
status-firefox18: affected → wontfix
status-firefox19: affected → wontfix
Verified fixed on FF 20b5 on Windows 7 and Mac OS 10.8
Status: RESOLVED → VERIFIED
status-firefox20: fixed → verified

Comment 55

2 years ago
Hi,

This issue seems to be ongoing.
On Windows 8, Firefox 40.0.2 the problem still occurs.

I have a <div> element that has a transition & transform applied to it. In order to prevent the bug from happening I still need to apply the 'rotation fix' by adding an arbitrary rotation to my transform i.e.

    transform : translate(0px, -100%) rotate(0.1deg)

This is then applied in a transition

    transition : transform 0.5s ease 0s;
New issues should go in a separate bug report, please.
You need to log in before you can comment on or make changes to this bug.