Bug 33808 – Animated scaling of background-image is too slow

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Animated scaling of background-image is too slow

[Oliver Hunt]

Created attachment 46849 [details]
Patch

[WebKit Review Bot]

Attachment 46849 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/198242

[Eric Seidel (no email)]

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

[Maciej Stachowiak]

Comment on attachment 46849 [details]
Patch

r=me but please unbreak Chromium before landing.

Comment on attachment 46849 [details]
Patch

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

Please don’t check in this code.

[Oliver Hunt]

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.

[Oliver Hunt]

Created attachment 47549 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 47549 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/216134

[Oliver Hunt]

(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

[Simon Fraser (smfr)]

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.

[Eric Seidel (no email)]

Yeah, the style doesn't matter, but the bots seem to think this will break Chromium due to a missing wtf/CurrentTime.h include.

[Oliver Hunt]

Committed r53949: <http://trac.webkit.org/changeset/53949>

[Darin Fisher (:fishd, Google)]

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

[Eric Seidel (no email)]

Thank you Darin!

[Darin Fisher (:fishd, Google)]

Chromium bustage fix is here:
http://trac.webkit.org/changeset/53951

[Dimitri Glazkov (Google)]

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?

[Dimitri Glazkov (Google)]

Created attachment 47638 [details]
Pixel output diff after this patch.

Here's the pixel diff. So what say you? Rebaseline? Real regression?

[Dimitri Glazkov (Google)]

Created attachment 47639 [details]
fast/backgrounds/backgroundSize16 pixel diff after the patch.

Actual png this time.

[Dimitri Glazkov (Google)]

Rebaseline landed as http://trac.webkit.org/changeset/54013.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Rolled out in http://trac.webkit.org/changeset/54075

[Oliver Hunt]

Created attachment 47800 [details]
Patch

[Oliver Hunt]

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

[WebKit Review Bot]

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.

[Oliver Hunt]

Committed r54113: <http://trac.webkit.org/changeset/54113>

[Nikolas Zimmermann]

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?

[Nikolas Zimmermann]

Still no comments? Oliver? Simon?

[James Robinson]

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>

[James Robinson]

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.