Bug 33808 - Animated scaling of background-image is too slow
Summary: Animated scaling of background-image is too slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-18 13:10 PST by Oliver Hunt
Modified: 2010-04-27 18:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.20 KB, patch)
2010-01-18 14:13 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (14.89 KB, patch)
2010-01-27 10:45 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Pixel output diff after this patch. (133 bytes, application/octet-stream)
2010-01-28 11:16 PST, Dimitri Glazkov (Google)
no flags Details
fast/backgrounds/backgroundSize16 pixel diff after the patch. (25.82 KB, image/png)
2010-01-28 11:29 PST, Dimitri Glazkov (Google)
no flags Details
Patch (16.73 KB, patch)
2010-01-31 14:51 PST, Oliver Hunt
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2010-01-18 13:10:37 PST
Animated scaling of background-image is too slow
Comment 1 Oliver Hunt 2010-01-18 14:13:23 PST
Created attachment 46849 [details]
Patch
Comment 2 WebKit Review Bot 2010-01-18 16:37:35 PST
Attachment 46849 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/198242
Comment 3 Eric Seidel (no email) 2010-01-19 14:26:51 PST
../../WebCore/rendering/RenderBoxModelObject.cpp: In static member function ‘static bool WebCore::RenderBoxModelScaleObserver::shouldPaintBackgroundAtLowQuality(WebCore::RenderBoxModelObject*, WebCore::Image*, const WebCore::IntSize&)’:
../../WebCore/rendering/RenderBoxModelObject.cpp:147: error: ‘currentTime’ was not declared in this scope
../../WebCore/rendering/RenderBoxModelObject.cpp:156: error: ‘currentTime’ was not declared in this scope

Looks like it's missing an include?  <wtf/CurrentTime.h>
Comment 4 Maciej Stachowiak 2010-01-22 02:09:00 PST
Comment on attachment 46849 [details]
Patch

r=me but please unbreak Chromium before landing.
Comment 5 mitz 2010-01-22 07:15:33 PST
Comment on attachment 46849 [details]
Patch

> +    if (false && imageSize == size) {

Please don’t check in this code.
Comment 6 Oliver Hunt 2010-01-22 11:11:42 PST
Comment on attachment 46849 [details]
Patch

sorry, this shouldn't have been r? -- hyatt and i had been discussing a sensible fix and aren't convinced that this approach is right.
Comment 7 Oliver Hunt 2010-01-27 10:45:28 PST
Created attachment 47549 [details]
Patch
Comment 8 WebKit Review Bot 2010-01-27 10:47:15 PST
Attachment 47549 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/transforms/TransformationMatrix.h:324:  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/transforms/TransformationMatrix.h:324:  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/transforms/TransformationMatrix.h:325:  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/transforms/TransformationMatrix.h:325:  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/transforms/TransformationMatrix.h:326:  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/transforms/TransformationMatrix.h:326:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 WebKit Review Bot 2010-01-27 10:56:49 PST
Attachment 47549 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/216134
Comment 10 Oliver Hunt 2010-01-27 10:57:17 PST
(In reply to comment #8)
> Attachment 47549 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/platform/graphics/transforms/TransformationMatrix.h:324:  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/transforms/TransformationMatrix.h:324:  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/transforms/TransformationMatrix.h:325:  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/transforms/TransformationMatrix.h:325:  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/transforms/TransformationMatrix.h:326:  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/transforms/TransformationMatrix.h:326:  Tests for
> true/false, null/non-null, and zero/non-zero should all be done without
> equality comparisons.  [readability/comparison_to_zero] [5]
> Total errors found: 6
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.
This is just moved code, i'm hesitant to reformat it for this reason, but can if necessary
Comment 11 Simon Fraser (smfr) 2010-01-27 11:50:47 PST
Comment on attachment 47549 [details]
Patch


> +    bool isIdentityOrTranslation() const
> +    {
> +        return m_matrix[0][0] == 1 && m_matrix[0][1] == 0 && m_matrix[0][2] == 0 && m_matrix[0][3] == 0 &&
> +        m_matrix[1][0] == 0 && m_matrix[1][1] == 1 && m_matrix[1][2] == 0 && m_matrix[1][3] == 0 &&
> +        m_matrix[2][0] == 0 && m_matrix[2][1] == 0 && m_matrix[2][2] == 1 && m_matrix[2][3] == 0 &&
> +        m_matrix[3][3] == 1;

Since you moved this you should fix the style (indentation on the following lines, and && at the stat of the line).

> diff --git a/WebCore/rendering/RenderBoxModelObject.cpp b/WebCore/rendering/RenderBoxModelObject.cpp

> +    const IntSize& size() const { return m_size; }
> +    double time() const { return m_time; }

What is "time"? Would be good to rename to make it more understandable. Yes, you're just moving this but I think it's a good opportunity to improve it.

I prefer getters and setters next to eachother.

> +    static void highQualityRepaintTimerFired(RenderBoxModelObject* object)
> +    {
> +        RenderBoxModelScaleObserver::boxModelObjectDestroyed(object);
> +        object->repaint();

Can this repaint be optimized to only repaint the part of the box covered by the image?

r=me with those changes.
Comment 12 Eric Seidel (no email) 2010-01-27 11:51:40 PST
Yeah, the style doesn't matter, but the bots seem to think this will break Chromium due to a missing wtf/CurrentTime.h include.
Comment 13 Oliver Hunt 2010-01-27 13:14:15 PST
Committed r53949: <http://trac.webkit.org/changeset/53949>
Comment 14 Darin Fisher (:fishd, Google) 2010-01-27 13:29:01 PST
(In reply to comment #12)
> Yeah, the style doesn't matter, but the bots seem to think this will break
> Chromium due to a missing wtf/CurrentTime.h include.

And indeed the bots did break.  I'm working on a fix.
Comment 15 Eric Seidel (no email) 2010-01-27 13:33:07 PST
Thank you Darin!
Comment 16 Darin Fisher (:fishd, Google) 2010-01-27 13:36:59 PST
Chromium bustage fix is here:
http://trac.webkit.org/changeset/53951
Comment 17 Dimitri Glazkov (Google) 2010-01-28 10:51:22 PST
This patch also changed the output of fast/backgrounds/size/backgroundSize16.html. I can't tell if it's a regression or not. Do you want me to just update the baseline?
Comment 18 Dimitri Glazkov (Google) 2010-01-28 11:16:47 PST
Created attachment 47638 [details]
Pixel output diff after this patch.

Here's the pixel diff. So what say you? Rebaseline? Real regression?
Comment 19 Dimitri Glazkov (Google) 2010-01-28 11:29:04 PST
Created attachment 47639 [details]
fast/backgrounds/backgroundSize16 pixel diff after the patch.

Actual png this time.
Comment 20 Dimitri Glazkov (Google) 2010-01-28 13:26:15 PST
Rebaseline landed as http://trac.webkit.org/changeset/54013.
Comment 21 Darin Fisher (:fishd, Google) 2010-01-29 12:11:21 PST
Unfortunately, this change causes an endless repaint loop on Chromium's new tab page.  That in turn disrupts a number of Chromium tests and automation associated with the new tab page.  This is keeping the Chromium tree closed, so I am inclined to back out this change since many developers are blocked from doing work.  I will be posting a reduced test case that demonstrates the problem.  Oliver OK'd the backout in #webkit.
Comment 22 Darin Fisher (:fishd, Google) 2010-01-29 12:22:28 PST
Rolled out in http://trac.webkit.org/changeset/54075
Comment 23 Oliver Hunt 2010-01-31 14:51:43 PST
Created attachment 47800 [details]
Patch
Comment 24 Oliver Hunt 2010-01-31 14:56:24 PST
Only change to this updated version of the patch is that we now only update the animation time if the transform is changing.  Otherwise what could happen is that the high quality repaint timer would trigger a return to low quality mode if there was still a scaling transform applied to the ctm.  This would then re-register the high quality timeout, and so result in endless repainting.  This effected chromium but i was never able to reproduce it locally so couldn't ever create a test.  Confirmed the issue was fixed via dglazkov with his chrome build.

The functional change from the earlier patch is that this check
if (!contextIsScaled && data->size() == size)
becomes
if ((!contextIsScaled || data->transform() == currentTransform) && data->size() == size)

--Oliver
Comment 25 WebKit Review Bot 2010-01-31 14:59:45 PST
Attachment 47800 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/transforms/TransformationMatrix.h:328:  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/transforms/TransformationMatrix.h:329:  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/transforms/TransformationMatrix.h:330:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Oliver Hunt 2010-01-31 15:01:13 PST
Committed r54113: <http://trac.webkit.org/changeset/54113>
Comment 27 Nikolas Zimmermann 2010-01-31 15:08:19 PST
I don't like that this patch clutters RenderBoxModelObject even more, it's the wrong direction.
Can you please split up the code into a new RenderBoxModelObjectScaleData.h header?
Comment 28 Nikolas Zimmermann 2010-02-01 04:17:16 PST
Still no comments? Oliver? Simon?
Comment 29 James Robinson 2010-02-02 18:31:56 PST
This breaks http://www.bing.com/reference/semhtml/Navagio.  Reduction here:

<!DOCTYPE html> 
<div style="border-bottom:1px solid; background:-webkit-gradient(linear,0% 100%, 0% 100%, from(red),to(blue))"> 
</div>
Comment 30 James Robinson 2010-04-27 18:01:21 PDT
The issue I referred to in the previous comment was filed as bug https://bugs.webkit.org/show_bug.cgi?id=34510 and fixed at http://trac.webkit.org/changeset/54272.