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.
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.
Created attachment 68569 [details] Patch
(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 on attachment 68569 [details] Patch I suspect that image doesn't correspond to WebKit's license terms.
Created attachment 68670 [details] Patch
(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.
Created attachment 68707 [details] Patch
(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.
Looks good (but I'm not a reviewer).
Created attachment 69084 [details] Patch
Comment on attachment 69084 [details] Patch R=me
Comment on attachment 69084 [details] Patch Clearing flags on attachment: 69084 Committed r68574: <http://trac.webkit.org/changeset/68574>
All reviewed patches have been landed. Closing bug.
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.
Created attachment 70638 [details] Patch
(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 on attachment 70638 [details] Patch What's the change from the last iteration? Can you construct a regression test for the white bars issue?
(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.
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?
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.
Thanks for persisting on a test! Can you add overflow:hidden to the <body> to avoid the scrollbar?
(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.
Created attachment 73740 [details] Patch
(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 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.
Created attachment 73939 [details] Patch
Comment on attachment 73939 [details] Patch Yay! Thanks for persisting.
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
Created attachment 73997 [details] Patch
(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.
Created attachment 74009 [details] Patch
Comment on attachment 74009 [details] Patch Regenerated the patch, as apparently there were merge issues.
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?
(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.
(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.
(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 on attachment 74009 [details] Patch Clearing flags
Comment on attachment 73939 [details] Patch Take two!
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
(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?
You're missing a result. A new test is a failing test.
Created attachment 74250 [details] Patch
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).
Created attachment 74394 [details] Patch
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 on attachment 74394 [details] Patch The edit to mac/test_expectations.txt is completely useless, but probably harmless as well.
Comment on attachment 74394 [details] Patch Clearing flags on attachment: 74394 Committed r72470: <http://trac.webkit.org/changeset/72470>
Reverted: http://trac.webkit.org/changeset/72864
Created attachment 75276 [details] Patch
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 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?
What's the difference between this and the reverted patch? I guess lines 235-238? Is image-rescale-clip the regression test?
(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.
(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.
Created attachment 75295 [details] Patch
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.
(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 on attachment 75295 [details] Patch Clearing flags on attachment: 75295 Committed r73169: <http://trac.webkit.org/changeset/73169>