Bug 42370

Summary:

Scaled Resized images are blurred when sent to Skia

Product:

WebKit

Reporter:

Morley Abbott <mabbo>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

brettw, commit-queue, eric, jamesr, krit, senorblanco, wjmaclean, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

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

Morley Abbott

Reported 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.

Attachments
Example test case (485 bytes, image/svg+xml) 2010-07-15 07:07 PDT, Morley Abbott	no flags	Details
Patch (143.11 KB, patch) 2010-09-23 12:53 PDT, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (327.21 KB, patch) 2010-09-24 06:38 PDT, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (327.24 KB, patch) 2010-09-24 10:39 PDT, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (323.54 KB, patch) 2010-09-28 12:11 PDT, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (323.33 KB, patch) 2010-10-13 11:33 PDT, W. James MacLean	no flags	Details Formatted Diff Diff
Attempt at creating test case for scrolling regression [Chromium] (1.39 KB, text/html) 2010-10-19 11:40 PDT, W. James MacLean	no flags	Details
Proposed Layout test to catch regression - works in Chrome and test_shell. (1.58 KB, text/html) 2010-11-11 10:16 PST, W. James MacLean	no flags	Details
Patch (345.28 KB, patch) 2010-11-12 07:17 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (345.19 KB, patch) 2010-11-15 15:42 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (1.03 MB, patch) 2010-11-16 08:40 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (1.03 MB, patch) 2010-11-16 10:40 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (345.93 KB, patch) 2010-11-18 10:20 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (375.23 KB, patch) 2010-11-19 08:26 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (574.76 KB, patch) 2010-12-01 08:53 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Patch (575.40 KB, patch) 2010-12-01 11:26 PST, W. James MacLean	no flags	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Morley Abbott

Comment 1 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.

W. James MacLean

Comment 2 2010-09-23 12:53:18 PDT

Created attachment 68569 [details] Patch

W. James MacLean

Comment 3 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.

David Levin

Comment 4 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.

W. James MacLean

Comment 5 2010-09-24 06:38:21 PDT

Created attachment 68670 [details] Patch

W. James MacLean

Comment 6 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.

W. James MacLean

Comment 7 2010-09-24 10:39:44 PDT

Created attachment 68707 [details] Patch

W. James MacLean

Comment 8 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.

Stephen White

Comment 9 2010-09-24 12:00:13 PDT

Looks good (but I'm not a reviewer).

W. James MacLean

Comment 10 2010-09-28 12:11:15 PDT

Created attachment 69084 [details] Patch

James Robinson

Comment 11 2010-09-28 12:14:44 PDT

Comment on attachment 69084 [details] Patch R=me

WebKit Commit Bot

Comment 12 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>

WebKit Commit Bot

Comment 13 2010-09-28 15:26:27 PDT

All reviewed patches have been landed. Closing bug.

James Robinson

Comment 14 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.

W. James MacLean

Comment 15 2010-10-13 11:33:45 PDT

Created attachment 70638 [details] Patch

W. James MacLean

Comment 16 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.

James Robinson

Comment 17 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?

W. James MacLean

Comment 18 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.

W. James MacLean

Comment 19 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?

W. James MacLean

Comment 20 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.

James Robinson

Comment 21 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?

W. James MacLean

Comment 22 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.

W. James MacLean

Comment 23 2010-11-12 07:17:04 PST

Created attachment 73740 [details] Patch

W. James MacLean

Comment 24 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.

James Robinson

Comment 25 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.

W. James MacLean

Comment 26 2010-11-15 15:42:53 PST

Created attachment 73939 [details] Patch

James Robinson

Comment 27 2010-11-15 15:49:26 PST

Comment on attachment 73939 [details] Patch Yay! Thanks for persisting.

WebKit Commit Bot

Comment 28 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

W. James MacLean

Comment 29 2010-11-16 08:40:58 PST

Created attachment 73997 [details] Patch

W. James MacLean

Comment 30 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.

W. James MacLean

Comment 31 2010-11-16 10:40:59 PST

Created attachment 74009 [details] Patch

W. James MacLean

Comment 32 2010-11-16 10:41:54 PST

Comment on attachment 74009 [details] Patch Regenerated the patch, as apparently there were merge issues.

James Robinson

Comment 33 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?

W. James MacLean

Comment 34 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.

James Robinson

Comment 35 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.

W. James MacLean

Comment 36 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.

James Robinson

Comment 37 2010-11-16 11:27:26 PST

Comment on attachment 74009 [details] Patch Clearing flags

James Robinson

Comment 38 2010-11-16 11:27:41 PST

Comment on attachment 73939 [details] Patch Take two!

WebKit Commit Bot

Comment 39 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

W. James MacLean

Comment 40 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?

Eric Seidel (no email)

Comment 41 2010-11-17 10:42:11 PST

You're missing a result. A new test is a failing test.

W. James MacLean

Comment 42 2010-11-18 10:20:18 PST

Created attachment 74250 [details] Patch

W. James MacLean

Comment 43 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).

W. James MacLean

Comment 44 2010-11-19 08:26:16 PST

Created attachment 74394 [details] Patch

W. James MacLean

Comment 45 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).

James Robinson

Comment 46 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.

WebKit Commit Bot

Comment 47 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>

WebKit Commit Bot

Comment 48 2010-11-19 23:56:51 PST

All reviewed patches have been landed. Closing bug.

James Robinson

Comment 49 2010-11-29 19:13:30 PST

Reverted: http://trac.webkit.org/changeset/72864

W. James MacLean

Comment 50 2010-12-01 08:53:20 PST

Created attachment 75276 [details] Patch

W. James MacLean

Comment 51 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?

Stephen White

Comment 52 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?

James Robinson

Comment 53 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?

W. James MacLean

Comment 54 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.

W. James MacLean

Comment 55 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.

W. James MacLean

Comment 56 2010-12-01 11:26:13 PST

Created attachment 75295 [details] Patch

James Robinson

Comment 57 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.

W. James MacLean

Comment 58 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!

WebKit Commit Bot

Comment 59 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>

WebKit Commit Bot

Comment 60 2010-12-02 11:24:34 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.