Bug 90624 – ImageSkia should take context scale into account when resizing for drawPattern

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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.

Created attachment 150970 [details]
Patch

[Adrienne Walker]

Comment on attachment 150970 [details]
Patch

Which tests? Why are they skipped?

[Stephen White]

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.

[Stephen White]

Created attachment 153351 [details]
Patch

[Stephen White]

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.

[Adrienne Walker]

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?

[Stephen White]

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

[Stephen White]

Created attachment 153595 [details]
Added a comment to explain the 1/ctm.scale business

[Stephen White]

Created attachment 153603 [details]
Patch

[Adrienne Walker]

Comment on attachment 153603 [details]
Patch

R=me.  Thanks for the extra comment.

[WebKit Review Bot]

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

[Stephen White]

Created attachment 153621 [details]
Patch for landing

[WebKit Review Bot]

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

[Stephen White]

Created attachment 153635 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 153635 [details]
Patch for landing

Clearing flags on attachment: 153635

Committed r123285: <http://trac.webkit.org/changeset/123285>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

Reopening to attach new patch.

[Stephen White]

Created attachment 153692 [details]
Patch

[Stephen White]

Landed new baselines manually in http://trac.webkit.org/changeset/123297; closed.

[Adam Barth]

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