Bug 72073 – [Skia] Computing the resampling mode ignores scale applied to the canvas

• Component: New Bugs
• Status: RESOLVED FIXED

[Daniel Sievers]

[Skia] Computing the resampling mode ignores scale applied to the canvas

[Daniel Sievers]

Created attachment 114595 [details]
Patch

[Daniel Sievers]

Created attachment 114597 [details]
Patch

[John Bates]

Looks good.

[Stephen White]

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.

[Daniel Sievers]

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

[WebKit Review Bot]

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

[Stephen White]

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

[Daniel Sievers]

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

[Daniel Sievers]

Created attachment 115496 [details]
Patch

[Daniel Sievers]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Daniel Sievers]

Created attachment 115687 [details]
Patch

[Stephen White]

Adding brettw since I'm still not sure if this is the right thing to do or not.  Should the scale be applied?

[WebKit Review Bot]

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

[James Robinson]

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

Created attachment 149799 [details]
Patch

[Stephen White]

Comment on attachment 149799 [details]
Patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 149799 [details]
Patch

Clearing flags on attachment: 149799

Committed r121452: <http://trac.webkit.org/changeset/121452>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.