Bug 39085 – Resized Images are not cached in Chromium (Skia)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Sergey Ulanov]

Skia version of Image::drawPattern() resizes images using bicubic scaling and doesn't cache results. This causes very poor performance when playing HTML5 video on YouTube when zoom factor is different from 1.0. Problem is that the video controls are refreshed after each frame and this requires resizing background image for the controls.

[Sergey Ulanov]

Created attachment 56025 [details]
Fix for this bug.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/skia/ImageSkia.cpp:350:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/skia/ImageSkia.cpp:350:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/skia/ImageSkia.cpp:351:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/skia/ImageSkia.cpp:352:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 4 in 1 files


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

[Sergey Ulanov]

Created attachment 56035 [details]
Fix for this bug.

[Brett Wilson (Google)]

This patch looks good to me (note that I'm not a real WebKit reviewer).

[Dmitry Titov]

Comment on attachment 56035 [details]
Fix for this bug.

I can't check the functional validity. If Brett thinks it's ok and layout tests with pixel compare don't fail it could be ok.

Some style issues:
The change needs a ChangeLog (hence r-)


> +                || bitmap->shouldCacheResampling(static_cast<int>(destBitmapWidth), static_cast<int>(destBitmapHeight),
> +                                                 static_cast<int>(destBitmapWidth), static_cast<int>(destBitmapHeight))) ) {
I don't think the space before the closing ')' is needed.

Could I suggest all those casts to be moved to the top, initializing some local 'destWidth' and 'destHeight', so we don't have a dozen of them repeated everywhere?

> +            // resize() caches resized images.
> +            resampled = bitmap->resizedBitmap(static_cast<int>(destBitmapWidth),
> +                static_cast<int>(destBitmapHeight));

Once the casts are gone, this line can be un-wrapped.

[Sergey Ulanov]

Created attachment 56052 [details]
Patch, addressed review comments.

[Sergey Ulanov]

Created attachment 56053 [details]
Patch.

[Sergey Ulanov]

Created attachment 56054 [details]
Patch

Added link to the bug in the ChangeLog.

[Sergey Ulanov]

Created attachment 56055 [details]
Patch.

[Dmitry Titov]

Comment on attachment 56055 [details]
Patch.

r=me

[Dmitry Titov]

Comment on attachment 56055 [details]
Patch.

Going to land this manually for Sergey. removing cq+.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/59487