Bug 90624 - ImageSkia should take context scale into account when resizing for drawPattern
Summary: ImageSkia should take context scale into account when resizing for drawPattern
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-05 11:25 PDT by vollick
Modified: 2012-07-27 01:16 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.13 KB, patch)
2012-07-05 12:05 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2012-07-19 14:45 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Added a comment to explain the 1/ctm.scale business (10.22 KB, patch)
2012-07-20 14:00 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (10.14 KB, patch)
2012-07-20 14:32 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch for landing (10.06 KB, patch)
2012-07-20 16:47 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch for landing (10.23 KB, patch)
2012-07-20 18:29 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (425.31 KB, patch)
2012-07-22 08:59 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 vollick 2012-07-05 11:25:21 PDT
When the resampling mode is RESAMPLE_AWESOME drawPattern creates a bitmap shader from a resized bitmap. The dimensions of the resized bitmap are the destination rect. Unfortunately, if the contentsScale (and hence the canvas scale) are not 1, the resulting image will occupy a different number of pixels than the destination rect. For example, if the canvas is scaled by two, the resized bitmap will be half as large as required and the result will be pixel doubled.
Comment 1 vollick 2012-07-05 12:05:15 PDT
Created attachment 150970 [details]
Patch
Comment 2 Adrienne Walker 2012-07-18 14:49:02 PDT
Comment on attachment 150970 [details]
Patch

Which tests? Why are they skipped?
Comment 3 Stephen White 2012-07-19 07:41:29 PDT
Comment on attachment 150970 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:383
> +        SkBitmap resampled = bitmap->resizedBitmap(srcRect, width * xScale, height * xScale);

Shouldn't this be height * yScale?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:389
> +        matrix.setScaleY(SkFloatToScalar(1 / yScale));

This doesn't look correct.  It looks like we're going to be doing a Lanczos resampling into the bitmap, followed by a bilinear resize in the pattern draw to handle the scale you've added.  That said, that looks like it can already happen if the pattern transform contains a rotation, since the pattern transform isn't taken into account in computeResamplingMode.

I think I have a notion of how to clean this whole mess up; let me try to put a patch together and let's see if it fixes your bug.
Comment 4 Stephen White 2012-07-19 14:45:37 PDT
Created attachment 153351 [details]
Patch
Comment 5 Stephen White 2012-07-19 14:55:00 PDT
Comment on attachment 150970 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:389
>> +        matrix.setScaleY(SkFloatToScalar(1 / yScale));
> 
> This doesn't look correct.  It looks like we're going to be doing a Lanczos resampling into the bitmap, followed by a bilinear resize in the pattern draw to handle the scale you've added.  That said, that looks like it can already happen if the pattern transform contains a rotation, since the pattern transform isn't taken into account in computeResamplingMode.
> 
> I think I have a notion of how to clean this whole mess up; let me try to put a patch together and let's see if it fixes your bug.

I take it back -- putting 1 / ctm.scale() in the patternTransform is exactly the right thing to do, so that when the pattern shader concatenates ctm and patternTransform internally, it gets identity scale.

I've attached a patch that incorporates this change as well as passing the concatenated matrices to computeResamplingMode and TransformDimensions(), so that the CTM is taken into account when determining the resampling algorithm as well.  Let me know if this patch fixes your bug.
Comment 6 Adrienne Walker 2012-07-20 13:06:21 PDT
Comment on attachment 153351 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:348
> +    SkMatrix ctm = context->platformContext()->canvas()->getTotalMatrix();
> +    SkMatrix totalMatrix;
> +    totalMatrix.setConcat(ctm, patternTransform);

style nit: ctm => context total matrix? Can you use a non-abbreviated variable name?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:379
> -        matrix.setScaleX(SkIntToScalar(1));
> -        matrix.setScaleY(SkIntToScalar(1));
> +        matrix.setScaleX(ctm.getScaleX() ? 1 / ctm.getScaleX() : 1);
> +        matrix.setScaleY(ctm.getScaleY() ? 1 / ctm.getScaleY() : 1);

I'm having a hard time convincing myself this is right.  Can the pattern have a scale?

Can you leave a comment about how you got to this scale value or alternatively use something more intuitive like width / destBitmapWidth?
Comment 7 Stephen White 2012-07-20 13:30:27 PDT
Comment on attachment 153351 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:348
>> +    totalMatrix.setConcat(ctm, patternTransform);
> 
> style nit: ctm => context total matrix? Can you use a non-abbreviated variable name?

currentTransformationMatrix.  Note that abbreviation is pretty common in WebKit:  GraphicsContext::getCTM(), concatCTM(), setCTM(), etc.  Mind if I keep it?

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:379
>> +        matrix.setScaleY(ctm.getScaleY() ? 1 / ctm.getScaleY() : 1);
> 
> I'm having a hard time convincing myself this is right.  Can the pattern have a scale?
> 
> Can you leave a comment about how you got to this scale value or alternatively use something more intuitive like width / destBitmapWidth?

Yes, the pattern can have a scale, but we've already applied both the CTM and patternTransform's scale in resizedBitmap() above.  The main thing is for ctm * patternTransform to end up at identity scale when the bitmap shader itself runs, otherwise we'll get some additional scaling/interpolation in Skia, which we don't want.  The only way to do that is to have the patternTransform's scale cancel out the CTM's scale.  We can't modify the CTM itself, since we need that to position the actual rect that gets drawn in paintSkPaint().
Comment 8 Stephen White 2012-07-20 14:00:46 PDT
Created attachment 153595 [details]
Added a comment to explain the 1/ctm.scale business
Comment 9 Stephen White 2012-07-20 14:32:43 PDT
Created attachment 153603 [details]
Patch
Comment 10 Adrienne Walker 2012-07-20 16:03:49 PDT
Comment on attachment 153603 [details]
Patch

R=me.  Thanks for the extra comment.
Comment 11 WebKit Review Bot 2012-07-20 16:06:45 PDT
Comment on attachment 153603 [details]
Patch

Rejecting attachment 153603 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
pp
patching file Source/WebCore/platform/graphics/skia/PlatformContextSkia.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 FAILED at 3641.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13313377
Comment 12 Stephen White 2012-07-20 16:47:23 PDT
Created attachment 153621 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-07-20 17:24:47 PDT
Comment on attachment 153621 [details]
Patch for landing

Rejecting attachment 153621 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ebug/obj.target/webcore_platform/Source/WebCore/platform/network/FormDataBuilder.o
Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp: In member function 'void WebCore::PlatformContextSkia::save()':
Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:220: error: 'hasImageResamplingHint' was not declared in this scope
make: *** [out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/skia/PlatformContextSkia.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13318259
Comment 14 Stephen White 2012-07-20 18:29:11 PDT
Created attachment 153635 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-07-20 19:50:46 PDT
Comment on attachment 153635 [details]
Patch for landing

Clearing flags on attachment: 153635

Committed r123285: <http://trac.webkit.org/changeset/123285>
Comment 16 WebKit Review Bot 2012-07-20 19:50:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Stephen White 2012-07-22 08:59:39 PDT
Reopening to attach new patch.
Comment 18 Stephen White 2012-07-22 08:59:42 PDT
Created attachment 153692 [details]
Patch
Comment 19 Stephen White 2012-07-22 09:29:52 PDT
Landed new baselines manually in http://trac.webkit.org/changeset/123297; closed.
Comment 20 Adam Barth 2012-07-27 01:16:07 PDT
Comment on attachment 153692 [details]
Patch

Cleared review? from attachment 153692 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).