Bug 42370 – Scaled Resized images are blurred when sent to Skia

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Morley Abbott]

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.

[Morley Abbott]

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]

Created attachment 68569 [details]
Patch

[W. James MacLean]

(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 on attachment 68569 [details]
Patch

I suspect that image doesn't correspond to WebKit's license terms.

[W. James MacLean]

Created attachment 68670 [details]
Patch

[W. James MacLean]

(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]

Created attachment 68707 [details]
Patch

[W. James MacLean]

(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]

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

[W. James MacLean]

Created attachment 69084 [details]
Patch

[James Robinson]

Comment on attachment 69084 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 69084 [details]
Patch

Clearing flags on attachment: 69084

Committed r68574: <http://trac.webkit.org/changeset/68574>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

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]

Created attachment 70638 [details]
Patch

[W. James MacLean]

(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 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]

(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]

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]

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]

Thanks for persisting on a test!  Can you add overflow:hidden to the <body> to avoid the scrollbar?

[W. James MacLean]

(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]

Created attachment 73740 [details]
Patch

[W. James MacLean]

(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 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]

Created attachment 73939 [details]
Patch

[James Robinson]

Comment on attachment 73939 [details]
Patch

Yay!  Thanks for persisting.

[WebKit Commit Bot]

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]

Created attachment 73997 [details]
Patch

[W. James MacLean]

(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]

Created attachment 74009 [details]
Patch

[W. James MacLean]

Comment on attachment 74009 [details]
Patch

Regenerated the patch, as apparently there were merge issues.

[James Robinson]

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]

(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]

(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]

(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 on attachment 74009 [details]
Patch

Clearing flags

[James Robinson]

Comment on attachment 73939 [details]
Patch

Take two!

[WebKit Commit Bot]

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]

(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)]

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

[W. James MacLean]

Created attachment 74250 [details]
Patch

[W. James MacLean]

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]

Created attachment 74394 [details]
Patch

[W. James MacLean]

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 on attachment 74394 [details]
Patch

The edit to mac/test_expectations.txt is completely useless, but probably harmless as well.

[WebKit Commit Bot]

Comment on attachment 74394 [details]
Patch

Clearing flags on attachment: 74394

Committed r72470: <http://trac.webkit.org/changeset/72470>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

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

[W. James MacLean]

Created attachment 75276 [details]
Patch

[W. James MacLean]

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 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]

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]

(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]

(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]

Created attachment 75295 [details]
Patch

[James Robinson]

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]

(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 on attachment 75295 [details]
Patch

Clearing flags on attachment: 75295

Committed r73169: <http://trac.webkit.org/changeset/73169>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.