Bug 97409 – [Chromium] Incorrect resampling of clipped/masked images.

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Florin Malita]

Created attachment 165287 [details]
Should display a green rect with arrows pointing up and left.

When applying a negative scale + masking/clipping, the result is incorrect (see attached test).

This is only triggered when using high quality interpolation/resampling.

Chromium issue: https://code.google.com/p/chromium/issues/detail?id=150788

[Florin Malita]

Philip Rogers bisected this down to http://trac.webkit.org/changeset/121452, but it looks like that patch simply exposes the issue by enabling high quality interpolation for this particular test.

The problem is that ImageSkia.cpp:drawResampledBitmap() expects the transform to be scaling/translation-only when computing the resampling subregion, but negative scaling can slip through the test and confuse it:

    // High quality interpolation only enabled for scaling and translation.
    if (!(matrix.getType() & (SkMatrix::kAffine_Mask | SkMatrix::kPerspective_Mask)))
        return RESAMPLE_AWESOME;

Forcing RESAMPLE_LINEAR (by adding a no-op rotation for example) is a known workaround.

We could extend that test to also exclude negative scaling, but I think is should be fairly easy to add support for it to drawResampledBitmap(). I'll see if I can come up with a patch.

[Florin Malita]

Created attachment 165295 [details]
Patch

[Florin Malita]

Created attachment 165299 [details]
Patch

Added a text description to help rebaseliners, at pdr's suggestion.

[Stephen White]

hclam@ has a patch up that reworks this area of the code substantially:  https://bugs.webkit.org/show_bug.cgi?id=95121.  That patch is pending a skia fix and roll.

Could you guys get together and figure out if Florin's patch is still needed, and if so, how to resolve the two?

[Florin Malita]

(In reply to comment #4)
> hclam@ has a patch up that reworks this area of the code substantially:  https://bugs.webkit.org/show_bug.cgi?id=95121.  That patch is pending a skia fix and roll.
> 
> Could you guys get together and figure out if Florin's patch is still needed, and if so, how to resolve the two?

Thanks for the pointer Stephen, looks like Hin-Chung's patch takes care of the negative scale issue also (builds a separate transform using setRectToRect and doesn't use the possibly-negative canvas scale directly).

[Hin-Chung Lam]

There's a small change my change can land this week. If this is urgent please go ahead and land it, I'll merge.

[Florin Malita]

(In reply to comment #6)
> There's a small change my change can land this week. If this is urgent please go ahead and land it, I'll merge.

Thanks for the heads up. Stephen, can you review this?

[Stephen White]

Comment on attachment 165299 [details]
Patch

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

Looks good.  r=me

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:208
> +    subsetTransform.reset();
> +    subsetTransform.setScale(SkScalarAbs(canvas.getTotalMatrix().getScaleX()),
> +                             SkScalarAbs(canvas.getTotalMatrix().getScaleY()));

Nit:  I don't think you need to reset() if you immediately setScale().

[Florin Malita]

Created attachment 166234 [details]
Patch for landing

[Florin Malita]

(In reply to comment #8)
> > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:208
> > +    subsetTransform.reset();
> > +    subsetTransform.setScale(SkScalarAbs(canvas.getTotalMatrix().getScaleX()),
> > +                             SkScalarAbs(canvas.getTotalMatrix().getScaleY()));
> 
> Nit:  I don't think you need to reset() if you immediately setScale().

Done, thanks!

[WebKit Review Bot]

Attachment 166234 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/graphics/skia/ImageSkia.cpp:207:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 166234 [details]
Patch for landing

Clearing flags on attachment: 166234

Committed r129897: <http://trac.webkit.org/changeset/129897>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

Also adding to Windows skip list: http://trac.webkit.org/changeset/130090