Bug 50775 - Jittering when animating a rotated image
Summary: Jittering when animating a rotated image
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Matthew Delaney
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-09 12:11 PST by Daniel Rice
Modified: 2011-09-01 17:18 PDT (History)
10 users (show)

See Also:


Attachments
Reproducible test case (2.39 KB, text/html)
2010-12-09 12:11 PST, Daniel Rice
no flags Details
Patch (3.08 KB, patch)
2011-02-21 11:07 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2011-04-05 07:12 PDT, Stephen White
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Rice 2010-12-09 12:11:07 PST
Created attachment 76102 [details]
Reproducible test case

The attached html/js file displays an animation of a rotating checkerboard image, along with a magnified view of each frame as it is drawn.  When the image is at a rotation angle near 45 degrees, it appears to shake.  Playing the same animation on Firefox gives a smooth appearance, where the antialiasing around the internal edges changes in a steady manner from frame to frame.
Comment 1 Matthew Delaney 2010-12-13 15:22:18 PST
Hi Daniel, I'm not seeing any shaking on my current build on my machine. Though, I'm not quite sure what you mean by shaking - could you maybe elaborate a little more. As a baseline, firefox 4 beta does the same as my Safari build.

Are the details you listed in the bug all correct? (i.e. Are you on Leopard, running a nightly?) If so, which nightly? Any other relevant info for me to try to reproduce would be great.

Also, if you could try the latest webkit nightly on a Leopard or Snow Leopard install and let me know if you still see the jittering that'd be great. Thanks!
Comment 2 Dmitry Chestnykh 2011-02-14 16:38:42 PST
Matthew, (In reply to comment #1)

Here's the similar bug I filed in Chromium bug tracker: http://code.google.com/p/chromium/issues/detail?id=67555

You can reproduce it by running WebKit's 32-bit build.
Comment 3 Dmitry Chestnykh 2011-02-14 17:17:16 PST
This commit might be related: http://trac.webkit.org/changeset/15470
Comment 4 Stephen White 2011-02-21 08:37:48 PST
(In reply to comment #3)
> This commit might be related: http://trac.webkit.org/changeset/15470

Looks like this all started back in http://trac.webkit.org/changeset/14739, to fix "Pixel cracks in weather widget at 1.83 scaling" (in Radar, unfortunately, so I have no way to repro this).  I have noticed similar problems with CoreGraphics image scaling before, but I don't think these apply to Skia, but I could be wrong.

I'm tempted to
Comment 5 Stephen White 2011-02-21 08:40:06 PST
I'm tempted to say let's just rip it out on the Skia path (which is what Android have done, with no apparent ill effects).  But obviously that won't help Chrome/Mac, nor Safari.
Comment 6 Dmitry Chestnykh 2011-02-21 08:59:21 PST
Moreover, change 14739 seem to no longer fix what it supposed to fix, at least on 10.6.6. The weather widget is broken at scale factor 1.5: http://i.imgur.com/IiT7S.png
Comment 7 Stephen White 2011-02-21 11:07:39 PST
Created attachment 83193 [details]
Patch
Comment 8 Stephen White 2011-02-22 08:25:40 PST
Some more data: the test case in https://bugs.webkit.org/show_bug.cgi?id=15720 (non-canvas draws, which do not yet have this coordinate adjustment) shows cracks in Chrome/Mac and Safari, but not in Chrome/Win.  This confirms that this is a CoreGraphics-specific issue, and doesn't apply to Skia.  So I think it is safe to land the attached patch.

For the CoreGraphics path, how about this for a compromise:  If the matrix contains a rotation, do not snap to integral pixel boundaries (ie., return the original coordinates).  Then there would be no cracks in the common case (scales, translations) such as that in the OSX weather widget, and rotations wouldn't show jitter, although they would show cracks if multiple parallel images were rotated together.  The latter seems like a less common case compared to the others.
Comment 9 Dmitry Chestnykh 2011-02-22 08:34:36 PST
Stephen, but the compromise won't fix the zooming jitter (as this test case: http://code.google.com/p/chromium/issues/detail?id=67555#c18), will it?
Comment 10 Stephen White 2011-02-22 08:51:03 PST
(In reply to comment #9)
> Stephen, but the compromise won't fix the zooming jitter (as this test case: http://code.google.com/p/chromium/issues/detail?id=67555#c18), will it?

True.  Zooming would still jitter.
Comment 11 Kenneth Russell 2011-02-22 11:40:42 PST
Comment on attachment 83193 [details]
Patch

Looks fine to me. If this is the solution desired, we could always #ifdef the similar code in GraphicsContextCG.cpp with !PLATFORM(CHROMIUM).
Comment 12 Stephen White 2011-02-23 04:16:32 PST
Landed as http://trac.webkit.org/changeset/79437.  Leaving the bug open while we decide what to do on other platforms.
Comment 13 Vangelis Kokkevis 2011-02-25 18:07:46 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Stephen, but the compromise won't fix the zooming jitter (as this test case: http://code.google.com/p/chromium/issues/detail?id=67555#c18), will it?
> 
> True.  Zooming would still jitter.

I'm confused.. why would zooming still jitter if you're only only apply the fix when the matrix has a rotation?  Do you mean if it has both rotation and scale? 

I actually applied a patch locally to eliminate the code in roundToDevicePixels and the zooming example from the crbug seems nice and smooth.
Comment 14 Dmitry Chestnykh 2011-02-26 03:19:32 PST
(In reply to comment #13)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Stephen, but the compromise won't fix the zooming jitter (as this test case: http://code.google.com/p/chromium/issues/detail?id=67555#c18), will it?
> > 
> > True.  Zooming would still jitter.
> 
> I'm confused.. why would zooming still jitter if you're only only apply the fix when the matrix has a rotation?  Do you mean if it has both rotation and scale? 

The proposed fix for CG was to _skip_ roundToDevicePixels when the matrix has rotation. But in case of zooming this will not work (as roundToDevicePixels still applies in this case).
 
> I actually applied a patch locally to eliminate the code in roundToDevicePixels and the zooming example from the crbug seems nice and smooth.

That's because you eliminated roundToDevicePixels, and that's the goal. However, there seem to be the case that eliminating roundToDevicePixels completely in CoreGraphics version introduces pixel cracks.
Comment 15 Vangelis Kokkevis 2011-03-07 18:39:19 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > Stephen, but the compromise won't fix the zooming jitter (as this test case: http://code.google.com/p/chromium/issues/detail?id=67555#c18), will it?
> > > 
> > > True.  Zooming would still jitter.
> > 
> > I'm confused.. why would zooming still jitter if you're only only apply the fix when the matrix has a rotation?  Do you mean if it has both rotation and scale? 
> 
> The proposed fix for CG was to _skip_ roundToDevicePixels when the matrix has rotation. But in case of zooming this will not work (as roundToDevicePixels still applies in this case).
> 
> > I actually applied a patch locally to eliminate the code in roundToDevicePixels and the zooming example from the crbug seems nice and smooth.
> 
> That's because you eliminated roundToDevicePixels, and that's the goal. However, there seem to be the case that eliminating roundToDevicePixels completely in CoreGraphics version introduces pixel cracks.

Oh, I see.

Is there any way to test that cracks are still an issue for the CG path? The fix/workaround: http://trac.webkit.org/changeset/14739   was checked in 5 years ago.  Anybody have access to the original rdar repro case?
Comment 16 Stephen White 2011-04-05 07:12:52 PDT
Created attachment 88224 [details]
Patch
Comment 17 Kenneth Russell 2011-04-06 17:16:19 PDT
Comment on attachment 88224 [details]
Patch

Looks good to me as long as it's been tested.
Comment 18 Stephen White 2011-04-11 13:57:56 PDT
Landed the second patch as r83490.  Leaving the bug open in case other ports (e.g., Safari) want to do something about this.