Animated scaling of background-image is too slow
Created attachment 46849 [details] Patch
Attachment 46849 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/198242
../../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 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.
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.
Created attachment 47549 [details] Patch
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.
Attachment 47549 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/216134
(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 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.
Yeah, the style doesn't matter, but the bots seem to think this will break Chromium due to a missing wtf/CurrentTime.h include.
Committed r53949: <http://trac.webkit.org/changeset/53949>
(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.
Thank you Darin!
Chromium bustage fix is here: http://trac.webkit.org/changeset/53951
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?
Created attachment 47638 [details] Pixel output diff after this patch. Here's the pixel diff. So what say you? Rebaseline? Real regression?
Created attachment 47639 [details] fast/backgrounds/backgroundSize16 pixel diff after the patch. Actual png this time.
Rebaseline landed as http://trac.webkit.org/changeset/54013.
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.
Rolled out in http://trac.webkit.org/changeset/54075
Created attachment 47800 [details] Patch
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
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.
Committed r54113: <http://trac.webkit.org/changeset/54113>
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?
Still no comments? Oliver? Simon?
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>
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.