Bug 42370

Summary: Scaled Resized images are blurred when sent to Skia
Product: WebKit Reporter: Morley Abbott <mabbo>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: brettw, commit-queue, eric, jamesr, krit, senorblanco, wjmaclean, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 47433    
Bug Blocks:    
Attachments:
Description Flags
Example test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Attempt at creating test case for scrolling regression [Chromium]
none
Proposed Layout test to catch regression - works in Chrome and test_shell.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Morley Abbott 2010-07-15 07:06:05 PDT
1. Have an SVG load an image at a lower quality than the actual image is at. (example: image is actually 512x512, but we load it with width=128 height=128)
2. Have the svg <image> tag include a transform that scales up the image, lets say back to it's original size. (example image tag has transform="scale(4)")

Result: The image is resampled down, as Skia assumes we're drawing it at the small size specified, then when we draw it, we're scaling it back up. We get the poor quality version we see here. And it only does this when using Skia versions of Chrome (which is why OSX Chrome has no issue). 

The problem is in WebCore/platform/graphics/skia/ImageSkia.cpp. 

When we have computeResamplingMode return RESAMPLE_AWESOME, we're scaling down the image. Having these conditions return RESAMPLE_LINEAR instead will ensure that we don't do the specific resampling that causes this issue, when these conditions arise. 

Patch to follow shortly that will check for later scaling in the canvas, and return RESAMPLE_LINEAR in that case.
Comment 1 Morley Abbott 2010-07-15 07:07:30 PDT
Created attachment 61653 [details]
Example test case

Example test case. The svg loads two images: one at the correct size, the other at a smaller size, but later scaled up. The result should be two identical images, but instead, the second is blurred.
Comment 2 W. James MacLean 2010-09-23 12:53:18 PDT
Created attachment 68569 [details]
Patch
Comment 3 W. James MacLean 2010-09-23 12:56:24 PDT
(In reply to comment #2)
> Created an attachment (id=68569) [details]
> Patch

This patch fixes a scaling issue that occurs with resampled bitmaps in ImageSkia.cpp. Previously, the size of the resampled bitmap did not take into account the current transform matrix of the canvas being used, with the result that (sometimes) under-sized re-sampled images were created and then re-interpolated to larger sizes, with predictably poor results.
Comment 4 David Levin 2010-09-23 13:34:56 PDT
Comment on attachment 68569 [details]
Patch

I suspect that image doesn't correspond to WebKit's license terms.
Comment 5 W. James MacLean 2010-09-24 06:38:21 PDT
Created attachment 68670 [details]
Patch
Comment 6 W. James MacLean 2010-09-24 06:44:58 PDT
(In reply to comment #5)
> Created an attachment (id=68670) [details]
> Patch

Revised patch to change image. The new patch uses a JPEG input image to keep file size a bit smaller (since a moderate-sized image with diverse and non-regular texture is useful here) - this can be made PNG if required.
Comment 7 W. James MacLean 2010-09-24 10:39:44 PDT
Created attachment 68707 [details]
Patch
Comment 8 W. James MacLean 2010-09-24 10:41:44 PDT
(In reply to comment #7)
> Created an attachment (id=68707) [details]
> Patch

Revised patch to *not* use SkMatrix::rectStaysRect(), as it seems not to work quite as advertised. We limit high-quality resampling to case of scaling & translation, but no skew, rotation or perspective.
Comment 9 Stephen White 2010-09-24 12:00:13 PDT
Looks good (but I'm not a reviewer).
Comment 10 W. James MacLean 2010-09-28 12:11:15 PDT
Created attachment 69084 [details]
Patch
Comment 11 James Robinson 2010-09-28 12:14:44 PDT
Comment on attachment 69084 [details]
Patch

R=me
Comment 12 WebKit Commit Bot 2010-09-28 15:26:20 PDT
Comment on attachment 69084 [details]
Patch

Clearing flags on attachment: 69084

Committed r68574: <http://trac.webkit.org/changeset/68574>
Comment 13 WebKit Commit Bot 2010-09-28 15:26:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 James Robinson 2010-10-08 16:00:41 PDT
This broke pages, reverted at http://trac.webkit.org/changeset/69427.  I suspect the issue is some interaction with the delayed awesome resize optimization but am not entirely sure.
Comment 15 W. James MacLean 2010-10-13 11:33:45 PDT
Created attachment 70638 [details]
Patch
Comment 16 W. James MacLean 2010-10-13 11:36:11 PDT
(In reply to comment #15)
> Created an attachment (id=70638) [details]
> Patch

The problem was confusion about which coordinates to use where in the original patch. This revised patch fixes those, and solves the scrolling issue while fixing the blurriness issue.
Comment 17 James Robinson 2010-10-13 15:22:54 PDT
Comment on attachment 70638 [details]
Patch

What's the change from the last iteration?  Can you construct a regression test for the white bars issue?
Comment 18 W. James MacLean 2010-10-14 05:48:28 PDT
(In reply to comment #17)
> (From update of attachment 70638 [details])
> What's the change from the last iteration?  Can you construct a regression test for the white bars issue?

The new patch is somewhat simpler. In the previous I was transforming destBitmapSubsetSkI in addition to destRect, which was throwing off calculation of what needed to be painted w.r.t. the image, but only when scaled. I realised that all I really need to do is make sure the re-sampled bitmaps are done at the correct resolution, hence only the transformed version of destRect is required, and it passed only to Skia bitmap calls relating to resizing.

As for a regression test, the white bars only ever appear in scrolling, and only while uncovering an image a bit at a time. Any image the was drawn using the full-image-resize [lines 220-1 in the patched version] never had bars. I could never get the white bars to appear in test_shell (so I don't know how dump-render-tree will do).

I don't know offhand how to recreate this in a layout test.
Comment 19 W. James MacLean 2010-10-19 11:40:59 PDT
Created attachment 71190 [details]
Attempt at creating test case for scrolling regression [Chromium]

This test case (which re-uses the image from the original CL) can detect the regression behaviour in chrome, but not in DumpRenderTree or test_shell. It only seems to work if the timeouts are used, as otherwise the scroll events seem to get combined.

I'd be interested in knowing if anyone can suggest a way to make it work properly as a layout test for chromium-linux and chromium-win.

Thoughts?
Comment 20 W. James MacLean 2010-11-11 10:16:13 PST
Created attachment 73621 [details]
Proposed Layout test to catch regression - works in Chrome and test_shell.

This test case works in test_shell, but has the disadvantage of having vertical scroll bars. If this is acceptable, I will generate baseline output re-submit the patch with this test case added.
Comment 21 James Robinson 2010-11-11 10:49:30 PST
Thanks for persisting on a test!  Can you add overflow:hidden to the <body> to avoid the scrollbar?
Comment 22 W. James MacLean 2010-11-11 11:31:18 PST
(In reply to comment #21)
> Thanks for persisting on a test!  Can you add overflow:hidden to the <body> to avoid the scrollbar?

Cool - I didn't know about overflow:hidden, but it perfect for this!

I've revised the .html file to include this and will resubmit the patch by the end of the day.
Comment 23 W. James MacLean 2010-11-12 07:17:04 PST
Created attachment 73740 [details]
Patch
Comment 24 W. James MacLean 2010-11-12 07:20:31 PST
(In reply to comment #23)
> Created an attachment (id=73740) [details]
> Patch

This patch fixes the earlier regression, and includes a new (test_shell-friendly) layout test to catch the regression. Expected output may fail for Mac and Win, but we will re-baseline using the output from the build bots.

The regression layout test has been modified to force the layouTestController to wait until all repainting has completed, and comments cleaned up.
Comment 25 James Robinson 2010-11-15 11:41:45 PST
Comment on attachment 73740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73740&action=review

R=me, but test could be a bit better - please see comments.

> LayoutTests/svg/custom/image-rescale-scroll.html:21
> +    setTimeout("var occluder = document.getElementById('occluder'); occluder.style.top = 600;", 50);
> +    setTimeout("endTest()", 100); // Let repaint complete before ending test.

Instead of doing this, you can use layoutTestController.display() to force an immediate repaint.  This will make the test run a bit faster and be more reliable.  LayoutTests/fast/repaint/resources.js has some basic helper functions.  Take a look at http://trac.webkit.org/browser/trunk/LayoutTests/fast/repaint/reflection-redraw.html as one example.
Comment 26 W. James MacLean 2010-11-15 15:42:53 PST
Created attachment 73939 [details]
Patch
Comment 27 James Robinson 2010-11-15 15:49:26 PST
Comment on attachment 73939 [details]
Patch

Yay!  Thanks for persisting.
Comment 28 WebKit Commit Bot 2010-11-15 20:36:06 PST
Comment on attachment 73939 [details]
Patch

Rejecting patch 73939 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
do ...............................
editing/unsupported-content .........
fast/backgrounds ..........................
fast/backgrounds/repeat .......
fast/backgrounds/size ..........................
fast/block/basic ...............................
fast/block/float ...............
fast/block/float/015.html -> failed

Exiting early after 1 failures. 5611 tests run.
97.53s total testing time

5610 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6088003
Comment 29 W. James MacLean 2010-11-16 08:40:58 PST
Created attachment 73997 [details]
Patch
Comment 30 W. James MacLean 2010-11-16 08:42:35 PST
(In reply to comment #29)
> Created an attachment (id=73997) [details]
> Patch

This version of the patch also re-baselines several tests which *may* be the cause of the commit failure in Comment #28. These tests did not trigger on the original patch.
Comment 31 W. James MacLean 2010-11-16 10:40:59 PST
Created attachment 74009 [details]
Patch
Comment 32 W. James MacLean 2010-11-16 10:41:54 PST
Comment on attachment 74009 [details]
Patch

Regenerated the patch, as apparently there were merge issues.
Comment 33 James Robinson 2010-11-16 10:48:41 PST
Comment on attachment 74009 [details]
Patch

I don't understand the rebaselines.  None of the rebaselines are for the test that failed (fast/block/float/015.html) and it's not clear why these tests would need changed.  Are these baselines generated on the same config as the bots (meaning hardy, 32 bit)?

Can you reproduce the failure on fast/block/float/015.html?  Could it be a flake?
Comment 34 W. James MacLean 2010-11-16 11:18:34 PST
(In reply to comment #33)
> (From update of attachment 74009 [details])
> I don't understand the rebaselines.  None of the rebaselines are for the test that failed (fast/block/float/015.html) and it's not clear why these tests would need changed.  Are these baselines generated on the same config as the bots (meaning hardy, 32 bit)?
> 
> Can you reproduce the failure on fast/block/float/015.html?  Could it be a flake?

Sorry, I should have talked about this.

I was unable to get the test mentioned above to fail, and yes I think it's flaky. It's been disabled since this patch was committed for intermittent failures.

I retested via run_webkit_tests and came up with non-flaky failures (and specific to my patch) on the tests I re-baselined. The output linked above indicated there were other failures, but I could not identify what they were.

We could try going back and re-submitting the original patch.
Comment 35 James Robinson 2010-11-16 11:22:10 PST
(In reply to comment #34)
> (In reply to comment #33)
> > (From update of attachment 74009 [details] [details])
> > I don't understand the rebaselines.  None of the rebaselines are for the test that failed (fast/block/float/015.html) and it's not clear why these tests would need changed.  Are these baselines generated on the same config as the bots (meaning hardy, 32 bit)?
> > 
> > Can you reproduce the failure on fast/block/float/015.html?  Could it be a flake?
> 
> Sorry, I should have talked about this.
> 
> I was unable to get the test mentioned above to fail, and yes I think it's flaky. It's been disabled since this patch was committed for intermittent failures.
> 
> I retested via run_webkit_tests and came up with non-flaky failures (and specific to my patch) on the tests I re-baselined. The output linked above indicated there were other failures, but I could not identify what they were.

OK - are these new baselines from 32 bit hardy?  I ask because 64 bit binaries and Lucid will both produce slightly different results for several SVG tests and we don't want to break the bots.

> 
> We could try going back and re-submitting the original patch.
Comment 36 W. James MacLean 2010-11-16 11:26:22 PST
(In reply to comment #35)
> 
> OK - are these new baselines from 32 bit hardy?  I ask because 64 bit binaries and Lucid will both produce slightly different results for several SVG tests and we don't want to break the bots.

Hmmm, no ... it's a 64-bit build. Maybe it's safer then to re-submit the original patch and see if it goes through.
Comment 37 James Robinson 2010-11-16 11:27:26 PST
Comment on attachment 74009 [details]
Patch

Clearing flags
Comment 38 James Robinson 2010-11-16 11:27:41 PST
Comment on attachment 73939 [details]
Patch

Take two!
Comment 39 WebKit Commit Bot 2010-11-17 02:39:37 PST
Comment on attachment 73939 [details]
Patch

Rejecting patch 73939 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ests/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ...........................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
528.79s total testing time

21970 test cases (99%) succeeded
1 test case (<1%) was new
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6149010
Comment 40 W. James MacLean 2010-11-17 10:11:19 PST

(In reply to comment #39)
> (From update of attachment 73939 [details])
> Rejecting patch 73939 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
> Last 500 characters of output:
> ests/websocket/tests/workers ......
> http/tests/workers .....
> http/tests/xhtmlmp .
> http/tests/xmlhttprequest ...........................................................................................................................................................................
> http/tests/xmlhttprequest/web-apps ...............
> http/tests/xmlhttprequest/workers .........
> 528.79s total testing time
> 
> 21970 test cases (99%) succeeded
> 1 test case (<1%) was new
> 13 test cases (<1%) had stderr output
> 
> Full output: http://queues.webkit.org/results/6149010

I looked over this, and cannot see what tests are failing. It appears something is going wrong in the java tests, but my code shouldn't affect that.

Is this commit queue flakiness, or am I missing something in the output?
Comment 41 Eric Seidel (no email) 2010-11-17 10:42:11 PST
You're missing a result.  A new test is a failing test.
Comment 42 W. James MacLean 2010-11-18 10:20:18 PST
Created attachment 74250 [details]
Patch
Comment 43 W. James MacLean 2010-11-18 10:22:40 PST
Comment on attachment 74250 [details]
Patch

I suspect this failed previously due to lack of test expected output for platform/mac. Have marked this test as FAIL in platform/mac/test_expectations.txt so we can get a baseline off the build bot (I don't currently have any way to build off a Mac to get the expected output).
Comment 44 W. James MacLean 2010-11-19 08:26:16 PST
Created attachment 74394 [details]
Patch
Comment 45 W. James MacLean 2010-11-19 08:29:40 PST
Comment on attachment 74394 [details]
Patch

Finally managed to un-break my Mac repo and have added platform/mac baselines (still marked as fail though, just in case - will update this once they get through the commit queue).
Comment 46 James Robinson 2010-11-19 12:19:02 PST
Comment on attachment 74394 [details]
Patch

The edit to mac/test_expectations.txt is completely useless, but probably harmless as well.
Comment 47 WebKit Commit Bot 2010-11-19 23:56:36 PST
Comment on attachment 74394 [details]
Patch

Clearing flags on attachment: 74394

Committed r72470: <http://trac.webkit.org/changeset/72470>
Comment 48 WebKit Commit Bot 2010-11-19 23:56:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 49 James Robinson 2010-11-29 19:13:30 PST
Reverted: http://trac.webkit.org/changeset/72864
Comment 50 W. James MacLean 2010-12-01 08:53:20 PST
Created attachment 75276 [details]
Patch
Comment 51 W. James MacLean 2010-12-01 08:55:27 PST
Comment on attachment 75276 [details]
Patch

I have re-baselined image-rescale.svg since the gamma appears to have changed (the old baseline was moved to mac/leopard, as that's where it was originally done).

Just in case the mac tests need re-baselining on Leopard, is there a way to indicate this?
Comment 52 Stephen White 2010-12-01 08:58:10 PST
Comment on attachment 75276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75276&action=review

> WebCore/platform/graphics/skia/ImageSkia.cpp:253
> +        }

This brace doesn't look right.  Is this a formatting error, or a code error?
Comment 53 James Robinson 2010-12-01 11:07:48 PST
What's the difference between this and the reverted patch?  I guess lines 235-238?  Is image-rescale-clip the regression test?
Comment 54 W. James MacLean 2010-12-01 11:18:34 PST
(In reply to comment #53)
> What's the difference between this and the reverted patch?  I guess lines 235-238?  Is image-rescale-clip the regression test?

The differences are:

203-4 (added)
206-8 (added)
235-8 (added)
243 (changed)

Basically the clipped dest subrect wasn't being calculated correctly, I believe it is now.

Yes, image-rescale-clip.html is the regression test: it creates a clipped dest subrect which is not just the entire destRect.
Comment 55 W. James MacLean 2010-12-01 11:21:31 PST
(In reply to comment #52)
> (From update of attachment 75276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75276&action=review
> 
> > WebCore/platform/graphics/skia/ImageSkia.cpp:253
> > +        }
> 
> This brace doesn't look right.  Is this a formatting error, or a code error?

Sorry, lines 240-252 should be indented 4 more spaces ... I'll fix this.
Comment 56 W. James MacLean 2010-12-01 11:26:13 PST
Created attachment 75295 [details]
Patch
Comment 57 James Robinson 2010-12-02 10:59:01 PST
Comment on attachment 75295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75295&action=review

R=me.  Third time's the charm?

> WebCore/platform/graphics/skia/ImageSkia.cpp:200
> +    // We also need to compute the transformed portion of the
> +    // visible portion for use below.

Note for the future: this is over-wrapped.  There's no 80 column limit in WebKit so this should just be one line.
Comment 58 W. James MacLean 2010-12-02 11:00:50 PST
(In reply to comment #57)
> (From update of attachment 75295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75295&action=review
> 
> R=me.  Third time's the charm?

Fingers crossed :-)

> > WebCore/platform/graphics/skia/ImageSkia.cpp:200
> > +    // We also need to compute the transformed portion of the
> > +    // visible portion for use below.
> 
> Note for the future: this is over-wrapped.  There's no 80 column limit in WebKit so this should just be one line.

Thanks, will do!
Comment 59 WebKit Commit Bot 2010-12-02 11:24:15 PST
Comment on attachment 75295 [details]
Patch

Clearing flags on attachment: 75295

Committed r73169: <http://trac.webkit.org/changeset/73169>
Comment 60 WebKit Commit Bot 2010-12-02 11:24:34 PST
All reviewed patches have been landed.  Closing bug.