Closed Bug 1088498 Opened 7 years ago Closed 7 years ago

Jittery images on www.neontv.co.nz

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: roc, Assigned: roc)

References

()

Details

Attachments

(2 files)

Attached file log.txt
Matthew noticed this and made a reduced testcase: http://flim.org/~kinetik/neontv/test.html

Hovering over the image with the mouse makes it scale, and the scaling is noticeably jittery.

I debugged this a bit. On my machine, at least, we're not layerizing the transform, which is unfortunate and probably deserves its own bug. The transforms look correct, not jittery. I think the problem is in the layout code that controls image drawing.

I set a breakpoint here and logged 'params' over a series of those animations:
  aImage->Draw(ctx, params.size, params.region, imgIContainer::FRAME_CURRENT,
               aGraphicsFilter, ToMaybe(aSVGContext), aImageFlags);
The results are attached.

Table of the _11, _22, and _31 values of imageSpaceToDeviceSpace as the image grows in size:
1		1		-59
1.0103744268	1.0103744268	-61.3653693199
1.0147877932	1.0147877932	-62.3716168404
1.0194369555	1.0194369555	-64.431625843
1.0242179632	1.0242179632	-65.5216956139
1.0270940065	1.0270940065	-65.1774334908
1.0286402702	1.0286402702	-66.5299816132
1.0301533937 	1.0301533937	-66.874973774
1.0312439203	1.0312439203	-66.1236138344
1.0320833921	1.0320833921	-66.3150134087
1.0326309204	1.0326309204	-67.4398498535
1.0329128504	1.0329128504	-67.5041298866
1.0329998732	1.0329998732	-66.5239710808
Notice that the _31 value --- which corresponds to the x-coordinate of the left edge of the image --- does not decrease uniformly. It increases from -65.5216956139 to -65.1774334908 and it also increases from -66.5299816132 to -66.1236138344. That seems wrong! I'm pretty sure it's the cause of the jitter. I assume it's the way image snapping works.
Note that the CSS has
  background: url("previewTile_r1_c1.jpg") no-repeat scroll center 0px;
so the anchor point is at the center of the image ... or would be if the image width was even, which it isn't.
Er, the image width *is* even, of course.
The block container is 315px wide. The centered anchor point is then at 157.5px. In ComputeSnappedImageDrawingParameters, we snap the anchor point after transforming it to device pixels by the current transform; since the transform origin is the center of the element, the anchor point in device pixels should remain stationary, but due to numerical error the value jiggles around slightly less or slightly more than 157.5px, and therefore can round in different directions at different points during the animation.

It's not immediately clear to me how to handle this.
In my case, it's not being layerized because I'm using BasicLayers which is forcing layers to be inactive to preserve subpixel antialiasing.
Seth, what do you think of this? I'm not thrilled with it but I can't think of anything better right now.
Attachment #8510986 - Flags: review?(seth)
Probably orthogonal to this bug, but I noticed that BasePoint::Round [1] does floor(x + 0.5). This will incorrectly round the greatest number less than 0.5 up to 1, and will also give an incorrect result for some very large numbers (e.g. for double, (2^53)-1 should round to itself). SM ran into the former problem fairly recently in bug 1000606 - their fixed implementation is at [2].

[1] http://hg.mozilla.org/mozilla-central/annotate/9781037ac408/gfx/2d/BasePoint.h#l78
[2] http://hg.mozilla.org/mozilla-central/annotate/9781037ac408/js/src/jsmath.cpp#l836
I don't think that matters here.
Comment on attachment 8510986 [details] [diff] [review]
Treat anchor offsets just less than 0.5 as 0.5 when rounding

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

I don't like this either, but I can't see a better solution.

With rounding there's always going to be a jump discontinuity that will negatively affect some content. FWIW I think there is a somewhat principled view of this change: we're adding a slight bias to move that jump discontinuity away from numbers that have a very high probability of being used by content, and towards numbers that are less likely.
Attachment #8510986 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/d67de20b8612
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.