Bug 95121 - [skia] Drawing of sub-rectangle in a bitmap is mis-aligned
Summary: [skia] Drawing of sub-rectangle in a bitmap is mis-aligned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hin-Chung Lam
URL:
Keywords:
Depends on:
Blocks: 94240
  Show dependency treegraph
 
Reported: 2012-08-27 13:39 PDT by Hin-Chung Lam
Modified: 2012-10-04 14:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.52 KB, patch)
2012-08-27 14:19 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (2.63 MB, application/zip)
2012-08-27 16:11 PDT, WebKit Review Bot
no flags Details
Patch (44.78 KB, patch)
2012-09-03 11:33 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (58.75 KB, patch)
2012-09-04 14:59 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (57.19 KB, patch)
2012-09-04 15:12 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (57.16 KB, patch)
2012-09-05 14:39 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (55.64 KB, patch)
2012-09-06 18:05 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (56.04 KB, patch)
2012-09-06 18:31 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (56.20 KB, patch)
2012-09-11 13:59 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (56.27 KB, patch)
2012-09-12 14:40 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (57.79 KB, patch)
2012-09-13 23:55 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (57.75 KB, patch)
2012-09-14 00:01 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (52.98 KB, patch)
2012-09-20 14:29 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (59.14 KB, patch)
2012-10-03 12:44 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (59.27 KB, patch)
2012-10-03 14:32 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch for landing (59.22 KB, patch)
2012-10-04 11:18 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2012-08-27 13:39:21 PDT
When drawing a sub-rectangle in a bitmap with float coordinates, e.g. (0.5, 0), WebKit Skia port round it to the enclosing integer rectangle. This creates an error of address the source bitmap by at most 2 pixels in width and height respectively. See crbug.com/117597 for more details.
Comment 1 Hin-Chung Lam 2012-08-27 14:19:46 PDT
Created attachment 160805 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-27 16:11:52 PDT
Comment on attachment 160805 [details]
Patch

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

New failing tests:
fast/backgrounds/size/contain-and-cover.html
fast/backgrounds/repeat/mask-negative-offset-repeat.html
svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html
fast/repaint/background-misaligned.html
svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html
fast/backgrounds/size/contain-and-cover-zoomed.html
Comment 3 WebKit Review Bot 2012-08-27 16:11:55 PDT
Created attachment 160842 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 4 Mike Reed 2012-08-29 12:13:18 PDT
ok w/ me
Comment 5 Hin-Chung Lam 2012-08-29 12:26:50 PDT
Those errors are fine. We just need rebaseline for those pixel results.
Comment 6 Stephen White 2012-08-29 13:27:32 PDT
Comment on attachment 160805 [details]
Patch

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

Thanks for taking this on.  If there are layout test failures, please add them to LayoutTests/platform/chromium/TestExpectations in the same patch.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:454
> +    return fabs(rect.x() - roundedRect.x()) >= epsilon

Sorry, my numerical methods course has paged out.  Why do we compare diff >= epsilon instead of diff > 0?  Does the latter trigger too often?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:559
>      paintSkBitmap(ctxt->platformContext(),
>                    *bm,
> -                  enclosingIntRect(normSrcRect),
> -                  normDstRect,
> +                  enclosingSrcRect,
> +                  enclosingDstRect,
>                    WebCoreCompositeToSkiaComposite(compositeOp));
>  

Doesn't BitmapImageSingleFrameSkia::draw() suffer from the same problem?  If so, perhaps this logic should be moved to paintSkBitmap() instead.

(Note that Image::drawPattern(), often used for drawing 9-pieces and backgrounds may also have a similar problem, but that might be trickier to solve.)
Comment 7 Hin-Chung Lam 2012-08-29 16:42:14 PDT
Comment on attachment 160805 [details]
Patch

Thanks. I'll move this to paintSkBitmap.
Comment 8 Hin-Chung Lam 2012-09-03 11:33:17 PDT
Created attachment 161942 [details]
Patch
Comment 9 Hin-Chung Lam 2012-09-04 14:59:09 PDT
Created attachment 162104 [details]
Patch
Comment 10 Hin-Chung Lam 2012-09-04 15:12:33 PDT
Created attachment 162106 [details]
Patch
Comment 11 Hin-Chung Lam 2012-09-04 15:16:08 PDT
The last patch is good for review. It solved 2 very closely related problems in one patch. It didn't make sense to solve these two problems separately a) source rect has to be aligned to integer boundaries in SkCanvas b) image fragments don't stitch together well because scale factors are slightly different.
Comment 12 Hin-Chung Lam 2012-09-05 14:39:11 PDT
Created attachment 162338 [details]
Patch
Comment 13 Hin-Chung Lam 2012-09-05 14:40:01 PDT
New patch to merge with ToT.
Comment 14 Nick Carter 2012-09-06 12:15:03 PDT
Publishing some comments ... I haven't seen any problems with the math so far, but I am still working through the cases to understand it fully.
Comment 15 Nick Carter 2012-09-06 15:13:25 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=162338&action=review

> Source/WebCore/ChangeLog:9
> +        to integer boundaries, skia round it to the closest enclosing integer

skia round -> skia expands

> Source/WebCore/ChangeLog:14
> +        for both cases of RESAMPLE_AWSOME and RESAMPLE_LINEAR.

AWSOME -> AWESOME

> Source/WebCore/ChangeLog:18
> +        appropriarely to ensure source rectangle is aligned.

appropriarely -> appropriately

> Source/WebCore/ChangeLog:20
> +        This patch also fixes a closely related problem. In RESAMPLE_AWSOME

AWSOME -> AWESOME

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:177
> +// The reason we do this manually is to preserve accuracy for SkIRect. And this is

Is accuracy really preserved when T is SkIRect? It looks to me as if there's not much integer math here at all, things are immediately converted to float.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:192
> +    return otherRectTransformed;

Seems like this whole function could be replaced with SkMatrix operations by using setRectToRect (to initialize the matrix using srcRect/dstRect) and mapRect (to transform otherRect). Consider doing this.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:199
> +static bool needsFancyAlignment(const SkRect& rect)

Would it be clearer if this function were named areBoundariesIntegerAligned()? The name "fancy alignment" could develop a specific meaning as a presentation strategy in the calling function, but this helper function should probably just be named for the mathematical operation it performs.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:266
> +static SkRect rectMulDiv(const SkRect& rect, int mulX, int denomX, int mulY, int denomY)

Have a look at SkMatrix (e.g. postIDiv) as a possible alternative for replacing this function altogether, though this might wind up being more lines of code.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:367
> +    // a difference then we can just blit it directly to enhance performance.

It's not entirely clear from reading it, whether this comment describes an optimization implemented today by canvas.drawBitmapRect, or whether it describes an idea for future improvements.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:531
> +        // fragment is slighyly larger to align to integer

slighyly -> slightly

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:536
>          // Since we just resized the bitmap, we need to remove the scale 

It's not clear to me where we account for the shift that is introduced by the expansion of enclosingIntRect. Shouldn't the value of enclosingScaledSrcRect be used somewhere?

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:83
> +            // FIXME: Scale the entire image instead of just the subset.

Seems like you should really fix this.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:117
> +    if (fragmentSize > kLargeBitmapSize)

This ought to be fullSize, I think.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:123
> +    if (fragmentSize <= kSmallBitmapSize)

I think you're changing the caching policy here. It doesn't look right to me. This ought to be fullSize.
Comment 16 Hin-Chung Lam 2012-09-06 18:05:33 PDT
Created attachment 162634 [details]
Patch
Comment 17 Hin-Chung Lam 2012-09-06 18:07:00 PDT
I have updated the patch with the problems fixed. I have included the fixes for scaled image caching also in this patch.
Comment 18 Hin-Chung Lam 2012-09-06 18:31:43 PDT
Created attachment 162639 [details]
Patch
Comment 19 Hin-Chung Lam 2012-09-11 13:59:22 PDT
Created attachment 163429 [details]
Patch
Comment 20 Hin-Chung Lam 2012-09-11 13:59:43 PDT
Merged again because of TestExpectations..
Comment 21 Nick Carter 2012-09-11 14:37:38 PDT
Things look pretty good. Everything under paintSkBitmap seems fine to me. Maybe I'm misunderstanding them, I'm still not totally convinced that the changes to the sampled pixels in the drawPattern/AWESOME path are really an improvement; I'd be curious to see image diffs for that path in the case where srcRect is not the whole src image. Any idea when that might happen?

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:227
> +    }

Of slight interest to me is the fact that the math isn't completely X/Y separable here. If both X and Y are integer aligned, we round. But what if X is integer aligned and Y is not? In that case we may expand X anyway (300 may convert to 300.00000001 and then expand to 301) and count on clipping to cause the extra column of pixels to be blended with effectively zero contribution. Does this distinction matter?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:236
> +    transform.mapRect(&enclosingDstRect);

You can skip the temp variable |enclosingDstRect| by using the 2-argument (*dst, &src) version of mapRect:

transform.mapRect(outDstRect, enclosingSrcRect);

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:246
> +// Because the scaled image size has to be integers. We approximate the real

"integers. We" -> "integers, we"

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:353
> +    // factor. This draw will perform a close to 1 scaling,

Incomplete sentence.
Comment 22 WebKit Review Bot 2012-09-11 18:59:33 PDT
Comment on attachment 163429 [details]
Patch

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

New failing tests:
inspector/timeline/timeline-decode-resize.html
Comment 23 Hin-Chung Lam 2012-09-12 14:39:41 PDT
(In reply to comment #21)
> Things look pretty good. Everything under paintSkBitmap seems fine to me. Maybe I'm misunderstanding them, I'm still not totally convinced that the changes to the sampled pixels in the drawPattern/AWESOME path are really an improvement; I'd be curious to see image diffs for that path in the case where srcRect is not the whole src image. Any idea when that might happen?
>

I think the code path for drawPattern (both AWSOME and LINEAR) won't have a material improvement. We could fix it in a separate patch and I need test to see the difference.
 
> View in context: https://bugs.webkit.org/attachment.cgi?id=162639&action=review
> 
> > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:227
> > +    }
> 
> Of slight interest to me is the fact that the math isn't completely X/Y separable here. If both X and Y are integer aligned, we round. But what if X is integer aligned and Y is not? In that case we may expand X anyway (300 may convert to 300.00000001 and then expand to 301) and count on clipping to cause the extra column of pixels to be blended with effectively zero contribution. Does this distinction matter?
> 
> > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:236
> > +    transform.mapRect(&enclosingDstRect);
> 
> You can skip the temp variable |enclosingDstRect| by using the 2-argument (*dst, &src) version of mapRect:
>

I do this because enclosingDstRect is SkRect while enclosingSrcRect is SkIRect hence the need to have this temporary variable.
 
> transform.mapRect(outDstRect, enclosingSrcRect);
> 
> > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:246
> > +// Because the scaled image size has to be integers. We approximate the real
> 
> "integers. We" -> "integers, we"
> 
> > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:353
> > +    // factor. This draw will perform a close to 1 scaling,
> 
> Incomplete sentence.

Both done.
Comment 24 Hin-Chung Lam 2012-09-12 14:40:15 PDT
Created attachment 163696 [details]
Patch
Comment 25 Hin-Chung Lam 2012-09-12 14:41:09 PDT
New patch with comments addressed and try to green cr-linux EWS bot.
Comment 26 WebKit Review Bot 2012-09-12 17:47:53 PDT
Comment on attachment 163696 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/fast/hidpi/image-set-border-image-simple.html
Comment 27 Stephen White 2012-09-13 09:30:40 PDT
Comment on attachment 163696 [details]
Patch

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

Is there a possibility that we'll now being doing both Lanczos and bilinear filtering in the same draw?  (I couldn't tell from reading the code).  I just eliminated one case of that recently, since it looked pretty bad.  Perhaps it's unavoidable in this case, however.

If possible, I'd also like to get brettw's input on the NativeImageSkia changes.  I'm a little concerned about the change to always cache the full-size resized image.  It'll be a win for (for example) pulling many subsets of the same size, but I think it'll be a perf hit if you always extract a small subset of a large resize.

At the very least, we should watch the page cyclers closely after this patch lands.

> Source/WebCore/ChangeLog:31
> +        Changes in Skia enable caching of the entire scaled image and return
> +        the scaled fragment because scale factor is now consistent for all
> +        fragments.

If you blow up a small image to a large size, but only use a small subset, won't this change negatively impact performance?

> Source/WebCore/ChangeLog:50
> +        A fragment is now identified by (scaledImageSize, scaledImagSubset).

scaledImagSubset -> scaledImageSubset

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:121
> +        if (diffWidth < std::numeric_limits<float>::epsilon()
> +            || diffHeight < std::numeric_limits<float>::epsilon()) {

Nit:  might be more readable to cache these in a temporary bool, say "widthNearlyEqual" and "heightNearlyEqual", and use it both here and at lines 98-99 above.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:285
> +    scaledSrcRect->intersect(scaledImageRect);

Should we be checking the return value here?  Or is the intersection always guaranteed to be non-empty?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:326
> +    // Destination rectangle is shrinked, find the corresponding rect in source
> +    // image.

Nit:  "shrinked" is not a word and there's a comma splice here.  Suggest you just delete the first phrase altogether, since it's obvious from context.  Also, "in source image" -> "in the source image".

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:337
> +    // Expand destination rectangle because source rectangle is expanded to fit
> +    // to integer boundaries.

"Expand destination"- > "Expand the destination", "source rectangle is expanded" -> "the source rectangle was expanded" (I think?)

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:350
> +    canvas.save();
> +    canvas.clipRect(destRectVisibleSubset);

I'm concerned about the performance implications of always doing clipping.  Can we skip clipping in some cases?

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:74
> +    // given image subset with the given scale and subset.

Comment seems wrong, since there's only a single parameter now?
Comment 28 Hin-Chung Lam 2012-09-13 23:27:43 PDT
Addressed all nits. Here's the answers to your questions about performance.

> Is there a possibility that we'll now being doing both Lanczos and bilinear filtering in the same draw?  (I couldn't tell from reading the code).  I just eliminated one case of that recently, since it looked pretty bad.  Perhaps it's unavoidable in this case, however.

Unfortunately we have always been doing it. The new code does not eliminate the bilinear filtering. This is unavoidable because we manually scale a fragment and try to stamp it onto the canvas, there is very likely a scale mismatch. The bilinear filtering is to cover this very small difference in scale.

Another case when bilinear filtering is unavoidable is when target rectangle in the screen space is not aligned to integer boundaries, this is probably very common with CSS scale transform.

The only way to eliminate bilinear filter completely is to have the canvas (rasterizer) does resampling and blends with target. However this limits that we can't do scaling (and cache) before rasterization. 

Note that WebKit CG also has this double scaling problem. http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp#L307
The reason they do this is not for optimization though but to avoid bleeding caused by fancy resampling algorithm tends to sample outside the source rectangle.

In any case the new code is not worse, there is at most 2 filtering.

> 
> If possible, I'd also like to get brettw's input on the NativeImageSkia changes.  I'm a little concerned about the change to always cache the full-size resized image.  It'll be a win for (for example) pulling many subsets of the same size, but I think it'll be a perf hit if you always extract a small subset of a large resize.
> 
> At the very least, we should watch the page cyclers closely after this patch lands.

Sure. I have changed the code such that we don't care the full resized image but just the subrect. The old code identifies a fragment as (src rect, scaled src size, dest rect) and new code is (scaled image size, dest rect) so the logic has to change with regard to caching. I tried to match the logic with the old code as much as possible.

> 
> If you blow up a small image to a large size, but only use a small subset, won't this change negatively impact performance?
> 

In the latest patch I tried to match the old code's behavior as much as possible, i.e. not caching the entire scaled image but just the scaled fragment. I think this won't be any worse. In fact because scale factor is consistent now the new code can extract a subrect in the cached resized fragment.

> I'm concerned about the performance implications of always doing clipping.  Can we skip clipping in some cases?

This is probably the only thing that is worse performance-wise. Mike is working on getting drawBitmapRect to take SkRect has source. When that is done we don't need clipping at all.
Comment 29 Hin-Chung Lam 2012-09-13 23:55:24 PDT
Created attachment 164059 [details]
Patch
Comment 30 Hin-Chung Lam 2012-09-14 00:01:06 PDT
Created attachment 164062 [details]
Patch
Comment 31 Stephen White 2012-09-17 11:04:04 PDT
Comment on attachment 164062 [details]
Patch

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

> Source/WebCore/ChangeLog:31
> +        Changes in Skia enable caching of the scaled image and return the
> +        scaled fragment because scale factor is now consistent for all
> +        fragments.

Is this still the case, now that we're keying off the scaled subrect size?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:229
> +    enclosingSrcRect.intersect(bitmapRect); // Clip to bitmap rectangle.

Still wondering if we need to check the return value of intersect() here, or if it's handled by the caller?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:288
> +    enclosingScaledSrcRect->intersect(SkIRect::MakeSize(scaledImageSize));

As above:  do we need to check the return value of intersect() here, or is an empty rect ok?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:347
> +    canvas.clipRect(destRectVisibleSubset);

Still concerned about clipping performance here.  Mike:  should I worry about this?  Is Skia fast enough at a simple clipped rect that this is not a concern?  If not, Alpha, is there a way we can skip clipping when unnecessary?
Comment 32 Mike Reed 2012-09-17 11:17:19 PDT
drawBitmapRectToRect(src=float, dst=float) is now pending on the skia side. It should land in chrome/webkit in a day or 2 (after the M23 branch).

If/when that lands, will this code still need the clipRect?
Comment 33 Hin-Chung Lam 2012-09-17 11:20:23 PDT
(In reply to comment #32)
> drawBitmapRectToRect(src=float, dst=float) is now pending on the skia side. It should land in chrome/webkit in a day or 2 (after the M23 branch).
> 
> If/when that lands, will this code still need the clipRect?

clipRect will not be necessary. I'm very happy to take that off the patch. I can wait for your change to land.
Comment 34 Hin-Chung Lam 2012-09-17 11:24:19 PDT
Comment on attachment 164062 [details]
Patch

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

Taking this off review since we're holding this until Skia's API change took place.

>> Source/WebCore/ChangeLog:31
>> +        fragments.
> 
> Is this still the case, now that we're keying off the scaled subrect size?

This is still the case. We don't cache the scaled full image but scaled fragment. This fragment can be used to cover a smaller scale request because scale factor is now consistent.

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:229
>> +    enclosingSrcRect.intersect(bitmapRect); // Clip to bitmap rectangle.
> 
> Still wondering if we need to check the return value of intersect() here, or if it's handled by the caller?

This is handled by caller. Line 415 checks to see of either of the output rectangles are empty.

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:288
>> +    enclosingScaledSrcRect->intersect(SkIRect::MakeSize(scaledImageSize));
> 
> As above:  do we need to check the return value of intersect() here, or is an empty rect ok?

It is okay to be empty. We just get an empty image below.

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:347
>> +    canvas.clipRect(destRectVisibleSubset);
> 
> Still concerned about clipping performance here.  Mike:  should I worry about this?  Is Skia fast enough at a simple clipped rect that this is not a concern?  If not, Alpha, is there a way we can skip clipping when unnecessary?

Looks like Mike's change is going to land soon. I can hold this off until then. We don't nee to clip with the new API.
Comment 35 Hin-Chung Lam 2012-09-17 11:29:27 PDT
(In reply to comment #32)
> drawBitmapRectToRect(src=float, dst=float) is now pending on the skia side. It should land in chrome/webkit in a day or 2 (after the M23 branch).
> 
> If/when that lands, will this code still need the clipRect?

Hm.. can you point me to the Skia change?

Actaully drawBitmapRectToRect() seems to solve only half of the problem.

There's two drawing path:

1. Canvas only has scale and translation transformation

drawBitmapRectToRect() can eliminate the use of clip.

2. Canvas has transformation other than scale and translation

drawBitmapRect() needs to take SkRect instead of SkIRect to eliminate clipping.
Comment 36 Hin-Chung Lam 2012-09-17 11:38:11 PDT
(In reply to comment #35)
> (In reply to comment #32)
> > drawBitmapRectToRect(src=float, dst=float) is now pending on the skia side. It should land in chrome/webkit in a day or 2 (after the M23 branch).

Just checked with Mike, the new API behaves the same as drawBitmapRect except taking SkRect which is exactly what I need. I'll wait until his change lands.
Comment 37 Mike Reed 2012-09-20 07:41:32 PDT
drawBitmapRectToRect has landed in chrome. Should be available to webkit.

drawBitmapRect is a ship that just converts its IRect to a Rect, and calls drawBitmapRectToRect.
Comment 38 Hin-Chung Lam 2012-09-20 14:29:36 PDT
Created attachment 164983 [details]
Patch
Comment 39 Hin-Chung Lam 2012-09-20 14:36:40 PDT
Stephen: Please review the code again. The code itself is complete and using Mike's API of drawBitmapRectToRect so there is no performance drop. Expected results of this patch is also correct.

However two things need to happen before I can land this:
1. I observed a problem with the new API, submitted a skia patch for review and Mike is reviewing.
2. Roll chromium DEPS in WebKit after things are landed and rolled.
Comment 40 Stephen White 2012-09-20 14:48:51 PDT
Comment on attachment 164983 [details]
Patch

This all looks great to me.  Thanks for going the extra mile.

While we're waiting for the skia fixes and roll, could you send me the results of fast/backgrounds/size/backgroundSize15.html, fast/backgrounds/size/contain-and-cover.html and fast/backgrounds/size/contain-and-cover-zoomed.html with this patch?  I'd just like to see what the effect are, if any.  Thanks!
Comment 41 WebKit Review Bot 2012-09-20 15:37:44 PDT
Comment on attachment 164983 [details]
Patch

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

New failing tests:
fast/images/paint-subrect.html
fast/images/paint-subrect-grid.html
fast/images/repaint-subrect-grid.html
scrollbars/listbox-scrollbar-combinations.html
Comment 42 Hin-Chung Lam 2012-09-21 10:44:07 PDT
This change depends in a proper skia fix from Mike otherwise everything else is good. Since a skia fix will probably take an extra week I'll leave this bug and proceed with the deferred image decoding patch.
Comment 43 James Robinson 2012-09-27 00:49:25 PDT
(In reply to comment #42)
> This change depends in a proper skia fix from Mike otherwise everything else is good. Since a skia fix will probably take an extra week I'll leave this bug and proceed with the deferred image decoding patch.

How are we now on the skia fix?
Comment 44 Mike Reed 2012-09-27 05:12:42 PDT
skia fix for drawBitmapRectToRect is in 5663
Comment 45 Hin-Chung Lam 2012-09-27 10:26:15 PDT
Skia roll in Chromium is just landed. I'll roll DEPS in WebKit shortly and then update this patch accordingly (test expectations).
Comment 46 Hin-Chung Lam 2012-09-27 10:45:21 PDT
Actually Skia roll for Chromium hasn't landed yet, looks like Mike is fixing it.
Comment 47 Mike Reed 2012-09-27 11:22:09 PDT
its more complicated than that.

The new code seems to vary by some low-bit, some of the time, depending on optimization levels, if the dst-rect lands on a 1/2 pixel boundary. Thus a few layouttests break if we enable it (and most pass).

At the moment the "fix" is disabled via a build-flag in skia.gyp... I am working now to see if I can make *all* 1/2 pixel cases pass, or to document that it can't reasonably be done, and just rebaseline those.

If nothing else, you should be able to remove this define from skia.gyp locally, and test that you like the new behavior...
 
search for SK_SUPPORT_INT_SRCRECT_DRAWBITMAPRECT
Comment 48 Hin-Chung Lam 2012-09-27 11:25:52 PDT
Will do.
Comment 49 Hin-Chung Lam 2012-09-27 17:29:20 PDT
I ran the tests I created without SK_SUPPORT_INT_SRCRECT_DRAWBITMAPRECT and the results look good to me.

If the regression caused by your change is acceptable (like there's no way to fix it or neglectable) I would like to roll it into Chromium and land this change.
Comment 50 Hin-Chung Lam 2012-09-28 13:35:43 PDT
Mike: any updates on the skia fix?
Comment 51 Mike Reed 2012-09-28 14:01:10 PDT
Was not able to dig further today. Hope to resolve it early next week.
Comment 52 Hin-Chung Lam 2012-10-03 11:16:31 PDT
Stephen, Mike: Looks like it will take longer for the fix to drawBitmapRectToRect to land. I would like to land the version with explicit clipping and update the code when the new API is ready, does it make sense? This change is blocking further progress to deferred image decoding.
Comment 53 Mike Reed 2012-10-03 11:23:38 PDT
SGTM -- I thought the skia fix would happen quicker, but it is taking longer than I hoped.
Comment 54 Hin-Chung Lam 2012-10-03 11:24:52 PDT
Thanks Mike! I'll upload a new patch.
Comment 55 Stephen White 2012-10-03 11:47:30 PDT
(In reply to comment #52)
> Stephen, Mike: Looks like it will take longer for the fix to drawBitmapRectToRect to land. I would like to land the version with explicit clipping and update the code when the new API is ready, does it make sense? This change is blocking further progress to deferred image decoding.

OK.  I'm still a little concerned about the clipping perf hit, but we're early in the M24 cycle, so we have time to fix it.
Comment 56 Hin-Chung Lam 2012-10-03 12:44:13 PDT
Created attachment 166942 [details]
Patch
Comment 57 Hin-Chung Lam 2012-10-03 12:45:25 PDT
Switched back to having an explicit clip. We should fix the performance problem as a follow up fix.
Comment 58 WebKit Review Bot 2012-10-03 14:30:46 PDT
Comment on attachment 166942 [details]
Patch

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

New failing tests:
scrollbars/overflow-scrollbar-combinations.html
Comment 59 Hin-Chung Lam 2012-10-03 14:32:51 PDT
Created attachment 166961 [details]
Patch
Comment 60 Hin-Chung Lam 2012-10-03 14:33:22 PDT
One more suppression. Verified the results just need rebaseline. It should be good this time.
Comment 61 Hin-Chung Lam 2012-10-03 15:59:44 PDT
All bots are green now. Please take a look.
Comment 62 Stephen White 2012-10-03 18:54:06 PDT
Comment on attachment 166961 [details]
Patch

r=me.  Please watch the page cyclers on landing.
Comment 63 Hin-Chung Lam 2012-10-04 11:18:06 PDT
Created attachment 167140 [details]
Patch for landing
Comment 64 WebKit Review Bot 2012-10-04 11:40:47 PDT
Comment on attachment 167140 [details]
Patch for landing

Clearing flags on attachment: 167140

Committed r130412: <http://trac.webkit.org/changeset/130412>
Comment 65 WebKit Review Bot 2012-10-04 11:40:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 66 Hin-Chung Lam 2012-10-04 13:56:39 PDT
Phew.. perf bots are green!
Comment 67 Stephen White 2012-10-04 14:04:53 PDT
(In reply to comment #66)
> Phew.. perf bots are green!

w00t!