Bug 95121 – [skia] Drawing of sub-rectangle in a bitmap is mis-aligned

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 160805 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike Reed]

ok w/ me

[Hin-Chung Lam]

Those errors are fine. We just need rebaseline for those pixel results.

[Stephen White]

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

[Hin-Chung Lam]

Comment on attachment 160805 [details]
Patch

Thanks. I'll move this to paintSkBitmap.

[Hin-Chung Lam]

Created attachment 161942 [details]
Patch

[Hin-Chung Lam]

Created attachment 162104 [details]
Patch

[Hin-Chung Lam]

Created attachment 162106 [details]
Patch

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 162338 [details]
Patch

[Hin-Chung Lam]

New patch to merge with ToT.

[Nick Carter]

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.

[Nick Carter]

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.

[Hin-Chung Lam]

Created attachment 162634 [details]
Patch

[Hin-Chung Lam]

I have updated the patch with the problems fixed. I have included the fixes for scaled image caching also in this patch.

[Hin-Chung Lam]

Created attachment 162639 [details]
Patch

[Hin-Chung Lam]

Created attachment 163429 [details]
Patch

[Hin-Chung Lam]

Merged again because of TestExpectations..

[Nick Carter]

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.

[WebKit Review Bot]

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

[Hin-Chung Lam]

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

[Hin-Chung Lam]

Created attachment 163696 [details]
Patch

[Hin-Chung Lam]

New patch with comments addressed and try to green cr-linux EWS bot.

[WebKit Review Bot]

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

[Stephen White]

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?

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 164059 [details]
Patch

[Hin-Chung Lam]

Created attachment 164062 [details]
Patch

[Stephen White]

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?

[Mike Reed]

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?

[Hin-Chung Lam]

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

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

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

[Hin-Chung Lam]

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

[Mike Reed]

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.

[Hin-Chung Lam]

Created attachment 164983 [details]
Patch

[Hin-Chung Lam]

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.

[Stephen White]

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!

[WebKit Review Bot]

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

[Hin-Chung Lam]

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.

[James Robinson]

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

[Mike Reed]

skia fix for drawBitmapRectToRect is in 5663

[Hin-Chung Lam]

Skia roll in Chromium is just landed. I'll roll DEPS in WebKit shortly and then update this patch accordingly (test expectations).

[Hin-Chung Lam]

Actually Skia roll for Chromium hasn't landed yet, looks like Mike is fixing it.

[Mike Reed]

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

[Hin-Chung Lam]

Will do.

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Mike: any updates on the skia fix?

[Mike Reed]

Was not able to dig further today. Hope to resolve it early next week.

[Hin-Chung Lam]

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.

[Mike Reed]

SGTM -- I thought the skia fix would happen quicker, but it is taking longer than I hoped.

[Hin-Chung Lam]

Thanks Mike! I'll upload a new patch.

[Stephen White]

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

[Hin-Chung Lam]

Created attachment 166942 [details]
Patch

[Hin-Chung Lam]

Switched back to having an explicit clip. We should fix the performance problem as a follow up fix.

[WebKit Review Bot]

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

[Hin-Chung Lam]

Created attachment 166961 [details]
Patch

[Hin-Chung Lam]

One more suppression. Verified the results just need rebaseline. It should be good this time.

[Hin-Chung Lam]

All bots are green now. Please take a look.

[Stephen White]

Comment on attachment 166961 [details]
Patch

r=me.  Please watch the page cyclers on landing.

[Hin-Chung Lam]

Created attachment 167140 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167140 [details]
Patch for landing

Clearing flags on attachment: 167140

Committed r130412: <http://trac.webkit.org/changeset/130412>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Phew.. perf bots are green!

[Stephen White]

(In reply to comment #66)
> Phew.. perf bots are green!

w00t!