Closed Bug 663776 Opened 12 years ago Closed 10 years ago

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


(Core :: Graphics, defect)

Not set



Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- verified
b2g18 --- wontfix


(Reporter: rob, Assigned: roc)




(5 files, 1 obsolete file)

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): 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
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
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:
That's really unusual.

Do we know this is definitely a graphics bug?
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.
Not sure if this is related, but for people testing the demo at, they get max one frame per second
Blocks: 770810
(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.
I just spotted this bug in Gaia. Adding a "rotate(0.01deg)" makes the animations much smoother.
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+
Sigh ... that assertion did not, of course, show up in my try run.
The Android test failures appear to be due to canvas not rendering at all on Android...
I wonder whether it's a compiler bug.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Just part 1:

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:

Compilation failed. Trying again:
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.
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.
> +     */

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

Attached patch Part 0.5 v2Splinter Review
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+
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.
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+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)

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:
Tests are still not running. Tried with slightly different options:
OK, the reftest still fails with those patches.

Fixed another uninitialized *aResidualTransform bug, and re-pushed with some logging of CanvasLayer::mEffectiveTransform:
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.
Whiteboard: [leave open]
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Not fixed on 19 or 18. I don't think we should land these patches there, that's unnecessary risk.
Verified fixed on FF 20b5 on Windows 7 and Mac OS 10.8

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.