Bug 72073 - [Skia] Computing the resampling mode ignores scale applied to the canvas
Summary: [Skia] Computing the resampling mode ignores scale applied to the canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-10 16:13 PST by Daniel Sievers
Modified: 2012-06-28 12:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.23 KB, patch)
2011-11-10 16:14 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (2.69 KB, patch)
2011-11-10 16:17 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (564.50 KB, patch)
2011-11-16 17:54 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (96.59 KB, application/zip)
2011-11-16 20:08 PST, WebKit Review Bot
no flags Details
Patch (568.17 KB, patch)
2011-11-17 14:40 PST, Daniel Sievers
jamesr: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (1.92 MB, patch)
2012-06-27 14:40 PDT, zlieber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Sievers 2011-11-10 16:13:33 PST
[Skia] Computing the resampling mode ignores scale applied to the canvas
Comment 1 Daniel Sievers 2011-11-10 16:14:57 PST
Created attachment 114595 [details]
Patch
Comment 2 Daniel Sievers 2011-11-10 16:17:05 PST
Created attachment 114597 [details]
Patch
Comment 3 John Bates 2011-11-10 18:17:15 PST
Looks good.
Comment 4 Stephen White 2011-11-11 07:46:02 PST
Comment on attachment 114597 [details]
Patch

I'm not sure about this change.  Checking the affine mask means the mapping won't be done when a bitmap is rotated.  Could this mean that an object would switch from high to low quality as it falls in and out of the zero rotation position?

Also, the check should probably be done in computeResamplingMode itself, and we just pass in the canvas matrix, so that it applies to patterns as well as images.  Also, there is already a check for the perspective flag in computeResamplingMode, so this change should probably be unified with that check somehow.  I must admit that this logic is already getting so complex that I'm loathe to add more complexity to it.
Comment 5 Daniel Sievers 2011-11-11 18:52:43 PST
(In reply to comment #4)
> (From update of attachment 114597 [details])
> I'm not sure about this change.  Checking the affine mask means the mapping won't be done when a bitmap is rotated.  Could this mean that an object would switch from high to low quality as it falls in and out of the zero rotation position?


Yes, but that is already the case now whenever we hit the cached awesome sampling path. (Probably more likely from high to bilinear rather than low.)

Are you suggesting we should allow awesome filtering with all rotations or multiples of 90 degrees only? It's probably a bit tricky and would require rewriting drawResampledBitmap() to handle this by transforming the dimensions only in this case etc.


> 
> Also, the check should probably be done in computeResamplingMode itself, and we just pass in the canvas matrix, so that it applies to patterns as well as images.  Also, there is already a check for the perspective flag in computeResamplingMode, so this change should probably be unified with that check somehow.  I must admit that this logic is already getting so complex that I'm loathe to add more complexity to it.


Hrm, this doesn't affect tiles/drawPattern, because the canvas scale is applied at the very end, right? I.e. we would only sample and cache one tile.

I think the current bug/flaw is in the mismatch of how computeResamplingMode() is used in paintSkBitmap() (not incorporating canvas scale) vs. what drawResampledBitmap() does (incorporate canvas scale). It calculates the wrong mode for what it ends up scaling.

Question about drawPattern(): Is patternTransform in that routine always going to be a pure scale/translate transform? I'm not sure I believe the way it undoes the scale in that routine (matrix.setScaleX(SkIntToScalar(1)) works if there is also a rotation or skew.
Comment 6 WebKit Review Bot 2011-11-11 21:50:08 PST
Comment on attachment 114597 [details]
Patch

Attachment 114597 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10460586

New failing tests:
svg/W3C-I18N/text-anchor-inherited-dirLTR-anchorStart.svg
svg/W3C-I18N/text-anchor-dirRTL-anchorStart.svg
svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg
svg/W3C-I18N/text-anchor-dirRTL-anchorEnd.svg
svg/W3C-I18N/text-anchor-inherited-dirLTR-anchorMiddle.svg
svg/W3C-I18N/text-anchor-dirLTR-anchorStart.svg
svg/W3C-I18N/text-anchor-inherited-dirRTL-anchorMiddle.svg
svg/W3C-I18N/text-anchor-inherited-dirRTL-anchorEnd.svg
svg/W3C-I18N/text-anchor-dirNone-anchorStart.svg
svg/W3C-I18N/text-anchor-dirLTR-anchorMiddle.svg
svg/W3C-I18N/text-anchor-dirNone-anchorMiddle.svg
svg/W3C-I18N/text-anchor-dirNone-anchorEnd.svg
fast/borders/border-image-scale-transform.html
svg/W3C-I18N/text-anchor-inherited-dirLTR-anchorEnd.svg
svg/W3C-I18N/text-anchor-dirRTL-anchorMiddle.svg
Comment 7 Stephen White 2011-11-14 12:47:36 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 114597 [details] [details])
> > I'm not sure about this change.  Checking the affine mask means the mapping won't be done when a bitmap is rotated.  Could this mean that an object would switch from high to low quality as it falls in and out of the zero rotation position?
> 
> 
> Yes, but that is already the case now whenever we hit the cached awesome sampling path. (Probably more likely from high to bilinear rather than low.)

Sorry, you're right; this not new to this patch.  (And it's probably not a big problem anyway, since we don't do Lanczos resampling while animating, only on first draw or after animation stops).  Ignore that.

> Are you suggesting we should allow awesome filtering with all rotations or multiples of 90 degrees only? It's probably a bit tricky and would require rewriting drawResampledBitmap() to handle this by transforming the dimensions only in this case etc.

No, I don't think we should do that.

> > Also, the check should probably be done in computeResamplingMode itself, and we just pass in the canvas matrix, so that it applies to patterns as well as images.  Also, there is already a check for the perspective flag in computeResamplingMode, so this change should probably be unified with that check somehow.  I must admit that this logic is already getting so complex that I'm loathe to add more complexity to it.
> 
> 
> Hrm, this doesn't affect tiles/drawPattern, because the canvas scale is applied at the very end, right? I.e. we would only sample and cache one tile.

I was just thinking (perhaps naively) that if a pattern was drawn with full-page zoom applied, it should use the same heuristic as images.  For example, you could create an SVG pattern with a single image tile, drawn at 1:1 scale in SVG, then with a page zoom.  It seems strange that full-page zoom would affect the criteria for high-quality resize in one case but not the other.

> I think the current bug/flaw is in the mismatch of how computeResamplingMode() is used in paintSkBitmap() (not incorporating canvas scale) vs. what drawResampledBitmap() does (incorporate canvas scale). It calculates the wrong mode for what it ends up scaling.

Fair enough.  Since this code has very little testing (and I only barely understand it myself), I'd like to see a test case that demonstrates the problem you're seeing, and its behaviour with and without this patch.

> Question about drawPattern(): Is patternTransform in that routine always going to be a pure scale/translate transform? I'm not sure I believe the way it undoes the scale in that routine (matrix.setScaleX(SkIntToScalar(1)) works if there is also a rotation or skew.

AFAICT, that code is only executed when computeResamplingMode() returns RESAMPLE_AWESOME, so we already know that there is no rotation or skew in that case.
Comment 8 Daniel Sievers 2011-11-16 17:42:18 PST
> > 
> > Hrm, this doesn't affect tiles/drawPattern, because the canvas scale is applied at the very end, right? I.e. we would only sample and cache one tile.
> 
> I was just thinking (perhaps naively) that if a pattern was drawn with full-page zoom applied, it should use the same heuristic as images.  For example, you could create an SVG pattern with a single image tile, drawn at 1:1 scale in SVG, then with a page zoom.  It seems strange that full-page zoom would affect the criteria for high-quality resize in one case but not the other.
>

Yes, it currently doesn't
 
> > I think the current bug/flaw is in the mismatch of how computeResamplingMode() is used in paintSkBitmap() (not incorporating canvas scale) vs. what drawResampledBitmap() does (incorporate canvas scale). It calculates the wrong mode for what it ends up scaling.
> 
> Fair enough.  Since this code has very little testing (and I only barely understand it myself), I'd like to see a test case that demonstrates the problem you're seeing, and its behaviour with and without this patch.
> 


Turns out the newly failing tests above show this. For example, fast/borders/border-image-scale-transform.html applies a '-webkit-transform scale(2)' which before my change was unnoticed when deciding on the filtering causing us to use simple filtering for the images, while with this change it uses awesome, so they need to be rebased. Similarly, in the other tests the svg 'viewBox' property in the other tests cause a scale on a canvas. Without this patch, we don't detect this scale, but only see scales on the content, e.g. from width/height in the img tag. 


> > Question about drawPattern(): Is patternTransform in that routine always going to be a pure scale/translate transform? I'm not sure I believe the way it undoes the scale in that routine (matrix.setScaleX(SkIntToScalar(1)) works if there is also a rotation or skew.
> 
> AFAICT, that code is only executed when computeResamplingMode() returns RESAMPLE_AWESOME, so we already know that there is no rotation or skew in that case.

It looks at the canvas matrix but not at patternTransform to detect if there is only scale+translate, so that might be a problem. It uses the function TransformDimensions() to figure out what scaling patternTransform does.
Comment 9 Daniel Sievers 2011-11-16 17:54:55 PST
Created attachment 115496 [details]
Patch
Comment 10 Daniel Sievers 2011-11-16 18:48:20 PST
> > Hrm, this doesn't affect tiles/drawPattern, because the canvas scale is applied at the very end, right? I.e. we would only sample and cache one tile.
> 
> I was just thinking (perhaps naively) that if a pattern was drawn with full-page zoom applied, it should use the same heuristic as images.  For example, you could create an SVG pattern with a single image tile, drawn at 1:1 scale in SVG, then with a page zoom.  It seems strange that full-page zoom would affect the criteria for high-quality resize in one case but not the other.
> 


Actually, drawPattern() is a bit busted. If you look at fast/borders/border-image-scale-transform.html there is some areas that go through that codepath and actually end up using nearest filtering for the canvas scale.

This does not take into account the canvas scale or destRect:
 paint.setFilterBitmap(resampling == RESAMPLE_LINEAR);

'resampling' was decided upon based on affineTransform only.
Comment 11 WebKit Review Bot 2011-11-16 20:08:43 PST
Comment on attachment 115496 [details]
Patch

Attachment 115496 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10340047

New failing tests:
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.composite.clip.destination-atop.html
canvas/philip/tests/2d.composite.clip.source-out.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.globalAlpha.canvas.html
canvas/philip/tests/2d.composite.canvas.destination-in.html
animations/3d/matrix-transform-type-animation.html
canvas/philip/tests/2d.composite.canvas.source-out.html
animations/3d/state-at-end-event-transform.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html
canvas/philip/tests/2d.composite.clip.source-in.html
canvas/philip/tests/2d.composite.canvas.copy.html
canvas/philip/tests/2d.composite.clip.destination-in.html
animations/3d/replace-filling-transform.html
animations/3d/change-transform-in-end-event.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
canvas/philip/tests/2d.composite.canvas.lighter.html
Comment 12 WebKit Review Bot 2011-11-16 20:08:47 PST
Created attachment 115515 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 13 Daniel Sievers 2011-11-17 14:40:39 PST
Created attachment 115687 [details]
Patch
Comment 14 Stephen White 2011-11-17 14:44:22 PST
Adding brettw since I'm still not sure if this is the right thing to do or not.  Should the scale be applied?
Comment 15 WebKit Review Bot 2011-11-17 20:10:39 PST
Comment on attachment 115687 [details]
Patch

Attachment 115687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10509498

New failing tests:
svg/W3C-SVG-1.1/render-groups-03-t.svg
svg/carto.net/scrollbar.svg
svg/W3C-I18N/text-anchor-no-markup.svg
svg/W3C-SVG-1.1/render-groups-01-b.svg
svg/custom/image-small-width-height.svg
svg/custom/scrolling-embedded-svg-file-image-repaint-problem.html
svg/W3C-I18N/text-anchor-inherited-dirRTL-anchorStart.svg
Comment 16 James Robinson 2012-02-21 20:20:43 PST
Comment on attachment 115687 [details]
Patch

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

How do we make progress here?

> Source/WebCore/ChangeLog:14
> +        Reviewed by Stephen White.

this is incorrect according to the bug history, Stephen hasn't r+'d this
Comment 17 zlieber 2012-06-27 14:40:31 PDT
Created attachment 149799 [details]
Patch
Comment 18 Stephen White 2012-06-28 11:52:52 PDT
Comment on attachment 149799 [details]
Patch

Looks good.  r=me
Comment 19 WebKit Review Bot 2012-06-28 12:39:10 PDT
Comment on attachment 149799 [details]
Patch

Clearing flags on attachment: 149799

Committed r121452: <http://trac.webkit.org/changeset/121452>
Comment 20 WebKit Review Bot 2012-06-28 12:39:15 PDT
All reviewed patches have been landed.  Closing bug.