r93580 which fixed bug 65587 changed the Win & Linux image results for the following three tests: svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html svg/custom/image-small-width-height.svg The new results are close to the old ones but not identical. At first glance it seemed like perhaps these should just be rebaselined. However, the changes are not obviously correct, so I'd like you to take a closer look. Using http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLargeExpectations=true&showExpectations=true&tests=svg%2Fcustom%2Fimage-small-width-height.svg%2Csvg%2Fdynamic-updates%2FSVGFEImageElement-dom-preserveAspectRatio-attr.html%2Csvg%2Fdynamic-updates%2FSVGFEImageElement-svgdom-preserveAspectRatio-prop.html , and opening the expected and actual results for some tests in new tabs to make comparison easy, take a look at (for example) the image-small-width-height results. The existing baseline has two identical images side-by-side (as the test claims is correct). The new result warps the left image vertically (but not horizontally), so that the top and bottom sections appear to shift upwards while the middle stays in one spot. As a result the new left image is not identical to the new right image. Similarly strange changes happen to the other tests, where the image will stretch along one axis and shift on the other. Brett noted that his high-quality resampling code has a bug which offsets pixel centers by 0.5, and so if we're changing when that triggers, we could be introducing new cases of that bug; but that he also thought the original patch wasn't supposed to change that (and that the image behaving differently along the X vs. the Y axis was mystifying). For now, I'm marking these as "expected to fail" in order to keep the tree green.
Thanks Peter, yes I think these tests should be rebaselined. There have always been two resize paths for drawResampledBitmap: 1. resize whole image and paint into whole destRect (let skia clip it). 2. resize only visible subset of cropped image and paint it into clipped destRect. There was a bug in the original code which caused the decision of path (1) or (2) to be harmlessly incorrect. Also, path (1) was only ever taken for uncropped images (srcImage == whole image). These test results are different because patch 65587 changes the decision making between path (1) and (2). The behavior is also slightly different for cropped images: 1. resize cropped image and paint visible subset into clipped destRect. 2. resize visible subset of cropped image and paint it into clipped destRect. The new code always paints the visible subset into the clipped-to-canvas destRect. In the original code, path 1 would paint the whole resized image into the whole destRect (allowing skia code to do the clip) and path 2 would paint the visible subset into the clipped-to-canvas destRect. Why aren't these painting techniques identical? I think it's because the platform/skia ClipToCanvas code is inaccurate -- it converts between int and float to get around a limitation of an int-only skia clip function. I think this would explain the 1-off crap. Ideally, we stay in float space for as long as possible, but we're going to have to add some additional float support in skia and then fix the platform/skia code.
(In reply to comment #1) > Thanks Peter, yes I think these tests should be rebaselined. OK, I'll take care of it. > I think it's because the platform/skia ClipToCanvas code is inaccurate -- it converts between int and float to get around a limitation of an int-only skia clip function. I think this would explain the 1-off crap. Ideally, we stay in float space for as long as possible, but we're going to have to add some additional float support in skia and then fix the platform/skia code. Is there a bug on this somewhere? Should this be that bug? I want to make sure that if something needs to be fixed eventually here, we don't lose that data.
(In reply to comment #2) > Is there a bug on this somewhere? Should this be that bug? I want to make sure that if something needs to be fixed eventually here, we don't lose that data. Absolutely, just filed: http://code.google.com/p/skia/issues/detail?id=345 https://bugs.webkit.org/show_bug.cgi?id=66810
Rebaselined in r93650.