Summary: | ImageSkia should take context scale into account when resizing for drawPattern | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | vollick | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | Stephen White <senorblanco> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | enne, senorblanco, tdanderson, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
vollick
2012-07-05 11:25:21 PDT
Created attachment 150970 [details]
Patch
Comment on attachment 150970 [details]
Patch
Which tests? Why are they skipped?
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. Created attachment 153351 [details]
Patch
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 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 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(). Created attachment 153595 [details]
Added a comment to explain the 1/ctm.scale business
Created attachment 153603 [details]
Patch
Comment on attachment 153603 [details]
Patch
R=me. Thanks for the extra comment.
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 Created attachment 153621 [details]
Patch for landing
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 Created attachment 153635 [details]
Patch for landing
Comment on attachment 153635 [details] Patch for landing Clearing flags on attachment: 153635 Committed r123285: <http://trac.webkit.org/changeset/123285> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 153692 [details]
Patch
Landed new baselines manually in http://trac.webkit.org/changeset/123297; closed. 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). |